[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-core-devel
Subject:    Re: KJournald in KDE-Review
From:       Nicolas Fella <nicolas.fella () gmx ! de>
Date:       2022-10-09 21:47:45
Message-ID: 8614c8b5-8b86-16a2-b7e1-1449e6c1a450 () gmx ! de
[Download RAW message or body]


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


["Screenshot_20221009_233713.png" (image/png)]

[prev in list] [next in list] [prev in thread] [next in thread]