[kcmkwin/kwindecoration] Load KCM decorations properly
AbandonedPublic

Authored by vpilo on Jan 7 2019, 11:27 PM.

Details

Summary

BUG: 397595
FIXED-IN: 5.15

The UI expects all decorations in the model to have a 'theme name'
which wasn't always set.

Test Plan

With both kcmshell and systemsettings5, switched between all themes in the KCM.
Each time saving the changes and then closing/reopening the application altogether.

Diff Detail

Repository
R108 KWin
Branch
arcpatch-D18084
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 6892
Build 6910: arc lint + arc unit
vpilo created this revision.Jan 7 2019, 11:27 PM
Restricted Application added a project: KWin. · View Herald TranscriptJan 7 2019, 11:27 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
vpilo requested review of this revision.Jan 7 2019, 11:27 PM
vpilo added a comment.Jan 7 2019, 11:33 PM

@ngraham, I reproduced your test environment (made a Neon Developer VM) and got the same behavior: I didn't have Breeze compiled by kdesrc-build, only via packages.
After building it, it became more difficult to reproduce.
This fixed it in all cases for me.

Added @davidedmundson since I'm not 100% sure how some themes have metadata and some don't, and I might be using themeName incorrectly.

mart accepted this revision.Jan 8 2019, 9:32 AM
This revision is now accepted and ready to land.Jan 8 2019, 9:32 AM
ngraham requested changes to this revision.Jan 8 2019, 11:59 PM

I'm afraid this still doesn't work for me. I still see the current theme selected only when running kcmshell5 kwindecoration, but not when browsing to the page in System Settings. :(

This revision now requires changes to proceed.Jan 8 2019, 11:59 PM
vpilo updated this revision to Diff 49123.Jan 9 2019, 11:49 PM
  • When the list is available, select a decoration immediately. Also improve finding decorations
vpilo added a comment.Jan 9 2019, 11:50 PM

Try this. If it also doesn't work, I'll make a patch with debugging and will have to ask you to try it :)

Try this. If it also doesn't work, I'll make a patch with debugging and will have to ask you to try it :)

Lets go straight for that.

If you have a patch that works but can't explain why I'll instantly reject it anyway.

Also, we can't have a situation where both the C++ side and the QML side is responsible for setting the currentIndex on the model.

The UI expects all decorations in the model to have a 'theme name'

Where?

vpilo updated this revision to Diff 49128.Jan 10 2019, 12:11 AM
  • Add debug logging

Try this. If it also doesn't work, I'll make a patch with debugging and will have to ask you to try it :)

Lets go straight for that.

If you have a patch that works but can't explain why I'll instantly reject it anyway.

Sure, why not cut the chase :)

Also, we can't have a situation where both the C++ side and the QML side is responsible for setting the currentIndex on the model.

If I can be sure that the QML is loaded when load() runs, I'll just ditch this initial theme variable nonsense that was there from the beginning.

The UI expects all decorations in the model to have a 'theme name'

Where?

'Twas a long day - I didn't mean the UI, but DecorationsModel::findDecoration()

ngraham resigned from this revision.Jan 10 2019, 7:47 PM

The latest revision works for me for both kcmshell5 and in System Settings. Here's the logging output: https://paste.kde.org/pckuki7oq

Probably hold off on landing this until we can make sense of the logging and figure out what the most appropriate diff is. :)

This revision is now accepted and ready to land.Jan 10 2019, 7:47 PM
davidedmundson requested changes to this revision.Jan 11 2019, 2:55 AM

Not sure what happened with phab, Nate has this as accepting and resigning.

Obviously it can't merge with the debug.

I don't like this approach, it doesn't follow data<-->UI separation which the other (much nicer) newly QML ported KCMs have.

I also can't understand how it magically fixes it for Nate when his output doesn't even have
"TEST current plugin on load: " in it which means we know it didn't get in this new code?

This revision now requires changes to proceed.Jan 11 2019, 2:55 AM
vpilo added a subscriber: ngraham.Jan 15 2019, 5:23 PM

I'm also really surprised about Nate's log output; there should be something. It should not be possible for that warning to not show up. @ngraham - did you run kcmshell/systemsettings via the command line? can you check in your ~/.xsession-errors ?

Logs aside - I'll be dropping this diff and refactoring the whole thing to the new way.

My log output was wrong and only showed part of it, due to a Konsole bug: https://bugs.kde.org/show_bug.cgi?id=403117.

Do you want me to test again and attach proper logs, or would that be a waste of time because of your planned refactoring?

vpilo added a comment.Jan 15 2019, 5:28 PM

Never a waste of time :) thank you!

vpilo added a comment.Jan 16 2019, 9:18 AM

Here's the correct paste data: https://paste.kde.org/plkc8ixmm

now it makes sense 😆 Thank you!

It seems it's all fine. I'll take the fix into the refactoring.

vpilo abandoned this revision.Jan 16 2019, 9:47 AM