Created attachment 56142 [details] [review]
patch for this bug: do necessary encoding conversion

This patch convert filename encoding from utf8 to glib when set background image file. And the internal filename encoding is utf8.

Created attachment 56429 [details] [review]
new patch for this bug, don't use previous patch.

The first patch can't work when set utf8 file on non-utf8 locale, and it is do too many unnecessary work.

The new patch only do conversion when read from or write to gconf database.

yydzero, your patch does not really work: 

*) that a string passes the g_utf8_validate test does not mean it is unicode. Moreover, you cannot pass NULL to gconf_client_set_string, as it emits a critical warning if you do.

*) the correct translation for filenames to utf8 and back is done using g_filename_to_utf8 and g_filename_from_utf8 (see <http://developer.gnome.org/doc/API/2.0/glib/glib-Character-Set-Conversion.html#g-filename-to-utf8>)


Independently of that: I think that one should store filenames in gconf using the G_FILENAME_ENCODING: otherwise, one needs to store the G_FILENAME_ENCODING too, as this may change from run to run. *But* gconf_client_set_string calls gcong_engine_set_string which demands that the string pass g_utf8_validate.

Maybe hp thought about this at gconf-api-thinking time? 

(In reply to comment #4)
> yydzero, your patch does not really work: 
> 
> *) that a string passes the g_utf8_validate test does not mean it is unicode.
> Moreover, you cannot pass NULL to gconf_client_set_string, as it emits a
> critical warning if you do.
Yes, non utf8 string may pass g_utf8_validate, but, as you know, it won't complain when pass this string to gconf_engine_set_string. and it will be transparent to the user, gnome-terminal just work fine even though it is not utf8 encoded string.

if filename is NULL, then g_utf8_validate will crash, so I add this condition here. of couse we can first validate filename is not utf8 then call g_utf8_validate.

> 
> *) the correct translation for filenames to utf8 and back is done using
> g_filename_to_utf8 and g_filename_from_utf8 (see
> <http://developer.gnome.org/doc/API/2.0/glib/glib-Character-Set-Conversion.html#g-filename-to-utf8>)
> 
oops, you are right, we need use g_filename_to/from_utf8 instead of g_locale_to/from_utf8.

> 
> Independently of that: I think that one should store filenames in gconf using
> the G_FILENAME_ENCODING: otherwise, one needs to store the G_FILENAME_ENCODING
> too, as this may change from run to run. *But* gconf_client_set_string calls
> gcong_engine_set_string which demands that the string pass g_utf8_validate.
> 
> Maybe hp thought about this at gconf-api-thinking time? 
If we store G_FILENAME_ENCODING filename into gconf, then we will got garbage when display them using tools such as gconf-editor. I guess it is the reason gconf store only utf8 string.
> 

Created attachment 71815 [details] [review]
convert filename between G_FILENAME_ENCODING and UTF-8

use g_filename_to/from_utf8 instead of g_locale_to/from_utf8

terminal_profile_set_background_image_file is called from two places: (i) profile-editor.c:background_image_changed, where the filename is gotten out of gtk_file_chooser_get_filename, and so is in the G_FILENAME_ENCODING, and (ii) terminal-screen.c:drag_data_received, where the filename is gotten out of g_filename_from_uri, which is then also in the G_FILENAME_ENCODING.

In any case, the encoding of the filename gotten by terminal_profile_set_background_image is the G_FILENAME one. Thus, your patch should convert from the filename econding to UTF-8 in any case. 

Indeed, the simplest thing should be to convert from/to the G_FILENAME_ENCODING whenever the string comes out of/goes into gconf.

On the other hand, I think it would be much better to replace your "g_return_if_fail (filename != NULL);" by a special case: if terminal_profile_set_background_image_file gets passed a NULL, make it set the gconf key to "", the empty string.

Created attachment 71880 [details]
GtkFileChooser display both UTF-8 and G_FILENAME_ENCODING filename on non-utf8 locale

(In reply to comment #7)
> terminal_profile_set_background_image_file is called from two places: (i)
> profile-editor.c:background_image_changed, where the filename is gotten out of
> gtk_file_chooser_get_filename, and so is in the G_FILENAME_ENCODING, and (ii)
> terminal-screen.c:drag_data_received, where the filename is gotten out of
> g_filename_from_uri, which is then also in the G_FILENAME_ENCODING.
> 
> In any case, the encoding of the filename gotten by
> terminal_profile_set_background_image is the G_FILENAME one. Thus, your patch
> should convert from the filename econding to UTF-8 in any case. 
Oh, I got your ideas, you are wondering why i use g_file_test to test whether it is
exist before conversion. This is because GtkFileChooser can display both UTF-8 filename and current locale filename on non-utf8 locale, eg: on zh_CN.GB18030 locale, GtkFileChooser can display both GB18030 encoded filename and UTF-8 encoded filename. When user choose utf-8 filename from file chooser, then the encoding of filename is UTF-8 instead of GB18030, so I add g_utf8_validate before convert to UTF-8 also. And use g_file_test to test whether the utf8 file exist or not before convert UTF-8 to G_FILENAME_ENCODING.

I attached one example.
> 
> Indeed, the simplest thing should be to convert from/to the G_FILENAME_ENCODING
> whenever the string comes out of/goes into gconf.
> 
> On the other hand, I think it would be much better to replace your
> "g_return_if_fail (filename != NULL);" by a special case: if
> terminal_profile_set_background_image_file gets passed a NULL, make it set the
> gconf key to "", the empty string.
If we pass "", then the original background image will lost and gnome-terminal has no background image anymore. If we return when filename == NULL, then gnome-terminal still use old background image.

which one you prefer?
> 

(In reply to comment #9)
> (In reply to comment #7)
> > On the other hand, I think it would be much better to replace your
> > "g_return_if_fail (filename != NULL);" by a special case: if
> > terminal_profile_set_background_image_file gets passed a NULL, make it set the
> > gconf key to "", the empty string.
> If we pass "", then the original background image will lost and gnome-terminal
> has no background image anymore. If we return when filename == NULL, then
> gnome-terminal still use old background image.
> 
> which one you prefer?

The reason we are getting lots of NULLs here is because we are listening to the wrong signal: selection-changed is not correct. Since my patch on bug 353196 seems to have been accepted, we'll be able to listen to file-set soon, and then when we get a NULL from the filechooser it will be because the user decided to.

In any case, it's Guilherme's call.

I'll think about the rest of your comment.

This problem has been fixed in the development version. The fix will be available in the next major software release. Thank you for your bug report.