[kcmkwin/kwindecoration] Rewrite the KWin decorations settings as a ConfigModule
ClosedPublic

Authored by vpilo on Tue, Jan 22, 5:27 PM.

Details

Summary
  • Wrote new KCM based on KQuickAddons::ConfigModule.
  • Remade QMLs for Buttons and Themes tabs.
  • Updated bridge model code for new plugin lookup API (fixes warnings).
  • Fixed decoration shadow changing messing with the previews sizes.
  • Fixed button drag and drop issues (see D18104).
  • Fixed default settings button behavior and detection of settings changes.
  • Updated Get Hot New Stuff.
  • Removed apply button in previewbridge.cpp: After applying changes, a theme's KCModule is invalidated.

BUG: 389431
BUG: 350122
BUG: 346222
BUG: 342816
BUG: 397595

|
|
|

Test Plan
  • Verified saving and loading for every setting
  • Checked shadows of Breeze and Oxygen
  • Tested all possible drag&drop operations on both sides of the fake titlebar
  • Changed color schemes (with kcmshell5 colors) while showing the Themes tab to see if all previews update correctly their palettes
  • Tested on a fresh Neon-developer account, via kcmshell and systemsettings

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes

Can you give me a summary of whatever's left that I need to help investigate?

I've commented on the tabs on the bug thread and I've fixed the combobox issues today.

zzag added a subscriber: zzag.Tue, Jan 29, 6:09 PM
zzag added inline comments.
kcmkwin/kwindecoration/declarative-plugin/previewbridge.cpp
133

In KWin, we tend to use constBegin/constEnd.

kcmkwin/kwindecoration/declarative-plugin/previewbutton.cpp
135–145

Doesn't QQuickItem have size() method?

kcmkwin/kwindecoration/declarative-plugin/previewbutton.h
62

const QColor &color

kcmkwin/kwindecoration/decorationmodel.cpp
162

Unrelated whitespace change.

173

It seems like that's unrelated change.

kcmkwin/kwindecoration/decorationmodel.h
36

Can't we start from Qt::UserRole +1?

kcmkwin/kwindecoration/kcm.cpp
52

Given that this is anonymous namespace you don't need static, also please don't indent contents of the namespace.

89–122

Use auto.

kcmkwin/kwindecoration/kcm.h
41

Please don't indent content of the namespace.

48–49

Same here.

kcmkwin/kwindecoration/utils.cpp
25

Function implementations, class, struct and namespace declarations always have the opening brace on the start of a line.

26–27

We don't need static. Same comments about indentation.

kcmkwin/kwindecoration/utils.h
30

Please use namespace instead.

vpilo updated this revision to Diff 50510.Tue, Jan 29, 7:07 PM
vpilo marked 14 inline comments as done.
  • Review comments (code, strings, translations)
vpilo updated this revision to Diff 50511.Tue, Jan 29, 7:09 PM
  • Mistakenly removed a const before commit, replaced
vpilo added a comment.Tue, Jan 29, 7:10 PM

OK. As I thought then,

In D18458#401828, @GB_2 wrote:

Looks great!
The close button is a bit hard to see though.

I don't know how else to tint these icons to the theme color without messing with their shapes. I welcome suggestions :D

kcmkwin/kwindecoration/declarative-plugin/previewbutton.cpp
135–145

Everything else can take/return real, but DecorationButton::paint() can only take a QReal so I thought to reduce conversions this way

kcmkwin/kwindecoration/decorationmodel.cpp
173

it's not, findDecoration() below might fail to find a theme because it only searches for themeName and not on visibleName

kcmkwin/kwindecoration/decorationmodel.h
36

It came all the way from 2014 somehow.. I'm not nostalgic. Done

vpilo added a comment.Tue, Jan 29, 7:14 PM

Can you give me a summary of whatever's left that I need to help investigate?

I've commented on the tabs on the bug thread and I've fixed the combobox issues today.

Thank you!
If you check on main.qml I had to add a couple workarounds. I cannot figure out why the main SimpleKCM item doesn't take all the available space and only that ; it either takes too much (and the KCM gets an useless scrollbar) or too little (and you get the empty space as in the comment screenshot above).

By checking in gammaray, it looks like it's an item inside the SimpleKCM hierarchy that is somehow not expanding. Or am I doing something wrong in my layouts?

zzag added inline comments.Tue, Jan 29, 7:25 PM
kcmkwin/kwindecoration/declarative-plugin/previewbridge.cpp
133

This isn't done, below cend is used.

kcmkwin/kwindecoration/declarative-plugin/previewbutton.h
62

This isn't done.

kcmkwin/kwindecoration/decorationmodel.cpp
173

Introduction of name variable is unrelated change. Given that info is a const ref, I think we can do

data.visibleName = info.name().isEmpty() ? info.pluginName() : info.name();
data.themeName = data.visibleName;
kcmkwin/kwindecoration/decorationmodel.h
36

We don't need +2, and +3.

kcmkwin/kwindecoration/kcm.cpp
90–93

Please align it.

zzag added inline comments.Tue, Jan 29, 7:32 PM
kcmkwin/kwindecoration/decorationmodel.h
36
enum DecoratinoRole {
    PluginNameRole = Qt::UserRole + 1,
    ThemeNameRole,
    ConfigurationRole
};
vpilo updated this revision to Diff 50517.Tue, Jan 29, 9:37 PM
vpilo marked 5 inline comments as done.
  • More review comments
vpilo added inline comments.Tue, Jan 29, 9:37 PM
kcmkwin/kwindecoration/decorationmodel.cpp
173

Ah right- it only made sense before I ditched the GHNS id->name data map.

I don't know how else to tint these icons to the theme color without messing with their shapes. I welcome suggestions :D

If we can make the titlebar and buttons on that tab look exactly like they do in the theme itself, that would seem to present a solution to the issue. Then but roundrects behind the draggable buttons on bottom could just use the titlebar color.

vpilo added a comment.Wed, Jan 30, 7:23 AM

I don't know how else to tint these icons to the theme color without messing with their shapes. I welcome suggestions :D

If we can make the titlebar and buttons on that tab look exactly like they do in the theme itself, that would seem to present a solution to the issue. Then but roundrects behind the draggable buttons on bottom could just use the titlebar color.

It's really messy to have that. I tried and it would require either:

  1. allowing access to the faux titlebar button models (but I think the biggest problem would be matching their actual location when trying to drag them in and out), or
  2. duplicating the entire thing, using the current system without actually painting anything, and have an actual decoration drawn underneath. And here, being sure the current model matches the deco underneath would be quite hard.

Also I am not sure whether it would actually be worth taking all that time implementing such solutions, when this one has just a slight contrast issue. For sure I would not do all that as part of this change, to not let it drag along too long.

davidedmundson commandeered this revision.Wed, Jan 30, 5:35 PM
davidedmundson edited reviewers, added: vpilo; removed: davidedmundson.

Update main page layout.

Killer difference is that we no longer nest flickables.

I just commandeered to push the layout change, feel free to take it back.

TabBar still needs some work, but on the QQC-desktop-theme side.

And there's some problems with the outside border, it should line up with the left edge of the "help" button, it does in kcmshell not in systemsettings.
I fear that's the declarative KCM embedding which ended up quite convoluted :/

Implicit size

vpilo commandeered this revision.Wed, Jan 30, 11:33 PM
vpilo edited reviewers, added: davidedmundson; removed: vpilo.
vpilo updated this revision to Diff 50584.Thu, Jan 31, 1:07 AM

Fixed the padding to look ok in the SystemSettings layout

vpilo added a comment.Thu, Jan 31, 1:11 AM

TabBar still needs some work, but on the QQC-desktop-theme side.

And there's some problems with the outside border, it should line up with the left edge of the "help" button, it does in kcmshell not in systemsettings.
I fear that's the declarative KCM embedding which ended up quite convoluted :/

Now I think that SimpleKCM needs to be remade like GridKCM is. Or that all *KCM items should inherit from SimpleKCM. This way we can be sure they will all look the same.

This version at least behaves fine in systemsettings5, but the borders are not right in kcmshell. I think the only one that matters is systemsettings, right?

vpilo added a comment.Mon, Feb 4, 11:05 PM

@davidedmundson, @ngraham, @zzag, @GB_2, @broulik :

This is the current look in SystemSettings. Comments?

In KCMShell the margins are 4px too wide (but who uses kcmshell anyway?).

Is this patch acceptable while we work out another fix, likely on the KCM QML components side?

(but who uses kcmshell anyway?).

Kwin.

Right click on title bar -> window manager settings

Is this patch acceptable

In general, yes.

while we work out another fix,

Lets just fix it together whilst we're still thinking about it.
Try this: https://phabricator.kde.org/D18739


Or that all *KCM items should inherit from SimpleKCM

We don't want to inherit from SimpleKCM.

SimpleKCM adds a scrollbar. It's not claiming it's a simple class, it's for use when the contents are simple.

In GridViewKCM only the grid scrolls, so we don't need scrollbars round the rest.

vpilo added a comment.Thu, Feb 7, 12:04 PM

Is this patch acceptable

In general, yes.

while we work out another fix,

Lets just fix it together whilst we're still thinking about it.
Try this: https://phabricator.kde.org/D18739


Or that all *KCM items should inherit from SimpleKCM

We don't want to inherit from SimpleKCM.

SimpleKCM adds a scrollbar. It's not claiming it's a simple class, it's for use when the contents are simple.

In GridViewKCM only the grid scrolls, so we don't need scrollbars round the rest.

@davidedmundson - I tried that fix, and removed the paddings from this patch's main.qml. The result is that both on KCMShell and SystemSettings I can see an extra 6 pixel margin around the main Page. In other words, the Frame's sides have now exactly 6 pixels of padding vs the buttons below (Apply, Defaults, Reset).

and removed the paddings from this patch's main.qml.

You would still need this.
The point is that it removes the margins that kcmshell5 adds so that the module can add its own. (which for some reason is the state on system settings)

vpilo added a comment.Thu, Feb 7, 3:05 PM

and removed the paddings from this patch's main.qml.

You would still need this.
The point is that it removes the margins that kcmshell5 adds so that the module can add its own. (which for some reason is the state on system settings)

I tried again and noticed my changes weren't being applied. I cleaned up and tried again with (and without) the padding. With your change D18739, this KCM is now properly padded like the other KCM modules.

vpilo edited the summary of this revision. (Show Details)Thu, Feb 7, 3:07 PM

Maybe change the subject line to start with [kcmkwin/kwindecoration] though we don't have a very good policy on this.

two minor comments left, and then I think we're good to ship this

kcmkwin/kwindecoration/package/contents/ui/main.qml
77

context /should/ be set automagically

And surely it would be kcmkwindecoration not kcm_kwindecoration ?

110

onToggled

vpilo updated this revision to Diff 51105.Thu, Feb 7, 3:32 PM
vpilo marked 2 inline comments as done.
  • Review comments
vpilo added inline comments.Thu, Feb 7, 3:32 PM
kcmkwin/kwindecoration/package/contents/ui/main.qml
77

I'll just remove that, then. I used the name of the library as context.

vpilo retitled this revision from Rewrite the KWin decorations settings as a ConfigModule. to [kcmkwin/kwindecoration] Rewrite the KWin decorations settings as a ConfigModule..Thu, Feb 7, 3:35 PM
zzag retitled this revision from [kcmkwin/kwindecoration] Rewrite the KWin decorations settings as a ConfigModule. to [kcmkwin/kwindecoration] Rewrite the KWin decorations settings as a ConfigModule.Thu, Feb 7, 3:36 PM
vpilo edited the summary of this revision. (Show Details)Fri, Feb 8, 8:05 AM
vpilo edited the test plan for this revision. (Show Details)

Let me know if it's now good :)

davidedmundson accepted this revision.Sun, Feb 10, 11:58 AM
This revision is now accepted and ready to land.Sun, Feb 10, 11:58 AM
This revision was automatically updated to reflect the committed changes.

With this new KCM, when I download new window decorations using the GHNS button, they never appear in the KCM for me. Can anyone else confirm?

Also the assortment of window decorations presented in the GHNS dialog is different (and much more limited) than it was before, but I'm not sure if that's related.

vpilo added a comment.Sun, Feb 10, 7:53 PM

With this new KCM, when I download new window decorations using the GHNS button, they never appear in the KCM for me. Can anyone else confirm?

Also the assortment of window decorations presented in the GHNS dialog is different (and much more limited) than it was before, but I'm not sure if that's related.

I was discussing this at the beginning of the review; the new GHNS APIs don't allow you to dynamically add more categories. Only those hardcoded in the .knsrc are used.
All mentions of category adding in the APIs are for filtering between the already selected categories.

ngraham added a subscriber: leinir.Sun, Feb 10, 8:14 PM

So what do we do? Right now it's a pretty significant regression to not show GHNS-downloaded decorations. We still have a lot of time before Plasma 5.16 ships, but we should really fix it before them. @leinir is our GHNS/store.kde.org expert here; maybe we need a meeting-of-the-minds on this?

Many lines without translation.

victorr added a comment.EditedMon, Feb 11, 2:33 AM

If you apply a patch

--auto about = new KAboutData(QStringLiteral("kcm_kwindecoration"),
+auto about = new KAboutData(QStringLiteral("kcmkwindecoration"),

There will be such a window

victorr added a comment.EditedMon, Feb 11, 2:36 AM

If you apply this patch, the translation works.

vpilo added a comment.EditedMon, Feb 11, 7:30 PM

If you apply this patch, the translation works.

Thank you! I'll commit it today. I don't think it needs a review, does it @ngraham / @davidedmundson ?

So what do we do? Right now it's a pretty significant regression to not show GHNS-downloaded decorations. We still have a lot of time before Plasma 5.16 ships, but we should really fix it before them. @leinir is our GHNS/store.kde.org expert here; maybe we need a meeting-of-the-minds on this?

the new GNS implementation is confusing. I thought I followed the apis, and in my tests it was working. But I now built kwin in a neon-developer VM, and I get issues..

It looks like the installation directory is somehow wrong: on install I now get "Wrong number of installation directories given". The next time I reopen systemsettings, none of the downloaded themes are marked as installed anymore. I'll make some more tests and submit another review.

By the way, is there a commented example knsrc file somewhere? I found in the source tree a couple of 'em with some comments but they're from kns2

I opened D18935.

Aurorae decorations are OK - I think I got the category wrong the last time I changed them during the review.