Re: KJournald in KDE-Review
Hi, > in the left side filter area, there are tooltips, do you mean others? Oh, indeed, maybe I was too impatient with my mouse, I can see them now :) > Do you have a hint how to do this best? Since we should not use contextProperties anymore, I do not directly see how to avoid a proxy class. Volker has done that a lot, see for example https://invent.kde.org/vkrause/kpublicalerts/-/blob/master/src/app/main.cpp#L109 Cheers Kai Uwe
Re: KJournald in KDE-Review
Hi Kai, many thanks for this very detailed review! For the nitpicks, I did a first cleanup. Some of the not low hanging fruits regarding UI and features I added to the backlog items in the Invent project. More details below. Cheers, Andreas On Donnerstag, 3. November 2022 14:58:06 CET Kai Uwe Broulik wrote: > * bootmodel.cpp: I don't think you need to call beginResetModel() when > you populate it in the constructor done > * bootmodel.cpp: QAbstractListModel is a flat list with a single column, > no need to re-implement parent(), columnCount(), etc done > * bootmodel.cpp: Admittedly it's prettier to have an Q_INVOKABLE for > bootId but if you made the roles Q_ENUM, you could call data() from QML > wiht e.g. bootModel.data(bootModel.index(row, 0), BootModel.FooRole) thanks, was actually an obsolete method, but Q_ENUM for roles is obviously a good thing > * colorizer.cpp: Multi-look-up: !contains() + operator[] + value(), > probably use find() and friends done > * fieldfilterproxymodel.cpp: Could use the enum values/ints instead of > custom mapping through QString (c.f. Q_ENUM suggestion above) I am ambivalent about this. Yes, it makes sense to provide an enum-based filtering but then from the API POV I am restricting the API to fields that I know, yet journald allows to use individual fields names without them being defined in any specification. For now, I will keep to strings and probably add a convenience API for very well known fields. > * fieldfilterproxymodel.h: you can probably READ rowCount for > Q_PROPERTY(int count) since it has a default-argument, instead of adding > a count() that just calls rowCount() thanks, done > * fieldfilterproxymodel.h: get() seems unused? yes, it currently is, will keep it in mind before making the API public to maybe remove it > * fieldfilterproxymodel.cpp: It feels like your filterAcceptsRow() impl > is superfluous and you could be using setFilterFixedString? nice cleanup, thanks > * filtercriteriamodel.cpp: You should emit dataChanged() in setData() > when data has changed. It already is in FilterCriteriaModel::setData(...), you were probably looking at SelectionEntry::setData(...) which is an internal data class but not exposed > * journaldhelper.cpp: instead of going QString -> std::string -> c_str, > you could be doing qUtf8Printable() Nice! That method was new to me. > * journaldhelper.cpp: mapField could be using QMetaEnum to turn an enum > into a string Good idea! done > * journalduniquequerymodel.cpp: openJournalFromPath() could be reusing > the QFileInfo instance for isDir() and isFile() check done > * journalduniquequerymodel.cpp: openJournalFromPath() returns true, even > if opening fails Ouch, thanks! > * journalduniquequerymodel.cpp: it wasn't entirely clear to me what the > std::pair<.., bool> is for You find a quite unclean code fragment that needs refactoring ;) Will put it on my list. Essentially, the boolean is reused for a selected property, which is quite wrong to put here... > * journalduniquequerymodel.cpp: you probably shouldn't return true if > setData() didn't change anything? Done > * journalduniquequerymodel.cpp: selectedEntries() appears unused Nice, already one user of the above hack, that I could remove :) > > I suggest you run QAbstractItemModelTester against your models to verify > they fully behave as Qt expects. Actually, that is already in place in all autotests, just double-checked... > Some comments on the UI: > * Would be nice to have a filter bar in the "Unit" and "Process" list I > have a thousand units there, it's hard to find them. added to this issue https://invent.kde.org/system/kjournald/-/issues/22 > * There should be tooltips on elided items, e.g. when the Unit name gets > elided in the left side filter area, there are tooltips, do you mean others? > * the ItemDelegate in the bootIdComboBox should have width: parent.width done > * The scrolling behavior for the journal content is awful, you probably > want to wrap your ListView in a ScrollView for proper desktop-y mouse > wheel scrolling the scrolling itself is not the problem here, but rather the caching logic, which is needed to interact with very big logs (I have 1-2 GB logs for single boots e.g. that I have to scroll efficiently) but there are improvements possible... added to this issue https://invent.kde.org/system/kjournald/-/issues/6 > * There isn't really any keyboard navigation, e.g. I should be able to > focus the LogView and use arrow keys to go up and down an item try the page up/down keys ;) but yes, this needs to be worked on, added to this issue https://invent.kde.org/system/kjournald/-/issues/6 > * A filter feature rather than highlight would be lovely, e.g. "kde" and > find all items matching kde added to this issue https://invent.kde.org/system/kjournald/-/issues/23 > * It's not really obvious that selecting copies to clipboard, the > selection should stay, and maybe there should be a context menu added to this issue
Re: KJournald in KDE-Review
Hi,
nice work!
Here's some comments, nitpicks, etc on the code (might apply to other
files than the one mentioned, too):
* bootmodel.cpp: I don't think you need to call beginResetModel() when
you populate it in the constructor
* bootmodel.cpp: QAbstractListModel is a flat list with a single column,
no need to re-implement parent(), columnCount(), etc
* bootmodel.cpp: Admittedly it's prettier to have an Q_INVOKABLE for
bootId but if you made the roles Q_ENUM, you could call data() from QML
wiht e.g. bootModel.data(bootModel.index(row, 0), BootModel.FooRole)
* colorizer.cpp: Multi-look-up: !contains() + operator[] + value(),
probably use find() and friends
* fieldfilterproxymodel.cpp: Could use the enum values/ints instead of
custom mapping through QString (c.f. Q_ENUM suggestion above)
* fieldfilterproxymodel.h: you can probably READ rowCount for
Q_PROPERTY(int count) since it has a default-argument, instead of adding
a count() that just calls rowCount()
* fieldfilterproxymodel.h: get() seems unused?
* fieldfilterproxymodel.cpp: It feels like your filterAcceptsRow() impl
is superfluous and you could be using setFilterFixedString?
* filtercriteriamodel.cpp: You should emit dataChanged() in setData()
when data has changed.
* journaldhelper.cpp: instead of going QString -> std::string -> c_str,
you could be doing qUtf8Printable()
* journaldhelper.cpp: mapField could be using QMetaEnum to turn an enum
into a string
* journalduniquequerymodel.cpp: openJournalFromPath() could be reusing
the QFileInfo instance for isDir() and isFile() check
* journalduniquequerymodel.cpp: openJournalFromPath() returns true, even
if opening fails
* journalduniquequerymodel.cpp: it wasn't entirely clear to me what the
std::pair<.., bool> is for
* journalduniquequerymodel.cpp: you probably shouldn't return true if
setData() didn't change anything?
* journalduniquequerymodel.cpp: selectedEntries() appears unused
I suggest you run QAbstractItemModelTester against your models to verify
they fully behave as Qt expects.
Some comments on the UI:
* Would be nice to have a filter bar in the "Unit" and "Process" list I
have a thousand units there, it's hard to find them.
* There should be tooltips on elided items, e.g. when the Unit name gets
elided
* the ItemDelegate in the bootIdComboBox should have width: parent.width
* The scrolling behavior for the journal content is awful, you probably
want to wrap your ListView in a ScrollView for proper desktop-y mouse
wheel scrolling
* There isn't really any keyboard navigation, e.g. I should be able to
focus the LogView and use arrow keys to go up and down an item
* A filter feature rather than highlight would be lovely, e.g. "kde" and
find all items matching kde
* It's not really obvious that selecting copies to clipboard, the
selection should stay, and maybe there should be a context menu
* I somehow managed to break the LogView in a way that I couldn't scroll
anymore using the arrow buttons or mouse wheel, just using the
scrollbar, couldn't reproduce, though
* Global menu doesn't use title capitalization. You probably want to add
i18nc("@action:inmenu", "..."), too
* The options in the "View" menu should be using radio buttons
* KAboutData is a Q_GADGET, you should be able to expose that to QML
without a proxy class
* For FlattenedFilterCriteriaProxyModel you might want to check out
KDescendantsProxyModel which is for turning a tree into a flat list, and
combine that with DelegateModel to pick a specific rootIndex for
displaying only a certain tree branch
Cheers
Kai Uwe
Re: KJournald in KDE-Review
On Sonntag, 23. Oktober 2022 10:34:36 CET Andreas Cord-Landwehr wrote: > Hi, just a short update: Thank you for the comments! Nearly all of them are > already fixed. For a very few I created invent.k.o issues instead, because > they are mostly a maintainability concern and make sense to fix together > with planned features (e.g. removal of fixed colors when refactoring > integration with desktop color scheme). > If you have more comments please raise them. In case nobody finds a blocker, > I will ask sysadmins at the end of the week to move kjournald to the > "system" module. Hi, kjournald now transitioned into the system module. Thanks to all! Andreas
Re: KJournald in KDE-Review
Hi, just a short update: Thank you for the comments! Nearly all of them are already fixed. For a very few I created invent.k.o issues instead, because they are mostly a maintainability concern and make sense to fix together with planned features (e.g. removal of fixed colors when refactoring integration with desktop color scheme). If you have more comments please raise them. In case nobody finds a blocker, I will ask sysadmins at the end of the week to move kjournald to the "system" module. Cheers, Andreas
Re: KJournald in KDE-Review
On Mittwoch, 12. Oktober 2022 03:56:03 CEST Aleix Pol wrote: [...] > Then I'd recommend to only make it public API when the time comes. :) > > +1 to making it optional instead, if it makes your life any better. > That said, when we've needed this kind of flexibility in the past, we > have often ended up splitting the repository into a separate one, not > sure it would make sense here. > > Also it would be useful to know what your plans are. :D Hi, just added such an option. My goal is to eventually split the library part out from the application as soon as it makes sense. At the moment there are still sometimes changes to the library API/moving parts that are not fully polished yet. The full story is that I started with the library, then noticed that my reference app to show how the API can be used has a lot of value as its own. So the focus right now is to make something for end users and later on to look at finalizing the library abstraction. Cheers, Andreas
Re: KJournald in KDE-Review
On Tue, Oct 11, 2022 at 9:47 PM Andreas Cord-Landwehr wrote: > > On Montag, 10. Oktober 2022 15:34:30 CEST Aleix Pol wrote: > > On Sun, Oct 9, 2022 at 7:18 PM Andreas Cord-Landwehr > [...] > > > 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! > > > > I'm a bit confused by the context. > > > > It's placed in libraries although it seems like system would be the > > place for it. > > https://invent.kde.org/system > > > > I see there's a library, are you planning on maintaining compatibility > > there? Are there other users? If there's no users outside, it could > > make sense to skip installing headers until there is a use-case, > > otherwise you might see yourself tied to an ABI unnecessarily. > > Yes, the https://invent.kde.org/system module sounds actually to be the best > place for the app; I wrote "utilities" in the original mail, but agree that > system is even better. > > I myself have use cases to use the library part as a library. Yet that is not > for a public project. I could introduce a build option to only install headers > optionally (default being OFF), which might makes it more clear to packagers > that currently they do not have to package the headers. Moreover, at the > moment their is not yet a fully stable API for the library, which is probably > another good reason to not install headers by default. > > In the mid-term/long-run, I am planning to provide a stable API for the > library. Then I'd recommend to only make it public API when the time comes. :) +1 to making it optional instead, if it makes your life any better. That said, when we've needed this kind of flexibility in the past, we have often ended up splitting the repository into a separate one, not sure it would make sense here. Also it would be useful to know what your plans are. :D Aleix
Re: KJournald in KDE-Review
On Montag, 10. Oktober 2022 15:34:30 CEST Aleix Pol wrote: > On Sun, Oct 9, 2022 at 7:18 PM Andreas Cord-Landwehr [...] > > 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! > > I'm a bit confused by the context. > > It's placed in libraries although it seems like system would be the > place for it. > https://invent.kde.org/system > > I see there's a library, are you planning on maintaining compatibility > there? Are there other users? If there's no users outside, it could > make sense to skip installing headers until there is a use-case, > otherwise you might see yourself tied to an ABI unnecessarily. Yes, the https://invent.kde.org/system module sounds actually to be the best place for the app; I wrote "utilities" in the original mail, but agree that system is even better. I myself have use cases to use the library part as a library. Yet that is not for a public project. I could introduce a build option to only install headers optionally (default being OFF), which might makes it more clear to packagers that currently they do not have to package the headers. Moreover, at the moment their is not yet a fully stable API for the library, which is probably another good reason to not install headers by default. In the mid-term/long-run, I am planning to provide a stable API for the library. Cheers, Andreas
Re: KJournald in KDE-Review
On Sun, Oct 9, 2022 at 7:18 PM Andreas Cord-Landwehr wrote: > > 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! I'm a bit confused by the context. It's placed in libraries although it seems like system would be the place for it. https://invent.kde.org/system I see there's a library, are you planning on maintaining compatibility there? Are there other users? If there's no users outside, it could make sense to skip installing headers until there is a use-case, otherwise you might see yourself tied to an ABI unnecessarily. Aleix
Re: KJournald in KDE-Review
Neat!
- clazy has some hints
- you could probably use
https://api.kde.org/ecm/module/ECMQtDeclareLoggingCategory.html for
loggincategory.cpp/h?
- for qml you should probably use Kirigami.Units.gridUnit*N instead of
hardcoding sizes
- the hardcoded colors also seem a bit of an odd choice
- they are done already but for future reference you might want to
prefer i18nc over i18n, in particular for single word string such as
`i18n("Url:")` (which btw is also incorrectly capitalized)
- talking about... TopMenuBar doesn't consistently use Title Capitalization
- may be of interest to shove the raw journal pointer into a
unique_ptr asap:
https://invent.kde.org/plasma/drkonqi/-/blob/master/src/coredump/memory.h
On Sun, Oct 9, 2022 at 7:18 PM Andreas Cord-Landwehr
wrote:
>
> 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
>
>
Re: KJournald in KDE-Review
El diumenge, 9 d’octubre de 2022, a les 19:18:15 (CEST), Andreas Cord-Landwehr va escriure: > 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! automoc complains a bit about some properties https://pst.klgrth.io/paste/kf9ta If you're planning to use those from QML maybe a Q_INVOKABLE is better? You need to use -DTRANSLATION_DOMAIN on your library (i understand it's supposed to be usable by third party apps) valgrind is reporting a massive memory leak, you need to free the char* returned by sd_journal_get_cursor Cheers, Albert > > Best regards, > Andreas
KJournald in KDE-Review
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
