Re: Haruna in kdereview
El dilluns, 9 d’agost de 2021, a les 22:02:29 (CEST), George Florea Banus va escriure: > On 09.08.2021 22:22, Albert Astals Cid wrote: > > El dilluns, 9 d’agost de 2021, a les 16:23:27 (CEST), George Florea Banus > > va escriure: > >> Or where to go from here. > > Which are your doubts? > > I'd like to get the review done so I can make a new release. > > Also, after review, how do I version the app? Do I have to make a 1.0 > release or can I make it 0.7.0? That's all up to you :) https://community.kde.org/ReleasingSoftware should have some guidelines. Cheers, Albert > > > Cheers, > >Albert >
Re: Haruna in kdereview
On 09.08.2021 22:22, Albert Astals Cid wrote: El dilluns, 9 d’agost de 2021, a les 16:23:27 (CEST), George Florea Banus va escriure: Or where to go from here. Which are your doubts? I'd like to get the review done so I can make a new release. Also, after review, how do I version the app? Do I have to make a 1.0 release or can I make it 0.7.0? Cheers, Albert
Re: Haruna in kdereview
El dilluns, 9 d’agost de 2021, a les 16:23:27 (CEST), George Florea Banus va escriure: > Or where to go from here. Which are your doubts? Cheers, Albert > > On 19.07.2021 15:54, George Florea Banus wrote: > > Hello > > > > I would like to move Haruna to KDEReview. > > > > Haruna is a media player based on mpv, focus is on video playback. > > > > https://invent.kde.org/multimedia/haruna > > >
Re: Haruna in kdereview
On 21.07.2021 19:38, Jonah Brüchert wrote: 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. Sorry for ignoring. I already made a FindYouTubeDl module based on Findsmartctl by the time you posted.
Re: Haruna in kdereview
I have addressed everything that was brought to my attention. Let me know if there's more. Or where to go from here. On 19.07.2021 15:54, George Florea Banus wrote: Hello I would like to move Haruna to KDEReview. Haruna is a media player based on mpv, focus is on video playback. https://invent.kde.org/multimedia/haruna
Re: Haruna in kdereview
On 21.07.2021 18:42, Harald Sitter wrote:
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?
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 }
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.
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
Re: Haruna in kdereview
El dimarts, 20 de juliol de 2021, a les 8:16:58 (CEST), George Florea Banus va escriure: > > The thing you call Header in View -> "Hide header" is called > > historically called toolbar, i think you should rename it. > > I'll called it header because that's what Qt calls it and the footer is > also a toolbar. > Do you want me to change only user facing part or everything? What > about the footer? User facing part is enough. Typically one would call the footer status bar, but that one is so full of widgets that i don't think statusbar would be a good name, anyhow the name is not exposed to the user, right? Cheers, Albert > > > All in all, the app is nice, but IMHO suffers a bit from being QML > > based and not being able to use KXMLGUI. > > > > Cheers, > >Albert > > Thanks for the review. > > >
Re: Haruna in kdereview
Formatting of my first reply looks terrible in the web mailing list.
Hopefully this one is better. Sorry.
On 20.07.2021 00:39, Albert Astals Cid wrote:
El dilluns, 19 de juliol de 2021, a les 14:54:26 (CEST), George Florea
Banus va escriure:
You need a
KLocalizedString::setApplicationDomain("haruna")
just after creating your QApplication. Otherwise translations are not
going to be loaded.
Done.
The menus look is a bit broken (separators and highlight being
drawn past the actual menu width, icons being painted "blurrily"), but
i guess that's not really your fault.
Nothing done here.
The Setting has a "Help!" <- remove the !
Done.
in that "Help!" menu you have a KHelpCenter entry that should be
called "Haruna handbook" like in the rest of applications.
Done.
The first time i open the app the "Color scheme" combo box doesn't
have contents, given there's a default entry i'd expect to see that.
I encountered a weird issues here. The color scheme is saved to the
"ColorScheme" entry
https://invent.kde.org/multimedia/haruna/-/blob/master/src/settings/generalsettings.kcfg#L46
But the default value for "GeneralSettings::colorScheme()" is
BreezeLight, even if I set a default value.
If I rename the entry, for example to "ColorScheme123" then it gets the
correct default value.
Or if I move it to another settings class like MouseSettings, then it
also gets the correct default value.
The "english only" help is kind of weird. Why are you using html for
that? Seems like something you should do with tooltips and if those
QML components don't support tooltips, at least make the help page
with QML so it's translatable?
That's what I used before adding the docbook file. The content of the
html pages are the same as the content in the docbook.
I kept them because the KHelpCenter doesn't render the html correctly.
Sometimes there's no space between words, if rows are long enough they
get behind the scrollbar, there's no bold.
As for why not use tooltips, sometimes there's just to much text.
https://invent.kde.org/multimedia/haruna/-/blob/master/help/VideoSettings.html
I've no idea what the "Open config..." entries in the settings menu
are supposed to do, but both fail with "file doesn't exist" for me.
They open the config file and config folder, but it tried to open a
wrong path, I fixed that now.
And now it's also hidden when the config folder doesn't exist.
Esc not only exits fullscreen (good) but enters fullscreen (weird).
Fixed.
The "Primary Subtitle" MenuItem that tries to be a title is kind of
weird because it has exactly the same look that the other menus,
at least disable clicking on it, because i can click on it and then
it's even more confusing if it's really wants to be a title or not
without reading the code :D
Yes, it's used as title. I wanted to change that menu, but couldn't come
with anything better.
I think I'll change it so it manages only the primary subtitle.
It does remember the last thing i was playing, which I dislike, I
tried to look for a config option to disable it but doesn't seem to be
one. Feel free to ignore this.
I'll add an option to disable that behavior, someone else requested it too.
The thing you call Header in View -> "Hide header" is called
historically called toolbar, i think you should rename it.
I'll called it header because that's what Qt calls it and the footer is
also a toolbar.
Do you want me to change only user facing part or everything? What
about the footer?
All in all, the app is nice, but IMHO suffers a bit from being QML
based and not being able to use KXMLGUI.
Cheers,
Albert
Thanks for the review.
Re: Haruna in kdereview
On 20.07.2021 00:39, Albert Astals Cid wrote:
El dilluns, 19 de juliol de 2021, a les 14:54:26 (CEST), George Florea Banus va
escriure:
You need a
KLocalizedString::setApplicationDomain("haruna")
just after creating your QApplication. Otherwise translations are not going to
be loaded.
Done.
The menus look is a bit broken (separators and highlight being drawn past the actual
menu width, icons being painted "blurrily"), but i guess that's not really your
fault.
Nothing done here.
The Setting has a "Help!" <- remove the !
Done.
in that "Help!" menu you have a KHelpCenter entry that should be called "Haruna
handbook" like in the rest of applications.
Done.
The first time i open the app the "Color scheme" combo box doesn't have
contents, given there's a default entry i'd expect to see that.
I encountered a weird issues here. The color scheme is saved to the
"ColorScheme" entry
https://invent.kde.org/multimedia/haruna/-/blob/master/src/settings/generalsettings.kcfg#L46
But the default value for "GeneralSettings::colorScheme()" is
BreezeLight, even if I set a default value.
If I rename the entry, for example to "ColorScheme123" then it gets the
correct default value.
Or if I move it to another settings class like MouseSettings, then it
also gets the correct default value.
The "english only" help is kind of weird. Why are you using html for that?
Seems like something you should do with tooltips and if those QML components don't
support tooltips, at least make the help page with QML so it's translatable?
That's what I used before adding the docbook file. The content of the
html pages are the same as the content in the docbook.
I kept them because the KHelpCenter doesn't render the html correctly.
Sometimes there's no space between words, if rows are long enough they
get behind the scrollbar, there's no bold.
As for why not use tooltips, sometimes there's just to much text.
https://invent.kde.org/multimedia/haruna/-/blob/master/help/VideoSettings.html
I've no idea what the "Open config..." entries in the settings menu are supposed to do,
but both fail with "file doesn't exist" for me.
They open the config file and config folder, but it tried to open a
wrong path, I fixed that now.
And now it's also hidden when the config folder doesn't exist.
Esc not only exits fullscreen (good) but enters fullscreen (weird).
Fixed.
The "Primary Subtitle" MenuItem that tries to be a title is kind of weird because
it has exactly the same look that the other menus, at least disable clicking on it,
because i can click on it and then it's even more confusing if it's really wants to be a
title or not without reading the code :D
Yes, it's used as title. I wanted to change that menu, but couldn't come
with anything better.
I think I'll change it so it manages only the primary subtitle.
It does remember the last thing i was playing, which I dislike, I tried to look
for a config option to disable it but doesn't seem to be one. Feel free to
ignore this.
I'll add an option to disable that behavior, someone else requested it too.
The thing you call Header in View -> "Hide header" is called historically
called toolbar, i think you should rename it.
I'll called it header because that's what Qt calls it and the footer is
also a toolbar.
Do you want me to change only user facing part or everything? What
about the footer?
All in all, the app is nice, but IMHO suffers a bit from being QML based and
not being able to use KXMLGUI.
Cheers,
Albert
Thanks for the review.
Re: Haruna in kdereview
El dilluns, 19 de juliol de 2021, a les 14:54:26 (CEST), George Florea Banus va
escriure:
> Hello
>
> I would like to move Haruna to KDEReview.
>
> Haruna is a media player based on mpv, focus is on video playback.
>
> https://invent.kde.org/multimedia/haruna
You need a
KLocalizedString::setApplicationDomain("haruna")
just after creating your QApplication. Otherwise translations are not going to
be loaded.
The menus look is a bit broken (separators and highlight being drawn past
the actual menu width, icons being painted "blurrily"), but i guess that's not
really your fault.
The Setting has a "Help!" <- remove the !
in that "Help!" menu you have a KHelpCenter entry that should be called "Haruna
handbook" like in the rest of applications.
The first time i open the app the "Color scheme" combo box doesn't have
contents, given there's a default entry i'd expect to see that.
The "english only" help is kind of weird. Why are you using html for that?
Seems like something you should do with tooltips and if those QML components
don't support tooltips, at least make the help page with QML so it's
translatable?
I've no idea what the "Open config..." entries in the settings menu are
supposed to do, but both fail with "file doesn't exist" for me.
Esc not only exits fullscreen (good) but enters fullscreen (weird).
The "Primary Subtitle" MenuItem that tries to be a title is kind of weird
because it has exactly the same look that the other menus, at least
disable clicking on it, because i can click on it and then it's even more
confusing if it's really wants to be a title or not without reading the code :D
It does remember the last thing i was playing, which I dislike, I tried to look
for a config option to disable it but doesn't seem to be one. Feel free to
ignore this.
The thing you call Header in View -> "Hide header" is called historically
called toolbar, i think you should rename it.
All in all, the app is nice, but IMHO suffers a bit from being QML based and
not being able to use KXMLGUI.
Cheers,
Albert
Haruna in kdereview
Hello I would like to move Haruna to KDEReview. Haruna is a media player based on mpv, focus is on video playback. https://invent.kde.org/multimedia/haruna
