On 27/05/2021 15:09, Harald Sitter wrote:
On Thu, May 27, 2021 at 12:20 PM Bart De Vries <b...@mogwai.be> wrote:
Hello everyone!
I would like to move kasts to kdereview.
https://invent.kde.org/plasma-mobile/kasts
<https://invent.kde.org/plasma-mobile/kclock>
Kasts is a podcast app for Plasma Mobile. It was started as a fork of
Alligator, the Plasma Mobile feed reader. It currently features a play queue,
play position resume/persistence, MPRIS2 support, and suspend inhibition.
Hey.
It's amazing. Also in incredible shape, well done!
Thank you! And thanks for the thorough review.
Some stuff I've noticed:
- the source is almost reuse compliant but not quite. needs some extra
data files equipped with CC-0 or the like [1][2]. You could then also
add an include on
https://invent.kde.org/sysadmin/ci-tooling/raw/master/invent/ci-reuse.yml
to your gitlab-ci.yml to ensure it stays at complete coverage
We've made all source files reuse compliant with one exception: the dbus
interface files. We couldn't figure out which license to apply.
Any pointers are welcome.
- your appdata file has no screenshots defined
Done, and screenshots have been added to cdn.kde.org.
- since you require qt 5.15 you can use Qt::Core etc. targets instead
of Qt5::Core. this will make porting to qt6 less work
Done. Thanks!
- the page stack between Settings and About is a bit wonky. they both
push onto layer stack rather the regular stack (I guess?) which seems
not exactly ideal considering from the way these two are presented in
the UI they should behave same as Queue and Episodes etc..
particularly problematic is however that if you switch between
settings and about you keep growing the page stack and the only way to
get back to the "real page" is to manually unwind the stack again
Your analysis is indeed correct.
Kasts is using PagePool and PagePoolAction for the navigation, and is
indeed pushing Settings and About to the layers stack. According to the
Kirigami docs, the layers stack is intended for modal pages, which
Settings and About are, in my opinion. What you're seeing is the
default behaviour of PagePoolAction.
I completely agree that the end result is very confusing, to say the
least. As a workaround, I've changed it such that Settings and About
are pushed to the regular stack. So they are now treated in the same
way as the other main pages.
Next to that, Carl Schwan has opened an MR to make the default
PagePoolAction less confusing. [1]
- also on the subject of the navigation pane: I believe the current
page ought to be highlighted in the navigation pane. when I'm drilled
down into a subpage of e.g. the Episodes page I wouldn't know that
this is where I came from other than through unwinding the stack
manually again
Very good point.
I've been checking, but it seems that PagePoolAction does not allow
highlighting of the currently active item.
Combined with the item above, that means I'll be porting away from
PagePoolAction soon in order to implement this.
- something is wonky with the colors on desktop. in all listviews it
looks like the delegates are inactive/disabled colors for some reason.
e.g. https://i.imgur.com/xkgigs4.png
Ah, yes, this is intended behaviour.
When you first import a podcast feed (or OPML), it will mark all
episodes up to that point as 'played'. Which will show them with those
inactive/disabled colors. Once the feed's been imported, new episodes
will show up with regular colors until they've been marked as 'played'.
This behaviour will save users having to manually mark all episodes as
marked.
- I'm not super certain but also on that same page you might need to
turn the entire "by %1" sequence into a translatable string rather
than just "by". I don't think you can presume that every language has
that particular order? you might want to ask on the kde-i18n-doc
mailing list.
Done.
- the generic entry delegate hardcodes size formatting, this results
in my system usually talking about MiB everywhere but kasts is talking
about MB (yet indeed it still means MiB). You'll want to use kformat
[3]
Done.
Thanks for pointing this out. I'm fairly inexperienced with KDE libs,
so I wasn't aware of kformat (same for time durations mentioned below).
- you may want to do a pass over all strings. I'm seeing some action
strings not using title capitalization (e.g. the actions in
GenericEntryDelegate.qml - "Add to queue") [4]
Done, all strings are now properly capitalized.
And again, thanks for the pointer to the docs.
- I see you are hardcoding a color in GenericHeader.qml, I'm not super
sure that will work correctly across different themes but then I also
don't really see the color actively used :shrug:
There are two hardcoded colours: one is an alpha mask to make the
blurred background picture on the header dark enough to have enough
contrast to be able to read the feed title in white (second hardcoded
colour).
This should work across all possible themes, but I'm open to other, more
elegant solutions.
- there is a `// TODO: implement` in the main.qml. you might want to do that ;)
Done.
- AudioManager::timeString probably can be replaced by KFormat::formatDuration
Done.
- AudioManager has lots of commented out qdebugs. I suggest to either
remove them, or turn them into qCDebugs (so they may be
enabled/disabled at runtime)
Done: ported to qCDebugs.
- AudioManager deals a lot with time numbers. it'd aid readability
(and probably type safety) if you used std::chrono type and in
particular also chrono literals
I'll have to have a closer look at this. AudioManager is a wrapper
around QMediaPlayer which is using qint64 for durations and positions.
As such, the current implementation avoids type conversions by using
qint64 everywhere for time numbers.
While I agree that std::chrono might be a more logical choice here, I'm
afraid it would also introduce a lot more type conversions to the
underlying QMediaPlayer.
- AudioManager uses some "old style" stringy connects
`loop.connect(&timer, SIGNAL(timeout()), &loop, SLOT(quit()));` these
SIGNAL() and SLOT() bits are only evaluated at runtime. To have the
compiler ensure the functions are valid I would very strongly suggest
to change them to function pointers like in the other connect calls.
Fixed.
- AudioManager also has some singleShot(0, lambda) calls. I believe
these are kind of unsafe since `this` might get deleted between the
call and the execution. you'll want to contextualize them
`singleShot(0, this, lambda)`
Fixed.
- QueueModel has an m_audio member that isn't ever set or used from
what I can tell
Argh, that's a leftover from a refactoring.
Fixed.
[1] https://apachelog.wordpress.com/2021/04/06/reuse-licensing-helper/
[2]
https://community.kde.org/Guidelines_and_HOWTOs/Licensing#License_Statements_in_Non-Source-Code_Files
[3]
https://invent.kde.org/plasma/plasma-nm/-/blob/9005f2580c37d5dbf32f1d264eb87d73ca723829/applet/contents/ui/ConnectionItem.qml#L258
[4] https://develop.kde.org/hig/style/writing/capitalization/
HS
[1] https://invent.kde.org/frameworks/kirigami/-/merge_requests/306
Best regards,
Bart
