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