Re: Haruna in kdereview
- 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
In AudioTube I've been using this FindYoutubeDl module. Maybe you can
make use of it too.
And if someone on the list finds a problem with it, let me know so I can
also fix it in AudioTube.
# SPDX-FileCopyrightText: 2021 Jonah Brüchert
#
# SPDX-License-Identifier: BSD-2-Clause
find_package(Python3 REQUIRED COMPONENTS Interpreter)
execute_process(COMMAND ${Python3_EXECUTABLE} -c "import youtube_dl"
RESULT_VARIABLE YTDL_CHECK_RESULT)
if (${YTDL_CHECK_RESULT} EQUAL 0)
set(YoutubeDL_FOUND TRUE)
endif()
OpenPGP_0xA81E075ABEC80A7E.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: Haruna in kdereview
On Wed, Jul 21, 2021 at 4:38 PM George Florea Banus
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?
> > - 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.
> > - 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 }
> >
> > 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
On Wed, Jul 21, 2021 at 4:38 PM George Florea Banus
wrote:
>
> 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
Re: Haruna in kdereview
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 }
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.
Re: Haruna in kdereview
It's an amazing app! Really awesome.
- 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
- the color-schemes dir seems to do nothing. it installs files that
don't actually exist in the source. fix it or rm -rf?
- for the screenshots.md and the metainfo.xml you should consider
using our screenshot CDN instead
https://invent.kde.org/websites/product-screenshots
- 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)
- Application::aboutApplication: we do have an AboutPage in klirigami
these days, maybe check that out so you can get rid of the qwidget
dialog
- 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
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
- Application::formatTime might want to use kformat::formatduration
https://api.kde.org/frameworks/kcoreaddons/html/classKFormat.html
- 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 }
HS
