announcement: Kwave is in kdereview

[David Faure]

On samedi 8 octobre 2016 09:39:24 CEST Thomas Eschenbacher wrote:
> Hello,
> 
> according to
> https://community.kde.org/Policies/Application_Lifecycle#Stage_2:_Stable
> I hereby announce the move of Kwave to "kdereview", on it's way to
> "kdemultimedia".

This is very good news. I have been a long time user of kwave
(not frequently, but regularly), and I am very happy to see it join the KDE 
infrastructure.

> Background:
> This is attempty #5. The first one (~2005) has hit the migration from
> CVS to SVN and I was asked to postpone it. The second one was ~2009,
> there I hit the migration from SVN to GIT. The third one was 2013, and
> finally 2015 I got into kdereview, where the project resides right now -
> and waits for approval to go further into kdemultimedia.

Wow, you have never been lucky with timing, it would appear.

As a user, I definitely vote for kwave going to kdemultimedia.
As a developer that means I have to review it ;-)

CMakeLists.txt:
 * I think you can remove the RPATH settings section, ECM takes care of that, 
no?
* 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.

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

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

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 NOOP needed anywhere anymore.
This means passing the parser to the app later than as a ctor argument though, 
obviously.

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

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.

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?

Kwave::TopWidget::executeCommand: is the "if (false) {" around a lot of code 
intended? Looks weird.

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

Kwave::Stripe::resize detaches without the mutex locked, while other methods 
in the same file do it with the mutex locked.

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

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

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 ;-)

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

OK, I have to stop here. I didn't read everything, and I'm not promising I 
ever will :)

-- 
David Faure, faure at kde.org, http://www.davidfaure.fr
Working on KDE Frameworks 5