Same for the GThemedIcon messages, btw.

Created attachment 257106 [details] [review]
Removed gettext markers for several messages

Review of attachment 257106 [details] [review]:

So...this is a messy subject.  There is a general rule though:

* Issues that only the software developer can/should fix -> g_return_if_fail() / g_assert()
* Issues that software user may see in deployment: GError

In some cases like the GIcon ones, we should probably change them to just g_return_if_fail() where possible.

Created attachment 257580 [details] [review]
Replace GError messages with g_return_if_fail for dev-only messages

Review of attachment 257580 [details] [review]:

This must spew a lot of compiler warnings - you probably want g_return_val_if_fail (assertion, NULL) since these functions return a pointer.

Created attachment 257585 [details] [review]
Replace GError messages with g_return_val_if_fail for dev-only messages

Review of attachment 257585 [details] [review]:

There is one issue but I'm not going to care too much about it - we leak the class reference when a precondition is triggered.  But really that's fine.  If someone cares they can fix it later.

https://git.gnome.org/browse/glib/commit/?id=00f0795a84d23f2e2654a86f8bd3a233c8af3771

Note that I changed the bug reference to be a URL per our standard https://wiki.gnome.org/GnomeLove/SubmittingPatches

This is definitely not correct.

Correct usage of this API would look like so:

GError *error = NULL;
GIcon *icon;

icon = g_icon_new_for_string (string_from_another_place, &error);
if (!icon)
  g_print ("error: %s\n", error->message);

Now icon will be NULL and error will be unset still.

A version code mismatch in icon deserialisation should _not_ crash my program.

There is a debate to be had over how meaningful-to-normal-users GError messages should be.  Some would argue that you're better off not showing these messages at all.  What is not up for debate: GError is totally appropriate for handling recoverable errors that are not the fault of the programmer.

Created attachment 257603 [details] [review]
Revert "Use g_return_val_if_fail() for developer-only messages"

This reverts commit 00f0795a84d23f2e2654a86f8bd3a233c8af3771.

Created attachment 257604 [details] [review]
gicon: Avoid translating detailed parse error messages

Only programmers really should care about the detail, so as a
compromise we'll translate the outer message only and avoid burdening
translators with internal technical goop.

I wonder if it's really worth it to change this.  These messages are ages-old so most translations have probably already done this message already anyway...

(In reply to comment #13)
> I wonder if it's really worth it to change this.  These messages are ages-old
> so most translations have probably already done this message already anyway...

I think it's worth dropping translations for stuff like this at least to set a precedent/model for future code.

Comment on attachment 257603 [details] [review]
Revert "Use g_return_val_if_fail() for developer-only messages"

Attachment 257603 [details] pushed as ed6ca39 - Revert "Use g_return_val_if_fail() for developer-only messages"

Found this when digging around today: https://bugzilla.gnome.org/show_bug.cgi?id=57691#c6

(In reply to comment #16)
> Found this when digging around today:
> https://bugzilla.gnome.org/show_bug.cgi?id=57691#c6

This is a complex topic, but let's assume 3 actors:

* User
  - Interacts with the GUI
  - Should always see translated messages in the GUI
* Sysadmin
  - Reads ~/.xsession-errors or the systemd journal
  - May or may not see translated messages
* App developer
  - Translated messages are highly likely to be irritating since they will need
    to find the relevant place in the source which is in English


Now, for the code you're talking about in giowin32.c, from my reading, those errors can only occur when the *app developer* screws up.  

In contrast, something like "Out of disk space" is something that should *clearly* be translated since it's an event on which both users and sysadmins can take action.

Going back to these specific icon messages; from what I can tell, they can *only* occur if application developers screw up.  That argues for their non-translation, as the i18n team requested.

(In reply to comment #17)
> Going back to these specific icon messages; from what I can tell, they can
> *only* occur if application developers screw up.  That argues for their
> non-translation, as the i18n team requested.

This is not exactly what's happening in this case, depending on your definition of "developer".  This is just a simple case of interface miss-match and could be caused by mixing GLib versions between two programs or two hosts, even.

This all goes to the core issue of "should I show GError to endusers?"  For something like "out of space", sure, but maybe not for some techno-mumble-jumble type messages like (most of) the ones above.  Or maybe we should.  Not all of our users are stupid.  Also, the ability to ask a friend "what does this mean?" is pretty nice.  Another nice usecase for error messages is typing them into google (which would probably work better untranslated, of course).

The problem is how to tell the difference.  g_error_set_contains_mumble_jumble()?  A high-order bit in the error code field for "this is average-user-meaningless"?

I insist that we do not stop using GError for cases like this.  I'm also fairly sure that we should always try to avoid showing English strings when running in a different locale, if at all possible.  What we may need is a way to prevent certain strings from being shown to the user in a GUI (but possibly OK at commandline or in logs).  I'd be happy to have those messages untranslated, then.

I think the i18n team needs to hash this out with the designers or something.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/191.