On 21.07.2021 13:53, Harald Sitter wrote:
It's an amazing app! Really awesome.
Thank you.
- you might want to consider using a newer version in your find_package call for ECM. ECM does enable more modern/useful features but only when used with a suitably high version. so, I'd put this at the actual minimal version you want to support
Done.
- the color-schemes dir seems to do nothing. it installs files that don't actually exist in the source. fix it or rm -rf?
It had the Breeze color schemes, but I removed them because I don't know their licenses
https://bugs.kde.org/show_bug.cgi?id=434465
- for the screenshots.md and the metainfo.xml you should consider using our screenshot CDN instead https://invent.kde.org/websites/product-screenshots
Already submitted a merge request
- not sure how big of a concern that will be in practice, but you should be careful with calling mimeTypeForFile, it is potentially costly and a local path may still be backed by a network'd mount. when in doubt it's probably better to check KFileItem::isSlow() first and use mimeTypeForUrl when isSlow is true (or resolve the mimetype in a qfuture/thread)
mimeTypeForUrl says it will use mimeTypeForFile for local files. Is checking with KFileItem::isSlow() still necessary?
Or is it still needed because "a local path may still be backed by a network'd mount"?
- Application::aboutApplication: we do have an AboutPage in klirigami these days, maybe check that out so you can get rid of the qwidget dialog
Done.
- you should codify the runtime dep on youtube-dl via cmake. make a dummy finder and set_package_properties it as type RUNTIME e.g. https://invent.kde.org/plasma/plasma-disks/-/blob/master/cmake/Findsmartctl.cmake
Done.
some further nitpicks: - _debug.h should actually be superfluous as qdebug can inject context on its own https://doc.qt.io/qt-5/qtglobal.html#qSetMessagePattern
Done.
- Application::formatTime might want to use kformat::formatduration https://api.kde.org/frameworks/kcoreaddons/html/classKFormat.html
KFormat::formatDuration doesn't seem to add leading 0, so I'll keep the existing code, unless something's wrong with it.
- the way static singletons are managed looks a bit old school. function local statics might be neater https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables Single *instance() { static Single s; return &s; } HS
I searched a little about this and people also recommend disabling the copying and moving for singletons.
Any opinions on that? Thanks for the review.
