KCM/Compositing: Use KConfig XT in UI
ClosedPublic

Authored by meven on Mar 11 2020, 4:56 PM.

Details

Summary

Use KConfig XT to manage most fields of the KCM.
Simplify code.

Depends on D27955

Test Plan

No functional change

Diff Detail

Repository
R108 KWin
Branch
kwin-kconfigxt
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23578
Build 23596: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
zzag added inline comments.Mar 16 2020, 9:44 AM
kcmkwin/kwincompositing/main.cpp
81

Is there a reason why the parent is not this?

ervin requested changes to this revision.Mar 16 2020, 6:53 PM
ervin added inline comments.
kcmkwin/kwincompositing/main.cpp
88–91

Well, this is captured because of m_settings, isn't it?

It would be nice to have this as third connect parameter though.

158

Please add this as third connect parameter

This revision now requires changes to proceed.Mar 16 2020, 6:53 PM
zzag added inline comments.Mar 16 2020, 8:29 PM
kcmkwin/kwincompositing/main.cpp
88–91

Yes, this is captured but I would say it's not an excuse for using a lambda. A method would be more appropriate in this case.

bport requested changes to this revision.Mar 17 2020, 8:31 AM
bport added inline comments.
kcmkwin/kwincompositing/kwincompositing_setting.kcfg
49

Perhaps we can add comment, kcfg can be used as documentation

kcmkwin/kwincompositing/main.cpp
204

Using m_settings to track unmanaged state lead to settings containing old data and new data. Managed widget update settings only on save.
You will need to use another object to track change in unmanaged widget.

meven updated this revision to Diff 78435.Mar 25 2020, 9:41 AM

Address review: store loaded values for unmanaged widgets in dedicated members

meven marked 9 inline comments as done.Mar 25 2020, 9:49 AM
meven added inline comments.
kcmkwin/kwincompositing/kwincompositing_setting.kcfg
49

I have added the proper enum's name

meven marked 2 inline comments as done.Mar 25 2020, 9:49 AM
meven updated this revision to Diff 78439.Mar 25 2020, 10:03 AM

Add two missing lines

ervin requested changes to this revision.Mar 27 2020, 5:26 PM
ervin added inline comments.
kcmkwin/kwincompositing/kwincompositing_setting.kcfg
26

This is asking for name + value (to avoid the dreaded "gL" on the C++ side)

46

ditto

65

ditto

73

ditto

kcmkwin/kwincompositing/main.cpp
105–106

{ should be on its own line

211

I'd split the clauses on several lines for readability

214

ditto (especially for that one the line is getting quite long)

This revision now requires changes to proceed.Mar 27 2020, 5:26 PM
meven updated this revision to Diff 78688.Mar 27 2020, 5:48 PM
meven marked 6 inline comments as done.

Break down check on multiple lines, use lowercase gl prefix, formatting

ervin accepted this revision.Mar 27 2020, 5:54 PM
ervin added inline comments.
kcmkwin/kwincompositing/main.cpp
57

That one was oddly fine (yes, Qt is inconsistent with dealing with GL and we are as well ;-))

213

Ah, not what I had in mind since I would have liked to keep the const but no big deal

meven updated this revision to Diff 78843.Mar 30 2020, 7:42 AM

Rebasing

meven updated this revision to Diff 78844.Mar 30 2020, 7:43 AM

Rebasing

meven updated this revision to Diff 78845.Mar 30 2020, 7:44 AM

Rebasing

zzag requested changes to this revision.Mar 30 2020, 9:24 AM

Hmm, I cannot test this kcm because it crashes.

lldb output
* thread #1, name = 'kcmshell5', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x00007fffef767d07 kwincompositing.so`KWinCompositingKCM::load(this=0x00005555555ef6c0) at main.cpp:243:68
   240      m_form.animationDurationFactor->setValue(index);
   241 
   242      m_settings->findItem("Backend")->readConfig(m_settings->config());
-> 243      m_settings->findItem("GLCore")->readConfig(m_settings->config());
   244      m_backend = m_settings->backend();
   245      if (m_backend == KWinCompositingSetting::EnumBackend::OpenGL) {
   246          m_glCore = m_settings->glCore();
(lldb) bt
* thread #1, name = 'kcmshell5', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: 0x00007fffef767d07 kwincompositing.so`KWinCompositingKCM::load(this=0x00005555555ef6c0) at main.cpp:243:68
    frame #1: 0x00007fffef7682a2 kwincompositing.so`KWinCompositingKCM::qt_static_metacall(_o=0x00005555555ef6c0, _c=InvokeMetaMethod, _id=0, _a=0x000055555567a478) at main.moc:85:25
    frame #2: 0x00007ffff605daab libQt5Core.so.5`QMetaCallEvent::placeMetaCall(this=0x000055555567a430, object=0x00005555555ef6c0) at qobject.cpp:617:24
    frame #3: 0x00007ffff605e9a5 libQt5Core.so.5`QObject::event(this=0x00005555555ef6c0, e=0x000055555567a430) at qobject.cpp:1314:31
    frame #4: 0x00007ffff6ee188d libQt5Widgets.so.5`QWidget::event(this=0x00005555555ef6c0, event=0x000055555567a430) at qwidget.cpp:9089:30
    frame #5: 0x00007ffff6e92ea5 libQt5Widgets.so.5`QApplicationPrivate::notify_helper(this=0x000055555557ea20, receiver=0x00005555555ef6c0, e=0x000055555567a430) at qapplication.cpp:3671:31
    frame #6: 0x00007ffff6e92c95 libQt5Widgets.so.5`QApplication::notify(this=0x00007fffffffdf70, receiver=0x00005555555ef6c0, e=0x000055555567a430) at qapplication.cpp:3621:31
    frame #7: 0x00007ffff6016ddf libQt5Core.so.5`QCoreApplication::notifyInternal2(receiver=0x00005555555ef6c0, event=0x000055555567a430) at qcoreapplication.cpp:1061:24
    frame #8: 0x00007ffff601783c libQt5Core.so.5`QCoreApplication::sendEvent(receiver=0x00005555555ef6c0, event=0x000055555567a430) at qcoreapplication.cpp:1456:27
    frame #9: 0x00007ffff60185ab libQt5Core.so.5`QCoreApplicationPrivate::sendPostedEvents(receiver=0x0000000000000000, event_type=0, data=0x000055555556c400) at qcoreapplication.cpp:1815:36
    frame #10: 0x00007ffff6017ee4 libQt5Core.so.5`QCoreApplication::sendPostedEvents(receiver=0x0000000000000000, event_type=0) at qcoreapplication.cpp:1674:46
    frame #11: 0x00007ffff60b0164 libQt5Core.so.5`::postEventSourceDispatch(s=0x00005555555e4380, (null)=0x0000000000000000, (null)=0x0000000000000000) at qeventdispatcher_glib.cpp:277:39
    frame #12: 0x00007ffff198c9be libglib-2.0.so.0`g_main_context_dispatch + 638
    frame #13: 0x00007ffff198e831 libglib-2.0.so.0`___lldb_unnamed_symbol354$$libglib-2.0.so.0 + 545
    frame #14: 0x00007ffff198e871 libglib-2.0.so.0`g_main_context_iteration + 49
    frame #15: 0x00007ffff60b08bf libQt5Core.so.5`QEventDispatcherGlib::processEvents(this=0x00005555555e75d0, flags=(i = 36)) at qeventdispatcher_glib.cpp:423:43
    frame #16: 0x00007fffef83242a libQt5XcbQpa.so.5`QXcbGlibEventDispatcher::processEvents(this=0x00005555555e75d0, flags=(i = 36)) at qxcbeventdispatcher.cpp:143:47
    frame #17: 0x00007ffff601347b libQt5Core.so.5`QEventLoop::processEvents(this=0x00007fffffffde30, flags=(i = 36)) at qeventloop.cpp:139:68
    frame #18: 0x00007ffff60137dd libQt5Core.so.5`QEventLoop::exec(this=0x00007fffffffde30, flags=(i = 0)) at qeventloop.cpp:232:22
    frame #19: 0x00007ffff601767e libQt5Core.so.5`QCoreApplication::exec() at qcoreapplication.cpp:1369:36
    frame #20: 0x00007ffff6560bd4 libQt5Gui.so.5`QGuiApplication::exec() at qguiapplication.cpp:1864:34
    frame #21: 0x00007ffff6e8fa27 libQt5Widgets.so.5`QApplication::exec() at qapplication.cpp:2811:33
    frame #22: 0x00007ffff7f7e4d3 libkdeinit5_kcmshell5.so`kdemain + 6435
    frame #23: 0x00007ffff7dd6023 libc.so.6`__libc_start_main + 243
    frame #24: 0x000055555555505e kcmshell5`_start + 46
kcmkwin/kwincompositing/kwincompositing_setting.kcfg
72–78

Please remove this entry because it's unused. In general, it would probably be nice to re-introduce the "OpenGL Platform Interface" option. It was originally present in the QML based version of this KCM, but then we switched back to qwidgets. :/

kcmkwin/kwincompositing/main.cpp
61

Make it static.

99

stray whitespace between "kcfg_WindowsBlockCompositing" and "->". Please remove it.

234

Maybe glCore?

This revision now requires changes to proceed.Mar 30 2020, 9:24 AM
meven updated this revision to Diff 78911.Mar 30 2020, 3:51 PM
meven marked 5 inline comments as done.

Fix crash, adress review, fix a couple of left issues

meven added inline comments.Mar 30 2020, 3:51 PM
kcmkwin/kwincompositing/main.cpp
234

Indeed this caused the crash when I changed the name for this entry.

meven marked an inline comment as done.Mar 30 2020, 3:52 PM
zzag requested changes to this revision.Mar 30 2020, 4:20 PM
zzag added inline comments.
kcmkwin/kwincompositing/main.cpp
40

Since isRunningPlasma is static, you don't need to put it in an anonymous namespace. Also, coding style nitpick: { must be on its own line.

243

We need to initialize m_glCore to something; otherwise it will cause problems in updateUnmanagedItemStatus() later on.

This revision now requires changes to proceed.Mar 30 2020, 4:20 PM
meven updated this revision to Diff 78955.Mar 31 2020, 7:42 AM
meven marked an inline comment as done.

Initial m_glCore, move isRunningPlasma out of anonymous namespace

meven marked an inline comment as done.Mar 31 2020, 7:42 AM
anthonyfieroni added inline comments.
kcmkwin/kwincompositing/main.cpp
169–172

It's translatable ?

meven added inline comments.Mar 31 2020, 7:52 AM
kcmkwin/kwincompositing/main.cpp
169–172

1/ That's not much related to this diff : this wasn't changed.

2/ That's up to translators to say I would guess.
For some languages, they may use a different alphabet to spell it.

zzag added a comment.Mar 31 2020, 7:53 AM

"Scale method" and "Scale method" must be visible only when the rendering backend is set to "XRender".

meven updated this revision to Diff 78956.Mar 31 2020, 8:01 AM

Fix case on load with Open GL 3.1 two scale methods fields displayed

meven updated this revision to Diff 78957.Mar 31 2020, 8:08 AM

Avoid small glitch in form scale method field position when switching backend between opengl and Xrender

zzag accepted this revision.Mar 31 2020, 8:30 AM
zzag added inline comments.
kcmkwin/kwincompositing/main.cpp
38

Put the opening brace on newline.

meven updated this revision to Diff 79003.Mar 31 2020, 3:43 PM

Put { on a newline

meven marked an inline comment as done.Mar 31 2020, 3:43 PM
meven updated this revision to Diff 79033.Apr 1 2020, 9:38 AM

Add immutability handling for backend and animation duration factor

ervin added a comment.Apr 7 2020, 1:52 PM

LGTM, I'll let the last word for @zzag though.

zzag added a comment.Apr 14 2020, 6:09 PM

Add immutability handling for backend and animation duration factor

When can those two be immutable?

In D27988#648372, @zzag wrote:

Add immutability handling for backend and animation duration factor

When can those two be immutable?

Most of kcm settings can have their field marked immutable by system administrators i.e https://userbase.kde.org/KDE_System_Administration/Kiosk/Introduction
And fields that are handled automatically by KConfig XT + KCM.
Those two fields have to be separately handled since they couldn't be handled this way.

zzag accepted this revision.Apr 16 2020, 8:42 AM
bport accepted this revision.Apr 17 2020, 12:52 PM
This revision is now accepted and ready to land.Apr 17 2020, 12:52 PM
meven updated this revision to Diff 80618.Apr 20 2020, 8:11 AM

Rebasing

This revision was automatically updated to reflect the committed changes.