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