Haruna in kdereview

[George Florea Banus]

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.