Details
Diff Detail
- Repository
- R120 Plasma Workspace
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
FYI, as maintainer of Kicker, I've reverted this commit in master until this review request is updated with additional info on what it's useful for. Please don't move patches through in <15 minutes without people having a chance to weigh in.
Rootmodel inherits appmodel.
Appmodel has a property flat. Therefore this does
It wasnt supported returning a non flat tree.
I do normally wait for maintainers on any new features or big changes, but this seems like a straightforward bug fix?
You got a point, but there's a higher-level thing with the Kicker backend in terms of where it came from and where it's going:
- Originally it was created specifically for the Kicker menu with the backend custom-designed to that UI. AppsModel is fairly generic (good), and the RootModel subclass exists almost solely to create the specific menu structure that the Kicker UI uses (it's the ugly wart in terms of this being a generic import, which it's increasingly used at)
- Eventually we grew other UIs and RootModel keeps growing additional feature and mode knobs to customize it for them and it's getting rather sprawling
I'd like to take the Kicker backend into a direction where it's more composable from QML to create different UIs rather than the equivalent of adding checkboxes to RootModel.
In particular I think there's an opportunity to take a step towards that with this specific use case, which I'm guessing is to just get one flat list of apps, right?
Namely I'd suggest:
- AppsModel is already exposed to QML via the plugin and already implements QQmlParserStatus
- Add a parameter-free constructor to AppsModel so it can be instanciated directly
- Make entryPath a Q_PROPERTY to AppsModel and default it to empty (root level)
Then whatever UI this is for can just do AppsModel { flat: true } and avoid all of the unnecessary stuff in RootModel (like setting tons of props to turn off features) and it's cleaner and a bit lighter.
I know that "please code my vision instead" in a patch review is a bit meh, but the resulting patch here would probably have a lower line count and the result's more useful down the line.
m_flat currently behaves a bit specially. It doesn't affect the toplevel, it only affects sub models created within it.
If we used AppsModel directly to fix the original bug, we would need to fix AppsModel to follow m_flat when showing an empty m_entryPath
So in addition to a property for entry path we would add something like:
AppsModel.cpp:481 - if (m_entryPath.isEmpty()) + if (m_entryPath.isEmpty() && !m_flat) { (bringing us into the else path that uses processServiceGroup which follows m_flat)
Which fixes the bug giving us a flat AppsModel. But it now that introduces a regression into the dash that's currently in kdeplasma-addons.
We're now implicitly affecting the RootModel top level to be flat, meaning the sidebar on the right hand side now lists every application not just groups - which obviously isn't OK.
We would need to introduce a property "flatButForRealThisTime" property on AppsModel which IMHO is going in a messy direction compared to only affecting allModel.
It's worth noting that this patch as-is also breaks the Dashboard UI ("All Applications" no longer shows content), so this revert wasn't just about implementation details but also actively broke upstream.
I'm writing an alternative patch now.