Take in consideration flat() for the All Applications menu
ClosedPublic

Authored by tcanabrava on Jul 26 2019, 1:38 PM.

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.
tcanabrava created this revision.Jul 26 2019, 1:38 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJul 26 2019, 1:38 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
tcanabrava requested review of this revision.Jul 26 2019, 1:38 PM
davidedmundson accepted this revision.Jul 26 2019, 1:43 PM
This revision is now accepted and ready to land.Jul 26 2019, 1:43 PM
This revision was automatically updated to reflect the committed changes.
hein added a subscriber: hein.Jul 27 2019, 12:50 PM

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.

hein reopened this revision.Jul 27 2019, 12:50 PM
This revision is now accepted and ready to land.Jul 27 2019, 12:50 PM
hein requested changes to this revision.Jul 27 2019, 12:50 PM
This revision now requires changes to proceed.Jul 27 2019, 12:50 PM

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?

hein added a comment.EditedJul 27 2019, 1:20 PM

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.

@hein I'll try to follow your approach.

hein added a comment.Aug 2 2019, 3:52 PM

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.

This revision was not accepted when it landed; it landed in state Needs Revision.Sep 16 2019, 12:17 PM
This revision was automatically updated to reflect the committed changes.