| dfaure |
| Plasma | |
| Konqueror |
Changes:
kDebug() -> qCDebug() with category, header file generation and category file install Read single click setting from style instead of KGlobalSettings Use new signal connection syntax QAction::triggered() no longer has (Qt::MouseButtons,Qt::KeyboardModifiers) parameters Use KIO::convertSize() to display total size Update tests to compile and run Deprecated QFontMetrics::width() -> horizontalAdvance() fsviewpart.rc installed in accordance with current standard
Builds and runs with these changes.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
I didn't test this, but I just want to thank you for keeping fsview alive. It's the main reason I still have Konqueror installed.
Correction to list of changes above - update to new signal connection syntax will follow in a separate review.
Nice work!
| plugins/fsview/fsview.cpp | ||
|---|---|---|
| 530 | You could remove all the if(1), qCDebug is off by default and easy to turn on. For the if(0), well, maybe a different verbose category... Anyway, not for this patch. | |
| plugins/fsview/fsview_part.cpp | ||
| 151 | I thought there was an activated() signal to encapsulate this logic? | |
Agreed that debugging is a mess, especially with the if(1) and if(0) randomly scattered around. Will address this later.
Would you also be able to look at D28551 (porting signal connections)?
| plugins/fsview/fsview_part.cpp | ||
|---|---|---|
| 151 | For QAbstractItemView and derived, yes, The item view here though is completely custom, though, so either TreeMapWidget or FSJob would need to look at the style option anyway. Trying to change the code as little as possible for the moment, although I agree that it would be cleaner to look at the option and emit a single activated() signal in TreeMapWidget. | |
Getting a CMake error on plugins/fsview/CMakeLists.txt line 16 (ecm_qt_declare_logging_category). Fedora 30, i686, KF5 5.64.
CMake Error at /usr/share/ECM/modules/ECMQtDeclareLoggingCategory.cmake:73 (message): Unexpected arguments to ecm_qt_declare_logging_category: EXPORT;fsview;DESCRIPTION;Konqueror FileSizeView plugin Call Stack (most recent call first): plugins/fsview/CMakeLists.txt:16 (ecm_qt_declare_logging_category)
@rrosch: The EXPORT and DESCRIPTION arguments to ecm_qt_declare_logging_category are supported since ECM 5.68.0.
If newer version of ECM is required, this needs to be reflected when searching for the dependencies. Currently the toplevel CMakeLists.txt only expects ECM/KF 5.38.0. So you need to bump KF5_MIN_VERSION to reflect the newer needs.
Is there a temporary workaround for people who cannot yet upgrade to 5.68? Something we could wrap in something like
#if KIO_VERSION < QT_VERSION_CHECK(5, 68, 0)
?
Especially since there are some people wanting to try out the new sidebar code for example, who might not currently be able to upgrade ECM. I'm on fedora 30, and the latest version available from official repos that I can find is 5.64. (Edit: just found this: https://koji.fedoraproject.org/koji/buildinfo?buildID=1481947 -- but same issue still applies for others who cannot upgrade yet)
If you build konqueror git master, you can most certainly build ECM from git master (it has no dependency, it's really easy to build)
I just found this unsubmitted comment, not sure what happened, but I figured it would be useful information to pass along anyway. (I somehow managed to work around it a month ago, without compiling ECM):
Though ECM itself didn't have any requirements, it did trigger several requirements (newer versions of already installed packages) in order to compile konqueror, which themselves would have required a full upgrade of KF5 for me (for stability). I have not yet looked into what going the git ECM path would lead to, but in this case I will just wait for the fedora koji packages for KF5 5.68 to make it to the repos.