KJournald in KDE-Review

[Kai Uwe Broulik]

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