announcement: Kwave is in kdereview

[Thomas Eschenbacher]

David Faure wrote:
> [...]
> As a user, I definitely vote for kwave going to kdemultimedia.
> As a developer that means I have to review it ;-)

yes, I vote for kdemultimedia too. Forget about extragear...

> CMakeLists.txt:
>  * I think you can remove the RPATH settings section, ECM takes care of that, 
> no?

OK, I tried it and I did not see any obvious side effect.
fixed per git commit 3d33357636182550abde5fa628b86a9e241c05f2

> * OPTION(DEBUG "build debug code [default=off]" OFF)
> Why not use the more standard CMAKE_BUILD_TYPE? You have many combinations 
> otherwise, some of them quite wrong.
> CMAKE_BUILD_TYPE=Debug and DEBUG=OFF would lead to -DNO_DEBUG -DQT_NO_DEBUG,
> quite unexpected.

That's right, but I remember that I had some trouble with that, AFAIR
Q_ASSERT did not work even when setting the CMAKE_BUILD_TYPE. Needs some
more investigation, I have put this onto the TODO list. Currently it
also switches on the debug plugin in the menu.

> LICENSES: mentions qt4+kdelibs4, while the code is KF5 based ;)

thanks!
fixed per git commit 4a4d71c60d257bb0a1b3fa4b3ddfdc9aeea574ea

> bin/kwave.wrapper.in mentions KDELANG/KDE_LANG which don't exist in KF5 
> anymore AFAIK. It also mentions KDEDIRS, which definitely doesn't exist 
> anymore. What is the use of COLON_SEPARATED (strange value, not used 
> anywhere)?

Oh, that's old stuff from KDE4 times, obviously needs some cleanup.
Will fix that later, as the wrapper currently does not work here anyway.
on my TODO list

> 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()  (or _ as you name it).

^^^^^^^^^^^^^^^^^^^
no, that's wrong. the _(...) macro has nothing to do with i18n

> No NOOP needed anywhere anymore.
> This means passing the parser to the app later than as a ctor argument though, 
> obviously.

No, I18N_NOOP is still necessary, can not be omitted. I remember that I
had spurious application crashes on startup that were caused by failed
translations and additionally at some points I must handle untranslated
strings internally but have the possibility to translate them on demand
later, thus I need an entry in the translation database.

> _(PACKAGE_VERSION) can't work. How can xgettext extract the string that needs 
> to be translated? *That* is actually a good use case for I18N_NOOP, but it 
> requires generating a cpp file with I18N_NOOP("4.2.1-1"). Well, assuming it 
> makes any sense to translate that; I'm not sure.

No, same as above, _(...) does not do any i18n, it only converts from
ASCII to QString.

> AFAIK KLocalizedString::setApplicationDomain isn't necessary, you should 
> instead define the domain as a -D flag during compilation, but I'm no expert on 
> that, check the wiki.

ok, that topic has already been discussed the last few days...

> App.cpp: parser.command() == _("newwindow")
> You translated internal commands? This seems dangerous to me. For instance:
> libgui/SignalView.cpp: emit sigCommand(_("newwindow(") +
> if someone translates that different string (trailing '(') differently from 
> the translation of "newwindow" + '(', then the whole thing will break.
> Are those command names really exposed to the user somewhere?

No, same thing again. this is no i18n.

> 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?

> 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. 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. 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.

> 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.
I put it on my TODO list...

> Is signal() really threadsafe? Kwave::WorkerThread::run surprises me with its 
> use of unix signals. Even if it works (which I didn't know), the mutex here 
> does not prevent multiple threads from calling it at the same time, since the 
> mutex is a *member* of the thread. Two threads, two mutexes, no mutual 
> exclusion. On top of that, "forbid sending SIGHUP" will forbid sending SIGHUP 
> to any thread, not just the one currently exiting, right?

POSIX says that this signalling does not (need to) work in multithreaded
environments. But in Linux it does. The signal is only sent to the right
thread, so no problem here. That worked 100% stable for years.

> Still in that file, cancel() and shouldStop() should lock m_lock (bool isn't an 
> atomic type, contrary to what many people think, see C++11 memory model).

That's not necessary, even if bool is not handled atomically, the
evaluation against zero or non-zero still works, so which kind of
intermediate states or race conditions could happen here? Would you feel
better if I change it to a QAtomicInt or so?

> Indentation: you seem to use 4 spaces for level-1 indentation, and then one 
> tab for level-2 indentation. Vieweing the file in vim (which here seems to use 
> a tab width of 4) makes the file unreadable. Might I suggest something more 
> portable, like 4 spaces, 8 spaces? (Qt5/KF5 coding style).

That's just the style it always had. Kdevelop supports it. I also did
not like that very much, but one gets used to it. I think changing it
only produces a lot of traffic in the version control and no customer
has any benefit from that.

> Did you ever try compiling with clang? If not I recommend giving it a try, it 
> will point out many more things than my small brain can figure out ;-)

Yes, I did and do that regularly. It has pointed out many things, but
also had some bad side effects, like some plugin that stopped working
due to strange linker breakage, some instrumentations did not work and
AFAIR the combination with valgrind also made trouble...

> On the positive side, the amount of API docs is nice ;)

thanks, I do my best to keep it up to date and complete :)

kind regards,
   Thomas