KJournald in KDE-Review

[Nicolas Fella]

Am 09.10.22 um 19:18 schrieb Andreas Cord-Landwehr:
> Hi, after a few releases over the past year, I would like to get KJournald
> included in KDE application releases. This project is about providing both an
> QItemModel abstraction library for the C-style journald API and providing an
> efficient graphical browser for journald logs.
>
> Sysadmins moved the repository to KDE Review today, you can find the source
> code here:
>
> https://invent.kde.org/libraries/kjournald
>
> (flatpack packaging is also available for easy trying it out)
>
> Even though KJournald is currently contained in the "libraries" playground
> module, I would like to get it included in the "utilities" module after
> passing KDE Review. The reason is that at the moment I more focus on the
> application part and that is the most user-facing part. Having it in
> "utilities" thus will avoid confusion.
>
> Looking forward for your review comments!
>
> Best regards,
> Andreas

Hi,

overall looks very good. Some smaller comments:

- reuse coverage is *almost* perfect (except for the po/ folder, but
that's a whole different topic). Would be nice to get that to 100% and
enable the reuse CI

- I'd recommend that you enable the clang-format integration and commit
hook from ECM and format the repo instead of a "custom" .clang-format file

- With the default window size the toolbar is cut off (see screenshot)

- The content of the log list goes below the scrollbar (see screenshot)

- The app is using context properties to pass stuff to QML, which is
deprecated/bad practice

- The desktop file has "Exec=kjournaldbrowser %U", but looking at the
code it seems to only process one URL, so it should be %u instead of %U

- "appstreamcli validate --pedantic
browser/org.kde.kjournaldbrowser.appdata.xml" has some complaints

- Since there's already a Flatpak version I'd recommend to enable the
Flatpak Gitlab CI. See
https://invent.kde.org/network/neochat/-/merge_requests/491 for an example

- Assuming the Flatpak version is stable it should be released on Flathub

Cheers

Nico

-------------- next part --------------
A non-text attachment was scrubbed...
Name: Screenshot_20221009_233713.png
Type: image/png
Size: 250511 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20221009/1103fb16/attachment-0001.png>