Having everyone who links to KDevPlatformUtil (which is almost everything
not only via public link interface of KDevPlatformProject) also link
to KF5::ItemModels, just because of a single enum which also is used
currently only from a single plugin, is worth to avoid, by the price
of a small enum-matching wrapper method.
Details
Details
- Reviewers
kfunk brauch - Group Reviewers
KDevelop - Commits
- R32:76d67d11fb71: Use custom enum MLLV::LastLevelViewMode, not raw KSPM::FilterBehavior
Builds as before.
Diff Detail
Diff Detail
- Repository
- R32 KDevelop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
| kdevplatform/util/multilevellistview.cpp | ||
|---|---|---|
| 458 | That dynamic_cast without testing the result deserves a separate fixing commit, possibly with global check for similar ones. Noted, so you do not have to :) | |
Comment Actions
To be honest, I'm not sure I like this change. It's adding code which we need to maintain (what if the KSelectionProxyModel::FilterBehavior enum is extended?) for very, very little gain.
I'd propose to just abandon this change, sorry.
| kdevplatform/util/multilevellistview.cpp | ||
|---|---|---|
| 458 | Right, d->proxies.last() can potentially be an instance not inheriting KSelectionProxyModel (iiuc when passing 1 to MultiLevelListView::setLevels(...)). Someone should check this code :) | |
| kdevplatform/util/multilevellistview.h | ||
| 46 | It's not an implementation detail if we pass the exact same value through to KSelectionProxyModel. | |
Comment Actions
Yeah, sorry, I'm also against this. Linking an extra lib we depend on anyways is a problem a compuer has to deal with, extra code is a problem humans have to deal with. The former wins against the latter unless there is a very good reason why not.
Comment Actions
Since MultiLevelListView::FilterBehavior is translated into KSelectionProxyModel::FilterBehavior, there should be no problem if the latter is extended. Of course one could think about also extending the former then, but I don't see something terrible happen if we don't.
My understanding isn't very deep here, but maybe we wouldn't want to blindly allow all values of KSelectionProxyModel::FilterBehavior? Say if there is a new value added, but we don't want to allow it, how would we do that? Right now we're enabling KSelectionProxyModel to extend the interface of MultiLevelListView, which doesn't exactly follow the OOP doctrine. I'm not saying this is necessarily a bad thing, but it deserves some further consideration.
Comment Actions
@brauch @kfunk I see your point in general. Just allow me to represent the case, initial patch was too much focussed on removing the lib dep
The old API of MultiLevelListView was using the raw KSelectionProxyModel::FilterBehavior enum in the API, as also used in the actual implementation.
Just, most values of that enum do not make sense with the idea of MultiLevelListView, actually it is just two of them, matching two different modes:
- showing complete subtrees in the view of the last level
- showing just the children of that level
The updated patch now also improves the API of MultiLevelListView to describe the actual design of the class, instead of being driven by implementation details as before.
Like @aaronpuchert said, here it is just a one directional mapping to a subset of the enum. That a KSelectionProxyModel is used behind the scenes is completely irrelevant to the API of MultiLevelListView itself, actually only confuses the API consumer if exposed.
Does it make more sense to you now? I should have done the API improvement already in the initial patch, blame me for being lazy here.
Comment Actions