Re: [linux-minidisc] Qt5 compatibility

[Michael Karcher <Michael.Karcher@fu-berlin.de>]

Hi, the evil critic is back...

Am Sonntag, den 01.12.2013, 23:29 +0100 schrieb Thomas Arp:
> i upgraded my windows build environment to Qt5 for testing.
> I made some changes so that the code can be complied with Qt4 and Qt5.
Thanks for your patch, but let me comment on it.

You remove "QtGui/" from the include directory paths, as it is mostly
"QtWidgets/" in Qt5. This is fine, as the module-specific subdirectories
are put into the include path by qmake. But this has a bad end result:
We pick up headers from QtCore using paths specifying the include
directory explicitly (like <QtCore/QApplication>), but we pick up
headers from QtGui without specifying it. I really dislike that
inconsistency. In my oppinion, we should drop "QtCore/" as well.


Furthermore, some things look like should be improved/fixed:

> @@ -282,8 +282,8 @@ bool QHiMDMainWindow::autodetect_init()
>  {
>      int k;
> 
> -    k = QObject::connect(detect, SIGNAL(himd_found(QString)), this, SLOT(himd_found(QString)));
> -    k += QObject::connect(detect, SIGNAL(himd_removed(QString)), this, SLOT(himd_removed(QString)));
> +    k = (bool)QObject::connect(detect, SIGNAL(himd_found(QString)), this, SLOT(himd_found(QString)));
> +    k += (bool)QObject::connect(detect, SIGNAL(himd_removed(QString)), this, SLOT(himd_removed(QString)));

I would prefer "k" to be a boolean variable here, which makes the first
cast superflous. You also can get rid of the second cast by writing that
line "k = k || QObject::connect(...)".
In my oppinion that one expresses the intent better that we want to know
whether the first *or* the second connect worked.
In fact, I would suggest to refactor the code this way:


>     bool worked = QObject::connect(detect, SIGNAL(himd_found(QString)), this, SLOT(himd_found(QString)))
>                || QObject::connect(detect, SIGNAL(himd_removed(QString)), this, SLOT(himd_removed(QString)));
> 
>     if(!worked)
>         return false;

The "||" operator implicitly treats the parameters as bools, and
"worked" is more expressive than "k".


I also dislike this hunk:

> @@ -300,7 +300,7 @@ void QHiMDMainWindow::open_himd_at(const QString & path)
>      QMessageBox himdStatus;
>      QString error;
> -    error = trackmodel.open(path.toAscii());
> +    error = trackmodel.open(path.toLatin1());
>      if (!error.isNull()) {
>          himdStatus.setText(tr("Error opening HiMD data. Make sure you chose the proper root directory of your HiMD-Walkman.\n") + error);

But this is not really your fault. You did do the correct transformation
here, but the code was bad before to begin with. QHiMDTracksModel
accepts a const QString &, so there is no need at all to convert the
string into the "default 8-bit encoding" (toAscii, Qt4 only) or Latin 1.
In fact, QHiMDTracksModel converts the path to utf-8, as this is what
glib (on which libhimd is based) likes to see as filename encoding. 

In fact, the old, bad, code was indeed (accidentally) working correctly
on Qt 4 as long as the file name was representable in the "default 8-bit
encoding", which could have been be something different from Latin 1
(but as no one calls setCodecForCStrings was Latin 1 all the time),
because the returned QByteArray was implicitly converted back to a
QString using the constructor accepting a QByteArray that undid
toAscii() per spec. On the other hand, in Qt5, the constructor of
QString accepting a byte array does *not* undo toLatin1, but interprets
the string as UTF-8 instead. So that patch breaks filenames like
"Blödsinn".

As we are talking about all those toLatin1 calls: There are a lot of
them in the QHiMDWinDetection code. The one for getting the drive letter
are OK in my oppinion, but is there any reason to use toLatin1 and the
CM_Locate_DevnodeA, instead of using toUtf16 and CM_Locate_DevodeW?

And last, but not least: You use the dreaded "reinterpret_cast" to cast
from void* to MSG*. As far as I understand C++, you should be able to
use static_cast as well. While there are no differences in the binary
code of either variant, static_cast is typically preferred a lot as
reinterpret_cast is able to do many strange things no one should ever
do, whereas static_cast is for the common, more safe things. In case a
reinterpret_cast is also valid as static_cast, as is the case here, I
*think*, the reinterpret cast is compiled exactly the same way as the
corresponding static_cast. It might even be defined that way in the C++
standard, but I am too lazy to look it up now.

As your patch is already in, I suggest proceeding this way:
1) Commit a second patch that removes "QtCore" from the include
directives (and does *just* that)
2) Commit one patch that rephrases the test for autodetectability and
removes the bad "toLatin1" as "minor fixes" patch. I think the ideal
thing from a traceability perspective (who did what when and why) would
be to have this two patches, as these are two logical changes, caused by
different reasons, and even two different severities (one is a code
"clean-up" without functional change, the other one is a bug fix), but I
understand worries about "commit bloat" for small changes, too. So do it
either way you want, or drop the "cleanup" completely, if you decide you
want to keep the "+=" construct with the casts to bool.

> .: Tested with Qt 4.7 and Qt 5.1 with gcc on Windows XP
> _______________________________________________
> linux-minidisc mailing list
> linux-minidisc@lists.fu-berlin.de
> https://lists.fu-berlin.de/listinfo/linux-minidisc