GNOME Bugzilla – Bug 386118
intltool-update cannot pick up '_ ("")'
Last modified: 2006-12-20 16:22:39 UTC
intltool-update -m does not detect _ function if it has the space char. To reproduce: % cat a.c _("test1"); % cat b.c _ ("test2"); % cd po % echo > POTFILES.in % intltool-update -m Then a.c is detected but b.c is not detected. I'm attaching the patch.
Created attachment 78417 [details] [review] Patch for intltool-update.in.in xgettext(intltool-update -p) can extract the messages.
Wouldn't the addition of " ?" be a better patch than using " *"? This at least matches the .GetString check in the if statement directly above the affected code here.
I'm not sure the part of .GetString however if " ?" is used, '_ ("");' failed to be detected, i.e. two spaces. What do you think?
I think if the messages aren't being extracted, it's a bug in gettext. The code you changed is only called for the maintainer mode of intltool-update (-m). Intltool doesn't extract strings from C files. With your patch, I imagine that _ FOO ("") will also get picked up, no?
> I think if the messages aren't being extracted, it's a bug in gettext. The code you changed is only called for the maintainer mode of intltool-update (-m). The fix is for the part of "intltool-update -m" only. xgettext(intltool-update -p) works fine without this patch. > Intltool doesn't extract strings from C files. Hmm.., what do you mean in this? When I had tests, xgettext(intltool-update -p) is no problem. > With your patch, I imagine that _ FOO ("") will also get picked up, no? No, the patch can detects C files which includes '_("")', '_ ("")' and '_ ("")'. I had tests with your case(_ FOO ("")).
(In reply to comment #5) > > Intltool doesn't extract strings from C files. > > Hmm.., what do you mean in this? When I had tests, xgettext(intltool-update -p) > is no problem. Your previous comments lead to confusion (especially the fact that you say "xgettext(intltool-update -p) can extract comment" in your comment for the attachment). > > With your patch, I imagine that _ FOO ("") will also get picked up, no? > > No, the patch can detects C files which includes '_("")', '_ ("")' and '_ > ("")'. > I had tests with your case(_ FOO ("")). OK. I was confused about * vs .* it seems. However RANDOM_DEFINE_ ("foo") will get picked up, even if RANDOM_DEFINE_ has nothing to do with translations. This is bad. We need to limit the check to the known macros such as N_ Q_ and _. And I think we need to limit the C# check immediately above this check, to object.GetString, although, the case for that being an issue is much smaller.
In fact, we need to handle all of the following: {FOO, N_("foo")} {FOO,N_("foo")} {N_("foo"), FOO)} foo = foo (_("foo")); foo = foo ( _("foo") ); foo ("%s", _("foo")); foo ("%s",_("foo")); Any idea how best to deal with this? There could be a few more that I'm just not thinking of right now, but these should cover our bases pretty good. The "/[NQ]?_ *\(/" part is pretty easy, but with all the (, {, and , possibilities, things are a bit more hectic. I have "/\(?\{?,?[NQ]?_ *\(QUOTEDTEXT/" right now in my local tree, but it doesn't work as we need it to, of course.
OK, forgot a couple cases: foo=_("foo"); foo?_("foo"):_("bar"); Whitespace implied plausible here. This leaves me at the following regex, which seems to cover our bases pretty well: /[\s\(\{\:\?\=,][NQ]?_ *\(QUOTEDTEXT/ I wonder if there's anything else I'm missing here? I'd like to commit this and get a release out asap.
Created attachment 78601 [details] [review] Patch to fix regex to check for _() N_() and Q_() more properly
> Your previous comments lead to confusion (especially the fact that you say "xgettext(intltool-update -p) can extract comment" in your comment for the attachment). Sorry for the short notice. I meant '-p' is ok so we need to fix '-m', too. > I wonder if there's anything else I'm missing here? I'd like to commit this and get a release out asap. I guess when the strings are even enclosed with comment tags, /* ... */, those strings can be detected ;). How about '+' to concatenate strings? How about '$(foo)_ ("foo")' for foo.py.in with configure?
As for the python stuff, I imagine we will have to add another check, to match the syntax of that language. What I'm trying to get working right now is the C/C++ part, given that's what 90%+ of the desktop uses. As far as things being commented out, I think claiming they are missing translations is fine. It at least points out bad coding practices, where as other cases are just entirely false.
I think /[\s\(\{\:\?\=,][NQ]?_ *\(QUOTEDTEXT/ is risky. It cannot detect "_(". e.g. a = _("foo"); '/[NQ]?_ *\(QUOTEDTEXT/' is ok with me for the most cases. Yes, 'AN_("foo")' can be detected but I think it's not so critical problem.
OK, I've reduced the check back to [NQ]?_ for now just to avoid the possibility of breaking too many things, since it seems like nobody else is actually going to test the patch, and I want to get at least this fix in and released. I also included the small changes for the .GetString for C# check as well.