announcement: Kwave is in kdereview

[Thomas Eschenbacher]

David Faure wrote:
> [...]
>>> main.cpp shouldn't use I18N_NOOP anymore in KF5. You should create the
>>> QCoreApplication instance first, then create the KAboutData using i18n(),
>>> then create the QCommandLineParser, also using i18n()
> [...]

OK, that seems to be a misunderstanding. You only talked about main.cpp
only.
I changed it as you and Albert have suggested, hope it is ok now:

see git commit beec6558c8517443fe4df69a58bcba4daa4a73b8

> [...]
>> I remember that I
>> had spurious application crashes on startup that were caused by failed
>> translations 
> 
> More details please. And was that really when creating the app first?

AFAIR it had to do with statically instantiated objects that used i18n.
That has a very good chance to fail, as the constructors of these
objects get called before main(), maybe before setting up the i18n
subsystem. Examples were some maps that I used for translating strings
<-> enums, there I initialized the map content in the constructor. Like
in Kwave::FileInfo.

>>> Kwave::TopWidget::executeCommand: is the "if (false) {" around a lot of
>>> code intended? Looks weird.
>>
>> Yes, that's because a switch/case cannot be made on strings, so I wrote
>> a macro to make the "if (string==...) else ..." cascades look a bit more
>> readable (the "CASE_COMMAND" macro). I could write a macro to
>> encapsulate/hide this, maybe some SWITCH_COMMAND() macro, but I'm not
>> sure if it really makes things any better... Do you have a better idea?
> 
> I don't follow. How does the use of that macro mean that you need if (false) 
> in your code? This code is dead, right?

Normally you would write things like this:

if (command == "do_this") {
   do_this(params);
} else if (command == "do_that") {
   do_that();
} else if (commmand == "do_something_else") {
... and so on...

But I didn't like this, I wanted to have something that looks more like
a switch/case, so I wrote a macro that encapsulates the repeating line
with "} else if (command == ...) {". The bad side effect is that for
this to work you need a dummy "if (false) {" or similar at the start :-(

>>> Kwave::SwapFile should be ported from int to qint64 for all file sizes,
>>> no?
>>> (to go above the 4GB limit). All the INT_MAX stuff and the casting from
>>> qint64 to int should IMHO be removed; even if 4GB is a crazy size, it
>>> would still make the code nicer (no casts).
>>
>> No. The point is that there are still some 32bit systems around. 
> 
> So? Even on 32 bit systems the QFile and QFileInfo APIs uses qint64.
> 
>> The limitation is not so obvious, but at some place these sizes/numbers go
>> into GUI elements - and on 32bit systems these still are simple plain
>> "int" values, thus 32 bit. 
> 
> If these values come from QFile or QFileInfo, they should be qint64.
> On all systems.

Yes, that's all correct, QFileInfo and so on are already safe. But think
about classes like a QIntSpinBox, QSlider and so on. These still take an
"int" which is 32 bit only on a 32 bit system (see NewSignalDialog.cpp
for a usage example). I tried to make the application completely 64bit
aware some years ago, and AFAIR the only points where I still was not
able to get rid of this limitation were the few places where
sample_index_t hits some GUI elements (plus some file formats, like RIFF
for example).

>> Additionally the 4GB is still a limitation
>> for many file formats which cannot handle such huge files, so breaking
>> the 2GB or 4GB limit is currently not something really urgent, nobody
>> complained about that the past few years.
> 
> OK, valid point, but I would still make the code more robust and cast-clean.

^^^^^^^^^^^
I 100% agree, and I already spent a lot of effort in making it cleaner.
Using "clang" helped a lot in this!

>>> Kwave::Stripe::resize detaches without the mutex locked, while other
>>> methods in the same file do it with the mutex locked.
>>
>> OK, that one needs some more time to check, that's not as trivial as it
>> might look now. AFAIR it had have side effects / deadlocks in this area
>> and changed the locked ranges for good reason.
> 
> That doesn't make me very confident about the code in question...

Just let me check this carefully! I will not change things in one of the
base components (which were running without any problems for many many
years) without checking for side effects. That should also be in your
interest (as user).

> [...]
> OK, but surely signal(SIGHUP, old_handler) affects all threads, doesn't it?
> How could this possibly affect just one thread?
> If it does affect all threads, then you have a clear race there (described 
> above).  The code is "stable" because the killing with a signal is probably 
> only rarely used, I suppose. It's still buggy ;)

Hmm... yes, I see... That could not work reliably.
Should be better now:
git commit 01d0d05d07c274d6045b764fb77b9bafd643da57

kind regards,
   Thomas