Re: Fw: gettext support

[Carles Pina i Estany]

Hello,

On Jun/17/2009, Vladimir 'phcoder' Serbinenko wrote:
> Hello
> 
> 
> > Comments for next steps to commit the patch are welcomed.
> 
> Quick look revealed that you haven't taken into account my comments exposed
> here:  http://lists.gnu.org/archive/html/grub-devel/2009-04/msg00181.html

I will copy-paste your comments and my reply with the attached patch.
I will comment only the concerns about things that are not about the
building system (this will come in another mail)

http://lists.gnu.org/archive/html/grub-devel/2009-04/msg00181.html

(double quote is me, single quote is vladimir)

> >         -Copy ca.mo to /usr/share/locale/ca/LC_MESSAGES/grub.mo 
> 
> Languages files should go to a subdir of $PREFIX. E.g. to
> $PREFIX/langs/$LANG.mo linux directories may be inaccessible 

Now the gettext module will search in $prefix/locale/lang.mo, where lang
is the variable that the user will setup in grub.cfg (e.g. ca for
catalan) and $prefix is usually /boot/grub

> > -I have seen that Grub2 is not printing correctly the accents,
> > could be a problem in gettext or in some other layer
> 
> Did you load unifont as your font? Are you in gfxterm mode? Plain pc
> console can't output unicode characters because it uses fixed-width
> 8-bit font. Perhaps loading the characters most useful for current
> languages to the upper 128 characters would be an option. OR we can just
> tell everyone to use gfxterm 

It's working fine, it was a problem generating the .mo file (I didnt'
setup the correct charset)

> > Index: gettext/gettext.c
> > ===================================================================
> > --- gettext/gettext.c   (revision 0)
> > +++ gettext/gettext.c   (revision 0)
> > +static int
> > +grub_gettext_get_info (int offset)
> > +{
> > +  int buf;

> Use grub_uint32_t here. Also be aware of endianness. It should be

Done!

> >grub_gettext_translation_number is a bit a misnomer because this name
> > would suggest transforming translation into number 

change from grub_gettext_translation_number (int i) to
grub_gettext_translation_from_position (int position) 
Don't confuse the string in position "N" with the position (in bytes) of
the string inside the file (called internal_position). I'm still not
happy with the names...

> > +  position=offsettranslation+i*8;
> Please respect GCS. This should be position = offsettranslation + i *
> 8; 

Done!

> > +  ret = grub_malloc(grub_strlen(orig) + 1);
> > +  grub_strcpy(ret,orig);
> > +  return ret;
> This would fail if the string isn't present at all in .mo 

I think that it's fine. If the string is in the .mo file, it will return
before:
      else if (grub_strcmp (current_string, orig) == 0)
        {
          grub_free(current_string);
          return grub_gettext_gettranslation_from_position (current);
        }

If we don't find the string, it will return the string that we wanted to
translate (orig is the original string).
So if grub tries to translate something that it's not in the .mo file we
will return the same string.

> >         + if (magic != 0x950412de) 
> A define instead of hardcoded number is suggested 

Done!

> >         + locale_prefix = grub_env_get ("locale_prefix"); 
> You need to treat the case when no locale_prefix is defined. I suggest
> to put a default $prefix/locale 

Done!

> >         +  grub_sprintf (mo_file, "%s/%s/LC_MESSAGES/grub.mo",
> > locale_prefix, lang);
> >         +  /* XXX: lang is written by the user, need to sanitaze the
> > input?  */

> I suggest
> grub_sprintf (mo_file, "%s/%s.mo", locale_prefix, lang);
> because .mo need to reside together with grub so all LC_MESSAGE is just
> unnecessary

Your way is easier. Previous way was talked with Robert, but then it
still needed some work in grub.cfg to detect the directory and in my
opinion is quite clear to have all grub stuff all together.

I send attached the patch. I will comment things that executes outside
grub environment (so, during the installation) in some other mail.

Thanks for all feedback and sorry to take so long to answer :-)

-- 
Carles Pina i Estany
        http://pinux.info

gettext11.patch
Description: Text Data