[haiku-gsoc] Re: Localekit patch: various fixes and cleanups
- From: Oliver Tappe <zooey@xxxxxxxxxxxxxxx>
- To: haiku-gsoc@xxxxxxxxxxxxx
- Date: Thu, 16 Jul 2009 20:25:02 +0200
Hi Adrien,
On 2009-07-15 at 17:51:40 [+0200], PulkoMandy <pulkomandy@xxxxxxxxx> wrote:
> >> This patch removes the numeric hash from the plaintext catalog files,
> >> as it is not needed there. It also ensures that the plaintext and the
> >> linked catalogs have the same fingerprint. I also started to fix other
> >> bugs but they are not fully fixed yet :)
> >
> > Update: write the correct language in the generated catalogs. Added
> > support for -l argument in linkcatkeys and use it in the jamrules.
> > This did not prevent the catalog system to work, but now we have the
> > correct language in the catalog and it is also copied as an attribute.
> > This means that the catalog system is now fully working properly
> > (until someone finds a bug, of course).
>
> Update again:
> Gather all the strings for one application into the same catkey file.
> Also fix a memory allocation problem in the new function
> escapeQuotedString, it now works properly.
I've looked at the patch and have found the following issues/comments:
- There are now several warnings during the build, some because there are
still places (e.g. DefaultCatalog.cpp) where fingerprint is used as an int32
instead of uint32. Some other warnings (during generation of the build tools)
complain about deprecated headers being used. There should really be no
warnings during the build of our own sources (maybe except for some
unavoidable cases).
- The resulting catalogs on the image have none of the locale-attributes
set (IIRC because of limitations in libroot_build.so). While that can be
fixed later, I expected that once I started the Locale preflet (which opens
french.catalog), the attributes of french.catalog would all be set. Language
and signature are alright now, but the fingerprint is still empty.
- The locale kit tests no longer build, due to the fingerprint related
changes. I assume that you haven't noticed because you do not include them in
the build. Please change your UserBuildConfig such that these tests are
copied onto the image, which will trigger them being built, too.
- The new method 'void escapeQuotedString(BString stringToEscape)' in
PlainTextCatalog.cpp is a no-op, because it uses a by-value parameter. It
should be escapeQuotedString(BString& stringToEscape). Additionally, I'd
suggest using the BString-method CharacterEscape() instead of the three
invocations of ReplaceAll(). Strangely enough, the resulting english.catkeys
file still contains the characters in escaped form (and continues to do so
after the invocations to escapeQuotedString() has been removed). Something is
fishy, there ;-)
- Style hint: in PlainTextCatalog::WriteToFile(), the code fragment where
textContent is filled should be wrapped such that the '<<' operator is at the
beginning of a line, not at the end. I have just checked and this does not
seem to be mentioned in our styleguide, but nevertheless, I believe that's
how it is done in most of our code.
cheers,
Oliver
Other related posts:
