New repo in kdereview: kasts

[Harald Sitter]

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!

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

- your appdata file has no screenshots defined

- 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

- 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

- 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

- 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

- 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.

- 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]

- 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]

- 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 is a `// TODO: implement` in the main.qml. you might want to do that ;)

- AudioManager::timeString probably can be replaced by KFormat::formatDuration

- 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)

- 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

- 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.

- 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)`

- QueueModel has an m_audio member that isn't ever set or used from
what I can tell

[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