| cfeck | |
| dfaure |
| Frameworks |
BUG: 245637
FIXED-IN: 5.70
The kmimetypechoosertest app still works
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
Nice work. But can you give the reasoning for removing the comment column? For non-english speakers this might be the only thing that's actually readable?
I'm also curious about the use of QStandardItemModel which I consider to be at best a class for unittesting proxies and views. Just like QTreeWidgetItem it stores the data.
Of course compared to QTreeWidgetItem it's one step into the "right" direction of item/view separation, so why not, but it's not exactly more efficient.
| src/kmimetypechooser.cpp | ||
|---|---|---|
| 44 | In my experience a vector of const pointers is usually a pain to work with, e.g. for find(). But you're lucky here it seems, it doesn't create trouble ;) | |
| 417 | QFontMetrics fm(fontMetrics()); | |
I thought that adding the "comment" as a tooltip makes sure it's always available if the dialog is shown with the flag for the comment column unset... but looking on lxr.kde all the usage of KMimeTypeChooserDialog always show the comments column; so that idea is redundant.
A tooltip isn't good enough here? I think the mimetype name and extension are more recognisable/useful than the comment; especially since the comment sometimes isn't that useful:
AMR AMR Audio
troff Troff document
x-base32 Base32 encoded data
x-rpm-spec RPM spec file
but I haven't set in lines in the sand about that column, bringing it back should be easy.
That answers a question that kept popping in my mind while working on this, whether to try sub-class'ing QAbstractItemModel, or just use QStandardItemModel; basically whether one is better than the other. Certainly the documentation that I read so far doesn't tell me exactly which way to go. Of course I noticed lots of sub-classing of QAbstractItemModel in KDE, but it seemed easier (less work :)) to use an existing model rather than sub-classing.
So what you're saying here is that sub-classing QAbstractItemModel is better; I'll try that, but I am curious about why a sub-class is better than using QStandardItemModel (are there any docs/blogs... etc about that?)
| src/kmimetypechooser.cpp | ||
|---|---|---|
| 44 | A vector of non-const pointers is better in that regard? | |
I don't know. I don't claim to be a UI expert.
But for instance your lineedit could filter on the comment as well, to help the user locate a mimetype based on a keyword in its description. If it's "hidden" in a tooltip then that doesn't really work.
This allows user to use words in their own language for that search (e.g. "lecteur audio" when looking for x-content/audio-player)
Or imagine looking for "3D Studio" files for instance -- but I can see how finding by extension is the most useful way, users know those are called *.3ds.
but I haven't set in lines in the sand about that column, bringing it back should be easy.
I think filtering *is* a great reason to bring it back.
it seemed easier (less work :)) to use an existing model rather than sub-classing.
Certainly.
So what you're saying here is that sub-classing QAbstractItemModel is better; I'll try that, but I am curious about why a sub-class is better than using QStandardItemModel (are there any docs/blogs... etc about that?)
I'm used to explaining it at length as a professional Qt trainer, but I don't really know all about online resources ;)
Yes, for anything above trivial sizes (like 10 items), subclassing QAbstractItemModel is generally better.
With QStandardItemModel (or Q*WidgetItem) you're *copying* the data into the model. In general that data is then twice in memory, once in the underlying application-specific data structures and once in the model. QStrings are cheap to copy, but when thinking about non-readonly models, it also means the two things need to be kept in sync.
Here the situation is a bit different, though. First it's readonly, and second, the data comes from a temporary QMimeDatabase instance (which reads and converts from a mmaped file, it doesn't *store* QStrings in memory). So the underlying data actually goes away, we don't actually have real duplication in memory.
Therefore this is one case where I cannot argue against QStandardItemModel ;)
It is just slightly more heavy (map of variants) than a simple model that would store 4 strings per node.
But the amount of code required for a custom tree model is, I think, not worth it here.
Let's go with QStandardItemModel. This dialog doesn't even stay in memory very long anyway.