[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAHNEV5QmxdSWuxT+XQcMgYiFA=j-4OBaOCXywxPN3kAYa8eUZQ@mail.gmail.com>
Date: Fri, 17 Apr 2015 09:30:20 -0400
From: Mathieu Laprise <mathlaprise@...il.com>
To: john-dev@...ts.openwall.com
Subject: Re: Improving Johnny
On Fri, Apr 17, 2015 at 7:50 AM, Aleksey Cherepanov <lyosha@...nwall.com>
wrote:
> Mathieu,
>
> On Tue, Apr 14, 2015 at 02:40:38PM -0400, Mathieu Laprise wrote:
> > First, I reviewed the code to see if the string were marked as
> > translatable. Almost all of them were. I found a problematic case in
> > MainWindow::showJohnError. There were two functions in #define and their
> > strings didn't register as translatable in the .ts file.
>
> I think you talk about
>
> #define C(code, text) case QProcess :: code: message = tr(text); break;
> #define P(code) C(code, "Problem with john: " # code);
> switch (error) {
> C(FailedToStart,
> "John failed to start. "
> "Check your Path to John setting. "
> "Check permissions on respective file.");
> C(Crashed, "John crashed.");
> P(Timedout);
> P(WriteError);
> P(ReadError);
> P(UnknownError);
> default:
> message = tr("There is a problem. Johnny could not handle it.");
> }
> #undef P
> #undef C
>
> C and P are not functions, they are macros. So they are expanded
> before compilation and tr() is called for each string. "their strings
> didn't register as translatable in the .ts file" - is it your
> assumption? Did you try it? Or are they not registered as translatable
> in some qt's tool? Please give more details, I'm curious.
>
> I know that your macros are writing something like this for each case
before the program is compiled:
case QProcess::Timedout:
message = tr("Problem with john Timedout");
break;
I agree that it's weird because it'll, at some point, become the same as
writing it directly in the .cpp. However, it's not an assumption,I tried it
and it didn't work with translation. All strings except those in this
function were in the .ts file which is generated by calling lupdate. Even
the one that were not related to enum like "John failed to start, check
your path etc.." didn't. Maybe it ignores macros, I don't know, I didn't
try to fix the macros as the code needed a refactoring anyway.
> Also you've changed the behaviour:
>
> + message = tr("Problem with john #") + QString::number(error);
>
> So johnny prints error's number while the code with macros showed
> error's name. Do you understand why? Please explain.
>
> I'd say it is a regression. Though the piece of code is
> overcomplicated and with macros. Macros are a bad style for C++. So
> I think the regression is ok as an intermediate step.
>
> Thanks
>
Yeah, sorry I misinterpreted the two macros. Regarding why it's doing that,
it's because it's writing the code that I wrote previously in this message.
For such a simple function that write 6 error messages, if we want to keep
good readability of the code and 100% translatable error messages, let's
just separate the switch case with 4
cases(Timedout,WriteError,ReadError,UnknownError) into different
cases/error messages.
> --
> Regards,
> Aleksey Cherepanov
>
Content of type "text/html" skipped
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
