Created attachment 92467 [details] [review]
Build against latest SVN head

>#: ../gui/gdmsetup.glade.h:200
>"_Path:       "
>is the whitespace needed here or could this be fixed by some glade magic?

There is no easy fix for this particular entry. The spaces are used to align the text entry for the Shutdown/Halt/Suspend commands with those of custom Commands (i.e. when you toggle the command selection box the size and the position of text entries stays more less the same). You should be able to just keep the number of spaces as it is and change the text - but some visual viewing is required to confirm that it is the case


>: ../daemon/gdm.c:633
>"command failed %s: %d"
>what is %s and what is %d here? please add a translator comment:

I have added the required comment, but %s and %d are non-translatable (they define the command string and the error code in case the command does not exist)

>property name="label" translatable="yes">    </property>
>please do not mark this for translation.

I have removed (hopefully) all translatable=yes options on all the padding labels

>property name="label" translatable="yes">LRla_bel:</property>
>what does this mean and how can this be translated properly? please add a
>translator comment:

LRLabel defines the label that will appear on the List and Radio button widgets (as opposed to the Label which appears on the buttons)

I applied the attached patch.

Lukasz, for issue #2 it might be better to change it like this:

_("command failed") " " "%s: %d"

so it appends the strings together and only the english part is marked as translatable.  This avoids the need to comment.  Could you provide an updated patch that works like this?  I won't be able to apply it until 2.21 since we are passed string freeze, but we should do this for 2.21.  I'll leave this bug open until you provide this change, okay?

The LRLabel is not something that should be marked as translatable since it is a hardcoded string that you need to use for all languages, right?

thanks guys!

#: ../gui/gdmsetup.glade.h:81
<property name="label" translatable="yes">LRla_bel:</property>
is still missing a comment (i wonder if anybody understands this even in the original en_US user interface)... glade supports comments by adding a
      comments="foo"
attribute to an object, for example
http://svn.gnome.org/viewcvs/evolution/trunk/calendar/gui/dialogs/recurrence-page.glade?r1=26468&r2=31488

Created attachment 92738 [details] [review]
Build against todays svn head

It seems like gdm_error does not like the string formatting (maybe due to its variadic nature):
error: expected ‘)’ before string constant

I could create couple of string variables and concatenate them together if you want me to but i think it would be a bit of an overkill for what it is

>The LRLabel is not something that should be marked as translatable since it is
>a hardcoded string that you need to use for all languages, right?

It can be translated as it defines text that appears on lists and radio buttons (More less as Label is used for buttons). If you translating the Label label you might as well translate LRLabel label (although it might be difficult). This strings are only used in gdmsetup and do not have any other effect on the underlying code

Andre i have added the necessary comment (please let me know if its clear enough)

I agree we can leave LRLabel for translation.

Committed.  Thanks.  I think this resolves the issue.  Closing this bug.  Please reopen if you find further issues relating to this, or open a new bug if unrelated translation issues.

Note that even though gdm_error doesn't like setting the string that way, you could still build such a string via g_strdup_printf, then use the string, then free it.  While allocating and freeing memory is a little slower, if it makes translating the string easier for the l10n people, that's a more important thing, I think.  They shouldn't need to worry about the "%foo" stuff.

I don't think that's really a good idea.   The problem is you don't really know where in the sentence the %s should go for an arbitrary language, if colon means the same thing in that language, (or if another character is used instead of colon), etc.

Another problem you run it is that in some languages words surrounding a %s sometimes need to change depending on what the %s expands to.  I think that's a lot harder problem to solve generally though.

Then again, the string could probably be better anyway.  What is an example of it expanded out?

thanks for the quick fixing!

Created attachment 93079 [details] [review]
Built against few days old svn head

Attached is the g_strdup_printf workaround.
Ray the error string is as follows:
Command failed %s: %d, where first %s is used for the actual command path (i.e. /sbin/shutdown) and tha %d represents the status from g_spawn_command_line_sync

I think in this case, the message is better and easier to translate.  I don't think this is a case where translators would want to reorganize the %s or %d values.

Dealing with this is probably being a bit too picky, but I think it's good for Lukasz to get a better understanding of how strings should be managed for translation.  We've had a lot of issues with translation problems in the gdmconfig changes Lukasz has been working on, so I think that Lukasz is becoming an expert on these issues by digging into these problems.  :)

I'm going to reopen this bug so I can make this change in 2.21 after the string freeze.

Okay, so the exit status of g_spawn_command_line_sync should never go directly to a string anywhere, because it's actually several values encoded into one number (the high bits say how the process terminated generally, the low bits give more specific information).  You really want something like

g_assert (WIFEXITED (status) || WIFSIGNALED (status));
if (WIFEXITED (status))		
     gdm_error (_("command '%s' exited with status %u"), array[i], WEXITSTATUS (status));
else 
     gdm_error (_("command '%s' was killed by signal '%s'"), array[i], g_strsignal (WTERMSIG (status));

Sorry Ray, when i meant status returned from g_spawn_command_line_sync i meant the   value of WEXITSTATUS (status). However the second part was not covered (WIFSIGNALED). try_commands calls try_command repeatedly - which also does error parsing - so maybe all the error processing should be moved there. Also do we really want these strings translatable? 

Created attachment 93142 [details] [review]
Build against few days old SVN head

I have changes the way things work a little bit (and "untranslated" the error message). What do you think?

Didn't you lose the g_strdup stuff from comment #9?  

Also, note that since you changed the strings this can't go in until 2.21 still.

If we want the better error handling to go into 2.19 then we can't change the string messages. 

Sorry Brian i have removed g_strdup stuff as im not sure if average user will benefit from knowing with which exit status the command exited - this will probably be more useful to people that will be fixing errors so i made them non-translatable. Also the function that is called repeatedly does not have translatable strings. Does that sound reasonable? Im not sure if we can do much to patch in comment #9 without moving/changing the code/strings too much

Thanks for clarifying.  I think this patch is appropriate to go into 2.19, then.  
Removing a translation string isn't a problem - adding or changing them is in terms of the freeze.

I also agree that for syslog debug messages that translation isn't so important.

Therefore I'm committing this patch upstream.

Lukasz, your patch broken the build.  It seems to be complaining that try_command only has one argument but places call it with two, and with the status variable.

Created attachment 93169 [details] [review]
Build against todays svn head

Sorry Brian my bad forgot to clean the function call. Attached fix (i have tested it and it compiles)

Thanks, works much better.  Committed and closing this bug.