Opened 10 years ago

Closed 9 years ago

#5408 closed bug (fixed)

Rename TR* macros

Reported by: bonefish Owned by: pulkomandy
Priority: normal Milestone: R1
Component: Kits/Locale Kit Version: R1/Development
Keywords: Cc:
Blocked By: #5927, #5928 Blocking:
Has a Patch: no Platform: All

Description

The TR* macros need to be renamed to not pollute the global namespace. A simple solution would be to add a B_ prefix, although more descriptive names (e.g. B_TRANSLATE instead of TR) wouldn't harm either.

Attachments (3)

5408.patch (3.3 KB ) - added by mmadia 9 years ago.
possible new macros
grep-TR_.txt (7.3 KB ) - added by mmadia 9 years ago.
grep -r [\(\t\ ]TR_[C][O][N] . | sed -e '/.svn/d'
grep-TR.txt (10.5 KB ) - added by mmadia 9 years ago.
grep -r [\(\t\ ]TR\( . | sed -e '/.svn/d'

Download all attachments as: .zip

Change History (15)

comment:1 by pulkomandy, 10 years ago

If they annoy you, you can disable them in any file with a simple #define. They have a short name on purpose, because they are used all over the place in a localized app. As an example, wxWidgets use _(), wxT(), and _T() for the same purpose, which is even less descriptive...

in reply to:  1 comment:2 by bonefish, 10 years ago

Replying to pulkomandy:

If they annoy you, you can disable them in any file with a simple #define.

It's really not relevant whether or not they annoy me. They are Haiku specific API and thus should live in an appropriate namespace (B_* in this case).

They have a short name on purpose, because they are used all over the place in a localized app.

Well, B_TRANSLATE would still be relatively short and would be a descriptive name. It's one thing to use a short macro name in some source file (for kernel tracing T is used, sometimes with a second letter, if there are different kinds of tracing in one file), but a completely different thing to define it in a public API.

As an example, wxWidgets use _(), wxT(), and _T() for the same purpose, which is even less descriptive...

No reason to copy bad API decisions, right?

comment:3 by stippi, 10 years ago

If it helps, Qt also uses a rather verbose "string wrapper".

comment:4 by axeld, 10 years ago

I wouldn't even mind a shorter name, but the namespace problem is indeed something that we cannot ignore (IMO _T() wouldn't be too bad either, as the _* namespace is already a system namespace).

While I wouldn't mind calling it B_TRANSLATE(), I could assume that this might reduce source readability when used a lot. I think we could have both, though, a short version in the system namespace, and a long version in the B_* namespace.

comment:5 by bonefish, 10 years ago

I'm not so sure about _* being a (generally recognized) system namespace (unlike __*). At least there are public functions like _exit() in it, third party libraries often make use of it (for private stuff), and actually our coding style requires to use it for private methods (different casing would prevent clashes with macros in this case, though).

Regarding readability, I grepped a bit through the preferences and apps sources and the usage density of TR() seems to be moderate to low. With the exception of alerts one has rarely more than one translation per statement, so I wouldn't be too concerned about readability. At any rate I don't see a problem with defining and using a TR() macro in a source file, if that improves readability.

comment:6 by axeld, 10 years ago

FWIW you convinced me to use a B_TRANSLATE() macro instead.

by mmadia, 9 years ago

Attachment: 5408.patch added

possible new macros

comment:7 by mmadia, 9 years ago

How's the patch look? I'm thinking, given the size of the task, it'd be better to define both macros during the transitional phase.

If looks good, i'll start crawling the code. Should I send the first few patches here or just commit them directly?

comment:8 by axeld, 9 years ago

I would also rename B_TRANSLATE_CMT to B_TRANSLATE_COMMENT, but the rest looks good IMO. Thanks!

in reply to:  8 comment:9 by mmadia, 9 years ago

Replying to axeld:

rename B_TRANSLATE_CMT to B_TRANSLATE_COMMENT

Will do. I'll start in src/add-ons and will go in alphabetical order... so if anyone else cares to lend a hand, please do.

comment:10 by stippi, 9 years ago

TR_MARK_CMT, TR_MARK_ALL and TR_MARK_ID have the wrong B_TRANSLATE_* equivalent. Otherwise looks fine.

by mmadia, 9 years ago

Attachment: grep-TR_.txt added

grep -r [\(\t\ ]TR_[C][O][N] . | sed -e '/.svn/d'

by mmadia, 9 years ago

Attachment: grep-TR.txt added

grep -r [\(\t\ ]TR\( . | sed -e '/.svn/d'

comment:11 by mmadia, 9 years ago

Blocked By: 5927, 5928 added

The two above attachments show what I believe to be the remaining bits. Two directories ( src/tests/kits/locale/ & src/apps/installedpackages/ ) do not build, have tickets reported ( #5927 & #5928 respectively), & have TR-->B_TRANSLATE patches attached.

As for cleaning up headers/os/locale/Catalog.h , i'd prefer someone else to do it or tell me the which parts should be deleted -- eg, just the #define TR[.*] lines or the #undef TR[.*] lines as well or ...

comment:12 by pulkomandy, 9 years ago

Resolution: fixed
Status: newclosed

Apparently this was already fixed.

Note: See TracTickets for help on using tickets.