Re: gettext patch (beta)

[phcoder]

Hello, thanks for your work. It's a nice stuff, however it has some 
minor problems
Carles Pina i Estany wrote:
> > -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
> > CALL FOR HELP:
> > I need to write the Makefile.in (see po/TODO :-( ). I'm not used or
> > familiar to write Makefiles :-( if someone wants to help it would 
> > speed up the process quite much. It needs only to merge the files with
> > the new .pot, compile (msgfmt), and install to the correct directory.
Use common.rmk, don't write directly to Makefile.
> > I exactly know what has to do, so if someone knows about
> > installation/Makefiles and doesn't know about gettext it's not a
> > problem, contact me. Else I will try to implement soon.
I can help you, I'm not a makefile expert but can be useful
> > I would even invite to a couple of beers in Fosdem if someone does
> > this part :-)
> >
> > TODO:
> > -the Makefile.in
> > -and more testing about 00_header with gettext detection.
> > -Add _("") for mainly all strings (I would do in a separate patch)
> > -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
> > Index: conf/common.mk
> > ===================================================================
> > --- conf/common.mk      (revision 1952)
> > +++ conf/common.mk      (working copy)
Don't include auto-generated files in your patch
> > 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
static grub_uint32_t
grub_gettext_get_info (int offset)
{
  grub_uint32_t buf;

  grub_file_seek (fd_mo, offset);
  grub_file_read (fd_mo, (char*) &buf, sizeof (buf));
  buf = grub_cpu_to_le32 (buf);
  return buf;
}
Same applies multiple times in different places.
grub_gettext_translation_number is a bit a misnomer because this name 
would suggest transforming translation into number
> > +  offsettranslation = grub_gettext_get_info (GETTEXT_OFFSET_TRANSLATION);
> > +
> > +  position=offsettranslation+i*8;
Please respect GCS. This should be position = offsettranslation + i * 8;
> > +  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
> > +  if (magic != 0x950412de)
A define instead of hardcoded number is suggested
> > +  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
> > +  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


--

Regards
Vladimir 'phcoder' Serbinenko