KJournald in KDE-Review

[Andreas Cord-Landwehr]

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.