Re: [Libexif-devel] Changes to libexif-gtk for migration to gtk3

[Valere M. <val...@ym...>]

Thanks for the review, Dan.


The new updated patch (updates1.diff) has been uploaded at the same place
than others i.e. http://sourceforge.net/projects/vmonseur.u/files/libexif-gtk/


Please, find below my comments on the review.

Cheers,
Valere

> -DLOCALEDIR is already set in configure.ac by GP_GETTEXT_HACK, isn't it?

Indeed, I'm not yet used to the GP_xxx macros ;)
I've removed -DLOCALEDIR from Makefiles.

> -       -DG_LOG_DOMAIN=\"libexif\"
> +       -DG_LOG_DOMAIN=\"libexif-gtk\"  \
> 
> Won't this break backward compatibility?  Or is this just for debugging?
> Could one expect changing it to break any software?

It could be used for other things than debugging.
I've reset G_LOG_DOMAIN to 'libexif'.

> diff -aurpN libexif-gtk-0.3.5-cvs/cvs/libexif-gtk/tests/test-libexif-gtk.c libexif-gtk-0.3.5-diff1/cvs/libexif-gtk/tests/test-libexif-gtk.c
> +       bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8");
> 
> Does GTK not operate in the current locale's character set? Is it fixed to
> use UTF-8 no matter what the environment?  If so, then this should really
> be called everywhere that bindtextdomain(GETTEXT_PACKAGE, LOCALEDIR)
> is called from within libexif-gtk. Apps won't know this textual domain name,
> anyway, since it's really an internal detail to libexif-gtk.

As per the gtk documentation (http://developer.gnome.org/gtk/2.24/gtk-question-index.html):
bindtextdomain has to be called with UTF8.
So, indeed bind_textdomain_codeset has to be called everywhere that
bindtextdomain is called from within libexif-gtk.
Change is done.

> -       gtk_set_locale ();
> -       textdomain (PACKAGE);
> +       bind_textdomain_codeset (GETTEXT_PACKAGE, "UTF-8");
> +       textdomain (GETTEXT_PACKAGE);
> 
> Since test-libexif-gtk.c itself doesn't use gettext, I wouldn't expect it to
> be calling textdomain().  In fact, it seems like a better test to NOT
> call it, so you can better ensure that the libexif-gtk library is doing
> the right thing with gettext on its own. In addition, the whole #ifdef
> ENABLE_NLS block at the head of that file should go.

Yep, I agree. 
Change is done.

> However, I believe you'll need a call to setlocale(LC_ALL,"") to set
> the locale. I'm guessing that's what gtk_set_locale() did, so that
> part should probably stay. I think this diff chunk should probably look
> like this:
> 
>         gtk_set_locale ();
> -       textdomain (PACKAGE);

As per the gtk documentation (http://developer.gnome.org/gtk/2.24/gtk-General.html):
locale is set by gtk_init, which is a mandatory call for every gtk program.
So there is no point in calling gtk_set_locale or set_locale.