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 https://invent.kde.org/system/kjournald/-/issues/7 > * 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 I will keep my eyes open if I can reproduce this :/ > * Global menu doesn't use title capitalization. You probably want to add > i18nc("@action:inmenu", "..."), too Will clean them up, thanks! > * The options in the "View" menu should be using radio buttons done > * KAboutData is a Q_GADGET, you should be able to expose that to QML > without a proxy class 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. E.g. using qmlRegisterSingletionInstance on the KAboutData object will not work because it is just a gadget... > * 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 Yeah, I was told just after I did my implementation of essentially the same logic. Right now, what I have works... But this is something where I will look into to switch to a shared implementation of the mechanism.