New repo in kdereview: kasts

[Bart De Vries]

On 27/05/2021 15:09, Harald Sitter wrote:
> On Thu, May 27, 2021 at 12:20 PM Bart De Vries <bart at 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