[haiku-development] Re: Patch reviews - help needed
- From: Stephan Assmus <superstippi@xxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Thu, 06 May 2010 16:08:38 +0200
On 2010-05-06 at 15:44:25 [+0200], Rene Gollent <anevilyak@xxxxxxxxx> wrote:
> On Thu, May 6, 2010 at 9:40 AM, Stephan Assmus <superstippi@xxxxxx> wrote:
> > At the moment, I have this personal problem that I feel more help with
> > this
> > would be most welcome. It's of course useful to discuss possible
> > improvements
> > to the workflow, but the fact remains that we cannot let so many patches
> > rot
> > in Trac. The less ideal the workflow the more reason to sharing the load.
>
> In my particular case the biggest problem is the lion's share of the
> patches lately have all been localization-related, and thus far I've
> yet to really have time to properly familiarize myself with the Locale
> Kit, so I'm completely unqualified to review those at the moment. If a
> patch is in an area I'm comfortable with, I'll try to check it, but
> there simply haven't been very many that fit in that category in
> recent times.
With the recent patches, there is basically these types of problems:
* Strings are translated that should not be. For example names of BViews.
Names of BViews might be used in FindView() and friends, which breaks stuff.
Also see the recent commit to revert parts of localization for Mail, which
localized add-on names and broke stuff. Another questionable translation is
debug messages which are only supposed to be seen by developers. I am not
even sure error messages that go to stderr are supposed to be translated,
but I can see some reason behind that.
* The application needs to supply storage for a BCatalog object. I think
it's broken design, but for the moment I don't know a good fix. In any case,
you should see "BCatalog fAppCatalog" added as member to the BApplication
class, and then a line similar to "be_locale->GetAppCatalog(&fAppCatalog);"
needs to appear in the code before any localized strings are used.
* The Jamfile should contain the DoCatalogs rule. Just look at a localized
app for how this has to look. The rule needs to mention all files with TR()
macros.
* The patch should ideally remove string composition:
BString errorString;
errorString << "The file '";
errorString fileName;
errorString << "' cannot be found.";
should become:
char buffer[512];
snprintf(buffer, sizeof(buffer), TR("The file '%s' cannot be "
"found."), fileName);
As you can see, it's really not too bad to check for these problems. It's
also ok to ask for improvements of the patch rather than doing them
yourself, and usually they will be done promptly.
Best regards,
-Stephan
Other related posts:
