GNOME Bugzilla – Bug 704592
Better handling of self-localized translation language list
Last modified: 2013-07-28 14:39:46 UTC
Created attachment 249674 [details] [review] Only load the language list at startup and simplify the code. After looking a little more at the language list for Bug 704510, I saw various issues with the current implementation: 1/ It reloads and fully parse the huge XML list of ISO-639 languages (on my system for instance, this XML file is 2175 lines) each time it constructs a GimpLanguageStore, which means in other words each time the user opens the preferences dialog. That's a waste of resources (actually I can feel the slight delay on my computer when I open the preferences, which disappeared with my new implementation); 2/ Because of 1/, we have to also localize each language name in itself each time the user opens the preferences as well, which is again a waste of resources, but even more can be a security risk. As pointed by glib docs: "Environment variable handling in UNIX is not thread-safe, and your program may crash if one thread calls g_setenv() while another thread is calling getenv(). (And note that many functions, such as gettext(), call getenv() internally.)" Well we have a lot of calls to gettext or other, and I think we want to be thread-safe. ;-) In any case, why take the risk? 3/ The current implementation is kind of doing a lot more of useless work. For instance as I said, it would first parse and save the whole list of languages, then only we check if we have a gettext translation for this language. As there are hundreds of languages but only a few dozen of available translations, we want to do the opposite: first check which language we have translations for, then check their name in the file. 4/ There was this class GimpTranslationStore which was only some kind of useless buffer after GimpLanguageStore. It has absolutely no interest (or I did not get it), and just does some convoluted processing for no reason (I had to concentrate hard to understand what was going on between GimpLanguagestore and GimpTranslationStore (basically save all the languages first in a hash table, then later save them in the GtkListStore, doing all twice). I propose to get rid of this intermediate useless class, thus simplify the code. The attached patch implements all this, making in my opinion a safer and cleaner code for managing this list.
Won't be able to look closer until back from vacation, but maybe it was for providing either untranslated or translated language names (depending on which you used, language or translation store). Didn't we want the language selection in tool options be untranslated?
Hi, Right. Let me have a look at this again.
Created attachment 250272 [details] [review] Only load the language list at startup and simplify the code. Mitch, attached a new version of the patch. I got back the GimpTranslationStore from the dead but cleaned out a lot of redundant code. I believe this is kind of an important patch because setenv/getenv are not thread-safe. And even though I had no problem because of this since we made this self-localized language feature, let's not take a chance of problems and crashes in the future or in some particular platforms. Also it avoids parsing the ISO-639 language list each time the user opens the preference or the text tool. :-)
I don't think I understand this patch any longer, go ahead and push if you think it's the right thing to do :)
Ok. Done. :-) commit a129f84c68568cc96c3f06a40e8bab6fba035a01 Author: Jehan <jehan@girinstud.io> Date: Sat Jul 20 14:51:13 2013 +0900 Bug 704592 - only load language lists once at gui startup. Improvements: - setenv/getenv() are not thread-safe, hence they should be run only at startup before any threading occurs. - it is counter-productive to load the huge ISO-639 XML file each time the user opens the Preferences dialog or the text tool options.
Now again crashes on OS X when opening preferences dialog (see also bug #704510, comments #4ff): $ gimp-git.sh quartz --verbose --version GNU Image Manipulation Program version 2.9.1 git-describe: soc-2012-unified-transform-after-gsoc-1145-ga129f84 using GEGL version 0.3.0 (compiled against version 0.3.0) using GLib version 2.36.3 (compiled against version 2.36.3) using GdkPixbuf version 2.28.2 (compiled against version 2.28.2) using GTK+ version 2.24.20 (compiled against version 2.24.20) using Pango version 1.34.1 (compiled against version 1.34.1) using Fontconfig version 2.10.93 (compiled against version 2.10.93) using Cairo version 1.12.14 (compiled against version 1.12.14) $ $ gimp-git.sh quartz This is a development version of GIMP. Debug messages may appear here. /Volumes/magenta/mp-trunk/quartz/../src/gimp/local/bin/gimp-2.9: fatal error: Segmentation fault: 11 /Volumes/magenta/mp-trunk/quartz/../src/gimp/local/bin/gimp-2.9 (pid:89695): [E]xit, [H]alt, show [S]tack trace or [P]roceed: s
+ Trace 232305
(script-fu:89696): LibGimpBase-WARNING **: script-fu: gimp_wire_read(): error $
As discussed on the mailing list, normally all fixed. :-) Tell me if there is any more issue. commit f0b3c76c9d1b8a2e1ae8960fad108d5f63687700 Author: Jehan <jehan@girinstud.io> Date: Mon Jul 29 02:12:17 2013 +1200 app - fix crash of the language parser on OSX. Stupid bug. I misused the GINT_TO_POINTER macro.
Just to confirm: on OS X 10.7.5 with dependencies installed via MacPorts, f0b3c76 launches ok and does not crash when opening preferences. thx :)