GNOME Bugzilla – Bug 740921
Some strings are not correctly marked for translation
Last modified: 2016-03-31 13:22:07 UTC
The following strings are not marked correctly for translation. As a result, GNOME Boxes does not present translated messages for them. diff --git a/src/i-properties-provider.vala b/src/i-properties-provider.vala index 17b61a9..7861c29 100644 --- a/src/i-properties-provider.vala +++ b/src/i-properties-provider.vala @@ -98,7 +98,7 @@ public SizeProperty (string name, scale.add_mark (min, Gtk.PositionType.BOTTOM, format_size (min, format_flags)); scale.add_mark (max, Gtk.PositionType.BOTTOM, - "%s (maximum)".printf (format_size (max, format_flags))); + _("%s (maximum)").printf (format_size (max, format_flags))); >>> This is not wrapped in the gettext function. diff --git a/src/properties.vala b/src/properties.vala index b20dc9b..39ea3ec 100644 --- a/src/properties.vala +++ b/src/properties.vala @@ -130,7 +130,7 @@ private void populate () { for (var i = 0; i < PropertiesPage.LAST; i++) { var page = new PageWidget (i, machine); - var label = new Gtk.Label (page_titles[i]); + var label = new Gtk.Label (_(page_titles[i])); >>> This is not wrapped in the gettext function. diff --git a/src/wizard.vala b/src/wizard.vala index e2d15ab..aa85b73 100644 --- a/src/wizard.vala +++ b/src/wizard.vala @@ -514,7 +514,7 @@ yield run_in_thread (() => { try { var volume_info = libvirt_machine.storage_volume.get_info (); var capacity = format_size (volume_info.capacity); - summary.add_property (_("Disk"), _("%s maximum".printf (capacity))); + summary.add_property (_("Disk"), _("%s maximum").printf (capacity)); >>> This is incorrectly wrapped.
Created attachment 291826 [details] [review] Make some UI strings translatable
Review of attachment 291826 [details] [review]: Hi, thanks for taking the time to provide a patch! ::: src/i-properties-provider.vala @@ +99,3 @@ scale.add_mark (min, Gtk.PositionType.BOTTOM, format_size (min, format_flags)); scale.add_mark (max, Gtk.PositionType.BOTTOM, + _("%s (maximum)").printf (format_size (max, format_flags))); I think we need a comment for the translators here, explaining with what the %s will be replaced and what the maximum refers to. (There may be different forms of the translation for male/female singular/plural forms.) IIRC if you place a comment directly in the line before this command gettext should recognize that. zeenix should know this better than me, maybe he can confirm. ::: src/properties.vala @@ +131,3 @@ for (var i = 0; i < PropertiesPage.LAST; i++) { var page = new PageWidget (i, machine); + var label = new Gtk.Label (_(page_titles[i])); If you look at line 14, the actual values of the page_titles array already run through N_() - which seems to be gettext_noop. Translating this in this place is the wrong thing to do IMO. You could git blame to find out why they used the N_() in line 14 instead of _() and maybe adjust it to get it translated. ::: src/wizard.vala @@ +518,1 @@ } catch (GLib.Error error) { good catch
Review of attachment 291826 [details] [review]: Being a nitpicker, I'd also suggest a slight improvement to shortlog: Mark some UI strings for translation. ::: src/i-properties-provider.vala @@ +99,3 @@ scale.add_mark (min, Gtk.PositionType.BOTTOM, format_size (min, format_flags)); scale.add_mark (max, Gtk.PositionType.BOTTOM, + _("%s (maximum)").printf (format_size (max, format_flags))); Yes, gettext picks up the comments from above or the same line even. Jiro, here is the comment you want here: // Translators: This is memory or disk size. E.g "1 GB (maximum)". ::: src/properties.vala @@ +131,3 @@ for (var i = 0; i < PropertiesPage.LAST; i++) { var page = new PageWidget (i, machine); + var label = new Gtk.Label (_(page_titles[i])); No need to check anything. Lasse is correct that these are translated using N_() as they should be. ::: src/wizard.vala @@ +518,1 @@ } catch (GLib.Error error) { Yeah, the same comment here would be nice too.
Review of attachment 291826 [details] [review]: ::: src/properties.vala @@ +131,3 @@ for (var i = 0; i < PropertiesPage.LAST; i++) { var page = new PageWidget (i, machine); + var label = new Gtk.Label (_(page_titles[i])); Actually I did need to check the docs myself. In this case you want to call gettext directly: https://developer.gnome.org/glib/stable/glib-I18N.html#N-:CAPS .
Hi Lasse, Zeeshan, (In reply to comment #4) > Review of attachment 291826 [details] [review]: > > ::: src/properties.vala > @@ +131,3 @@ > for (var i = 0; i < PropertiesPage.LAST; i++) { > var page = new PageWidget (i, machine); > + var label = new Gtk.Label (_(page_titles[i])); > > Actually I did need to check the docs myself. In this case you want to call > gettext directly: > https://developer.gnome.org/glib/stable/glib-I18N.html#N-:CAPS . If I misunderstand you, please correct me. I understand that N_() does not translate arguments. It does nothing at runtime unlike _(). It is used just to mark strings as translatable for extractors (intltool or xgettext) to put the strings into the PO template in the case where _() cannot be used. I think you use N_() for the page titles, because they become static variables out of any function block in the C layer and calling _() is not allowed there. We need to use _() for each page title to translate it at runtime. See also the GNU gettext manual[1]. _() is equievalent to gettext(). So both are OK, but I believe _() is more popular and better in respect of readability. Thank you. [1]https://www.gnu.org/software/gettext/manual/gettext.html#Special-cases
(In reply to comment #5) > Hi Lasse, Zeeshan, > > (In reply to comment #4) > > Review of attachment 291826 [details] [review] [details]: > > > > ::: src/properties.vala > > @@ +131,3 @@ > > for (var i = 0; i < PropertiesPage.LAST; i++) { > > var page = new PageWidget (i, machine); > > + var label = new Gtk.Label (_(page_titles[i])); > > > > Actually I did need to check the docs myself. In this case you want to call > > gettext directly: > > https://developer.gnome.org/glib/stable/glib-I18N.html#N-:CAPS . > > If I misunderstand you, please correct me. > > I understand that N_() does not translate arguments. It does nothing at runtime > unlike _(). It is used just to mark strings as translatable for extractors > (intltool or xgettext) to put the strings into the PO template in the case > where _() cannot be used. I think you use N_() for the page titles, because > they become static variables out of any function block in the C layer and > calling _() is not allowed there. We need to use _() for each page title to > translate it at runtime. See also the GNU gettext manual[1]. > > _() is equievalent to gettext(). So both are OK, but I believe _() is more > popular and better in respect of readability. > > Thank you. > > [1]https://www.gnu.org/software/gettext/manual/gettext.html#Special-cases The example there is pretty similar to the one in the glib manual I linked to above. So what you need to do is: 1. Leave the N_() like it is now. 2. Replace the _() with gettext() where the array member is used. P.S. Not a big deal or anything but in future kindly reply inline to inline comments using the 'Review' link. It just makes the context very clear.
(In reply to comment #5) > If I misunderstand you, please correct me. > > I understand that N_() does not translate arguments. It does nothing at runtime > unlike _(). It is used just to mark strings as translatable for extractors > (intltool or xgettext) to put the strings into the PO template in the case > where _() cannot be used. I think you use N_() for the page titles, because > they become static variables out of any function block in the C layer and > calling _() is not allowed there. We need to use _() for each page title to > translate it at runtime. See also the GNU gettext manual[1]. > > _() is equievalent to gettext(). So both are OK, but I believe _() is more > popular and better in respect of readability. > > Thank you. > > [1]https://www.gnu.org/software/gettext/manual/gettext.html#Special-cases I think this is all right. Considering this your original fix is actually very valid. (I've done too much python lately so I didn't spot it as exact as you did.) However I think it would be a better solution to make the page_titles array not constant and translate it right there. * It reduces complexity - if you use values of this array you don't expect that you have to translate it and that leads to bugs as this one. * If the array is used at several places we may save one or two gettext lookups (ok, not really relevant - just mentioning) Because of the complexity of this matter I think an own commit with an explanation would be good. Actually I'm in favor of reusing the page_titles array in the PageWidget subclass, replacing the following code (line 32 ff.): switch (page) { case PropertiesPage.GENERAL: name = _("General"); break; case PropertiesPage.SYSTEM: name = _("System"); break; case PropertiesPage.DEVICES: name = _("Devices"); break; case PropertiesPage.SNAPSHOTS: name = _("Snapshots"); break; } by: if (page < 4 && page >= 0) name = Properties.page_titles[page]; This way we have less static strings in the code (-> making it better maintainable) and less code in general (-> making it better maintainable) thus better code quality while maybe even saving one or more gettext invocations :). This would be an own commit too.
(In reply to comment #7) > (In reply to comment #5) > > If I misunderstand you, please correct me. > > > > I understand that N_() does not translate arguments. It does nothing at runtime > > unlike _(). It is used just to mark strings as translatable for extractors > > (intltool or xgettext) to put the strings into the PO template in the case > > where _() cannot be used. I think you use N_() for the page titles, because > > they become static variables out of any function block in the C layer and > > calling _() is not allowed there. We need to use _() for each page title to > > translate it at runtime. See also the GNU gettext manual[1]. > > > > _() is equievalent to gettext(). So both are OK, but I believe _() is more > > popular and better in respect of readability. > > > > Thank you. > > > > [1]https://www.gnu.org/software/gettext/manual/gettext.html#Special-cases > > I think this is all right. Considering this your original fix is actually very > valid. (I've done too much python lately so I didn't spot it as exact as you > did.) > > However I think it would be a better solution to make the page_titles array not > constant and translate it right there. > > * It reduces complexity - if you use values of this array you don't expect > that you have to translate it and that leads to bugs as this one. > * If the array is used at several places we may save one or two gettext > lookups (ok, not really relevant - just mentioning) > > Because of the complexity of this matter I think an own commit with an > explanation would be good. > > Actually I'm in favor of reusing the page_titles array in the PageWidget > subclass, replacing the following code (line 32 ff.): > > switch (page) { > case PropertiesPage.GENERAL: > name = _("General"); > break; > > case PropertiesPage.SYSTEM: > name = _("System"); > break; > > case PropertiesPage.DEVICES: > name = _("Devices"); > break; > > case PropertiesPage.SNAPSHOTS: > name = _("Snapshots"); > break; > } > > by: > > if (page < 4 && page >= 0) > name = Properties.page_titles[page]; Actually in a patch coming soon to git master, I'm removing this array itself so your fix is only for 3.14 branch. So unless I'm wrong, I'd prefer we simply do what I suggested in previous comment of mine. Alternatively, you can leave it as is and I can backport the patch to 3.14 as well: index b20dc9b..f3bb2fb 100644 --- a/src/properties.vala +++ b/src/properties.vala @@ -11,8 +11,6 @@ } private class Boxes.Properties: Gtk.Notebook, Boxes.UI { - private const string[] page_titles = { N_("General"), N_("System"), N_("Devices"), N_("Snapshots") }; - public UIState previous_ui_state { get; protected set; } public UIState ui_state { get; protected set; } @@ -130,7 +128,7 @@ private void populate () { for (var i = 0; i < PropertiesPage.LAST; i++) { var page = new PageWidget (i, machine); - var label = new Gtk.Label (page_titles[i]); + var label = new Gtk.Label (page.name);
Created attachment 292063 [details] [review] Make UI strings translatable Wrap UI strings correctly in the gettext function and add comments for traslators. I split the previous patch to fix only the disk size messages.
Created attachment 292064 [details] [review] Update POTFILES.in/skip
(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #5) > > > If I misunderstand you, please correct me. > > > > > > I understand that N_() does not translate arguments. It does nothing at runtime > > > unlike _(). It is used just to mark strings as translatable for extractors > > > (intltool or xgettext) to put the strings into the PO template in the case > > > where _() cannot be used. I think you use N_() for the page titles, because > > > they become static variables out of any function block in the C layer and > > > calling _() is not allowed there. We need to use _() for each page title to > > > translate it at runtime. See also the GNU gettext manual[1]. > > > > > > _() is equievalent to gettext(). So both are OK, but I believe _() is more > > > popular and better in respect of readability. > > > > > > Thank you. > > > > > > [1]https://www.gnu.org/software/gettext/manual/gettext.html#Special-cases > > > > I think this is all right. Considering this your original fix is actually very > > valid. (I've done too much python lately so I didn't spot it as exact as you > > did.) > > > > However I think it would be a better solution to make the page_titles array not > > constant and translate it right there. > > > > * It reduces complexity - if you use values of this array you don't expect > > that you have to translate it and that leads to bugs as this one. > > * If the array is used at several places we may save one or two gettext > > lookups (ok, not really relevant - just mentioning) > > > > Because of the complexity of this matter I think an own commit with an > > explanation would be good. > > > > Actually I'm in favor of reusing the page_titles array in the PageWidget > > subclass, replacing the following code (line 32 ff.): > > > > switch (page) { > > case PropertiesPage.GENERAL: > > name = _("General"); > > break; > > > > case PropertiesPage.SYSTEM: > > name = _("System"); > > break; > > > > case PropertiesPage.DEVICES: > > name = _("Devices"); > > break; > > > > case PropertiesPage.SNAPSHOTS: > > name = _("Snapshots"); > > break; > > } > > > > by: > > > > if (page < 4 && page >= 0) > > name = Properties.page_titles[page]; > > Actually in a patch coming soon to git master, I'm removing this array itself > so your fix is only for 3.14 branch. So unless I'm wrong, I'd prefer we simply > do what I suggested in previous comment of mine. > > Alternatively, you can leave it as is and I can backport the patch to 3.14 as > well: > > index b20dc9b..f3bb2fb 100644 > --- a/src/properties.vala > +++ b/src/properties.vala > @@ -11,8 +11,6 @@ > } > > private class Boxes.Properties: Gtk.Notebook, Boxes.UI { > - private const string[] page_titles = { N_("General"), N_("System"), > N_("Devices"), N_("Snapshots") }; > - > public UIState previous_ui_state { get; protected set; } > public UIState ui_state { get; protected set; } > > @@ -130,7 +128,7 @@ private void populate () { > > for (var i = 0; i < PropertiesPage.LAST; i++) { > var page = new PageWidget (i, machine); > - var label = new Gtk.Label (page_titles[i]); > + var label = new Gtk.Label (page.name); That's good. My patch is not needed any more for the page titles. I don't think 3.14 needs the change above. It seems to break a UI and string freeze, unless I'm wrong.
Review of attachment 292063 [details] [review]: Patch itself is good but I think the description doesn't apply to this patch (after the split). While you fix this, would be nice if you act on my nitpick about shortlog in the previous review as well. :)
Review of attachment 292064 [details] [review]: Looks good. Please only push once the other patch is pushed.
(In reply to comment #9) > Created an attachment (id=292063) [details] [review] > Make UI strings translatable > > Wrap UI strings correctly in the gettext function and add comments > for traslators. > > > I split the previous patch to fix only the disk size messages. Thanks but I don't see the fix for the property names anymore. Forgot to upload that patch?
(In reply to comment #11) > (In reply to comment #8) > > (In reply to comment #7) > > > (In reply to comment #5) > > > > If I misunderstand you, please correct me. > > > > > > > > I understand that N_() does not translate arguments. It does nothing at runtime > > > > unlike _(). It is used just to mark strings as translatable for extractors > > > > (intltool or xgettext) to put the strings into the PO template in the case > > > > where _() cannot be used. I think you use N_() for the page titles, because > > > > they become static variables out of any function block in the C layer and > > > > calling _() is not allowed there. We need to use _() for each page title to > > > > translate it at runtime. See also the GNU gettext manual[1]. > > > > > > > > _() is equievalent to gettext(). So both are OK, but I believe _() is more > > > > popular and better in respect of readability. > > > > > > > > Thank you. > > > > > > > > [1]https://www.gnu.org/software/gettext/manual/gettext.html#Special-cases > > > > > > I think this is all right. Considering this your original fix is actually very > > > valid. (I've done too much python lately so I didn't spot it as exact as you > > > did.) > > > > > > However I think it would be a better solution to make the page_titles array not > > > constant and translate it right there. > > > > > > * It reduces complexity - if you use values of this array you don't expect > > > that you have to translate it and that leads to bugs as this one. > > > * If the array is used at several places we may save one or two gettext > > > lookups (ok, not really relevant - just mentioning) > > > > > > Because of the complexity of this matter I think an own commit with an > > > explanation would be good. > > > > > > Actually I'm in favor of reusing the page_titles array in the PageWidget > > > subclass, replacing the following code (line 32 ff.): > > > > > > switch (page) { > > > case PropertiesPage.GENERAL: > > > name = _("General"); > > > break; > > > > > > case PropertiesPage.SYSTEM: > > > name = _("System"); > > > break; > > > > > > case PropertiesPage.DEVICES: > > > name = _("Devices"); > > > break; > > > > > > case PropertiesPage.SNAPSHOTS: > > > name = _("Snapshots"); > > > break; > > > } > > > > > > by: > > > > > > if (page < 4 && page >= 0) > > > name = Properties.page_titles[page]; > > > > Actually in a patch coming soon to git master, I'm removing this array itself > > so your fix is only for 3.14 branch. So unless I'm wrong, I'd prefer we simply > > do what I suggested in previous comment of mine. > > > > Alternatively, you can leave it as is and I can backport the patch to 3.14 as > > well: > > > > index b20dc9b..f3bb2fb 100644 > > --- a/src/properties.vala > > +++ b/src/properties.vala > > @@ -11,8 +11,6 @@ > > } > > > > private class Boxes.Properties: Gtk.Notebook, Boxes.UI { > > - private const string[] page_titles = { N_("General"), N_("System"), > > N_("Devices"), N_("Snapshots") }; > > - > > public UIState previous_ui_state { get; protected set; } > > public UIState ui_state { get; protected set; } > > > > @@ -130,7 +128,7 @@ private void populate () { > > > > for (var i = 0; i < PropertiesPage.LAST; i++) { > > var page = new PageWidget (i, machine); > > - var label = new Gtk.Label (page_titles[i]); > > + var label = new Gtk.Label (page.name); > > That's good. My patch is not needed any more for the page titles. I don't think > 3.14 needs the change above. It seems to break a UI and string freeze, unless > I'm wrong. Oh sorry I missed that. No, it doesn't. These strings are visible in the UI and I just messed up by not properly marking them for translation so not very different from the other strings you are fixing from the POV of translators.
(In reply to comment #12) > Review of attachment 292063 [details] [review]: > > Patch itself is good but I think the description doesn't apply to this patch > (after the split). While you fix this, would be nice if you act on my nitpick > about shortlog in the previous review as well. :) Thank you for the review. But I'm sorry I don't get your point. Do you mean only the subject is enough and the description is unecessary for the patch? Or the description is not good, so I need to rewrite it?
(In reply to comment #16) > (In reply to comment #12) > > Review of attachment 292063 [details] [review] [details]: > > > > Patch itself is good but I think the description doesn't apply to this patch > > (after the split). While you fix this, would be nice if you act on my nitpick > > about shortlog in the previous review as well. :) > > Thank you for the review. But I'm sorry I don't get your point. Do you mean > only the subject is enough and the description is unecessary for the patch? Or > the description is not good, so I need to rewrite it? Sorry about all the confusion. I'll update the log before pushing so dont worry about it. Just include the changes suggested in comment#6.