export org.kde.appmenuview
ClosedPublic

Authored by mart on Dec 5 2017, 8:42 PM.

Details

Summary

drop the global settings and export the service wich will activate
the appmenu kded

Test Plan

adding the menu button makes new apps export it,
removing it makes new apps using the interlal one again

Diff Detail

Repository
R108 KWin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mart created this revision.Dec 5 2017, 8:42 PM
Restricted Application added a project: KWin. · View Herald TranscriptDec 5 2017, 8:42 PM
Restricted Application added subscribers: kwin, plasma-devel. · View Herald Transcript
mart requested review of this revision.Dec 5 2017, 8:42 PM
mart planned changes to this revision.Dec 5 2017, 8:43 PM

WIP

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 5 2017, 8:43 PM
broulik added a subscriber: broulik.Dec 5 2017, 8:45 PM

Not sure. Imho the button should be there by default and shown if global menu is enabled in settings. I wouldn't want people manually having to fiddle that title bar menu button in there in order for global menu to work let alone suddenly having it use it once the button is there (update case)

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 5 2017, 8:45 PM

Imho the button should be there by default and shown if global menu is enabled in settings

With that approach:

  • users have an icon in the previews they can't see on their titlebar, which is weird.
  • users of the applet still have to do two steps
  • we still have that complex 4 possible options of having a button and the setting being enabled.

From a technical POV, I much prefer this. Especially on the Applet version of this patch.

From a UI POV, there's nothing to stop the old KCM setting remaining and instead manipulating the kwin buttons / running a plasma script.
IMHO it's not worth it, it's a far less intuitive place to look, but it's not something I'd object to either.


As for this patch, code here is fine, but there's stuff that could then be removed.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 5 2017, 9:55 PM

I'm not saying the current way it's to be configured is good, it's not, it's terrible. I really like the applet approach indeed. Mostly I'm concerned about what'll happen to existing setups when this lands, I certainly don't want appmenu to suddenly be enabled just because the appmenu button is present on the title bar (I made it the default).

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 5 2017, 9:59 PM

(I made it the default).

You half made it the default

It's the default in kwin, it's not in the KCM default.

Which means unless a user edited this, it's not in their config unless they explicitly added it.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 5 2017, 10:17 PM

Which means unless a user edited this, it's not in their config unless they explicitly added it.

If that is so, then I'm all for it \o/

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 5 2017, 10:28 PM
davidedmundson requested changes to this revision.Dec 7 2017, 2:53 PM

We're all happy with the concept then. \o/

appmenu.cpp
71

All this needs fixing.
(and probably other related methods)

decorations/settings.cpp
158–159

Remove this please.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 7 2017, 2:53 PM
mart added inline comments.Dec 8 2017, 11:24 AM
decorations/settings.cpp
158–159

to make it not default anymore you mean?

Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 8 2017, 11:24 AM
davidedmundson added inline comments.Dec 8 2017, 11:46 AM
decorations/settings.cpp
158–159

Yes please.
Otherwise you'd be adding it to everyone's kwin automatically.

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 8 2017, 11:46 AM
mart updated this revision to Diff 23680.Dec 9 2017, 12:30 PM
  • make enabled depend from the dbus service presence
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 9 2017, 12:30 PM
mart marked 4 inline comments as done.Dec 9 2017, 12:31 PM
mart added inline comments.
appmenu.cpp
71

value depends from appmenu service existing now, config values dropped

Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 9 2017, 12:31 PM
ngraham added a subscriber: ngraham.Dec 9 2017, 1:45 PM
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 9 2017, 1:45 PM
davidedmundson accepted this revision.Dec 12 2017, 9:24 AM
This revision is now accepted and ready to land.Dec 12 2017, 9:24 AM
Restricted Application edited projects, added Plasma; removed KWin. · View Herald TranscriptDec 12 2017, 9:24 AM
This revision was automatically updated to reflect the committed changes.
mart marked an inline comment as done.
Restricted Application edited projects, added KWin; removed Plasma. · View Herald TranscriptDec 12 2017, 2:06 PM