Haruna in kdereview

[George Florea Banus]

On 21.07.2021 18:42, Harald Sitter wrote:
> On Wed, Jul 21, 2021 at 4:38 PM George Florea Banus
> <georgefb899 at gmail.com> wrote:
>> On 21.07.2021 13:53, Harald Sitter wrote:
>>> - 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
> Good call. Maybe add a comment to the commented out option in the
> cmakelists? There is also the question whether it is a good idea to
> copy the schemes to begin with though. What's the use case behind
> this?

I think it was for Windows.

>>> - 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?
> You are right, I am misremember. What you want to do is match on
> extension only when the file is slow
> https://doc.qt.io/qt-5/qmimedatabase.html#MatchMode-enum
>
> Mind you, putting the mimetype resolution into a QFuture (and not
> change the matchmode) still might be better choice over all since you
> eventually create previews that will read form the file anyway. So, if
> you first resolve a mimetype no harm is done, so long as you don't do
> it on the qApp thread.
>
>> Or is it still needed because "a local path may still be backed by a
>> network'd mount"?
> Yep, that is what KFileItem::isSlow() is telling us. If you always
> resolve in a QFuture (even for local files) you don't really need to
> check isSlow.

I opted for the KFileItem::isSlow() version as I never used QFuture 
before and it seems even more complicated when QML comes into play.

https://invent.kde.org/multimedia/haruna/-/commit/4a4ba7f35ba333b9263b8aecf805189700647a52

>>> - 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?
> Sounds like a good idea ->
> https://doc.qt.io/qt-5/qobject.html#Q_DISABLE_COPY_MOVE in private
> section of class should get that sorted

Done.