KCM KWinTouchScreen port to KConfigXT
ClosedPublic

Authored by crossi on Mar 5 2020, 9:59 AM.

Details

Summary

Also manage KCModule states (isSaveNeeded and isDefaults)

BUG: 405573

Test Plan
  • exec kcmshell5 kwintouchscreen
  • In an edge, select action Present windows - All desktops, then apply
  • close and reopen
  • Should display action Present windows - All desktops in the previously selected edge.
  • Do same test with Present windows - current desktop

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 24619
Build 24637: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
crossi retitled this revision from KWin Touch Screen port to KConfigXT to KCM KWinTouchScreen port to KConfigXT.Mar 5 2020, 10:45 AM
crossi updated this revision to Diff 77028.Mar 5 2020, 2:06 PM

fix CMakeList

zzag added inline comments.Mar 5 2020, 2:45 PM
kcmkwin/kwinscreenedges/touch.cpp
68

Could you please use an explicit type here? It's not obvious what type setting has.

https://community.kde.org/Policies/Library_Code_Policy#auto_Keyword

364

No short names

int a -> int action

ervin requested changes to this revision.Mar 5 2020, 4:04 PM

It's only a first step right? We'll move toward addConfig in a further step? Wondering what the plan is there.

kcmkwin/kwinscreenedges/touch.cpp
68

If I had my way this policy would get an update. This is "obviously" a setting object so it can be loaded. ;-)

More seriously, and more importantly (to me) this lacks a qAsConst for m_scriptSettings

87

qAsConst missing

108

qAsConst missing

kcmkwin/kwinscreenedges/touch.h
87

a -> action also I wonder why it's an int and not ElectricBorderAction

This revision now requires changes to proceed.Mar 5 2020, 4:04 PM
crossi updated this revision to Diff 77292.Mar 9 2020, 4:03 PM

Consider comments

crossi updated this revision to Diff 77297.Mar 9 2020, 4:24 PM
crossi marked 4 inline comments as done.

remove auto, qAsConst

crossi marked 2 inline comments as done.Mar 9 2020, 4:25 PM
crossi added inline comments.
kcmkwin/kwinscreenedges/touch.h
87

Sadly it takes the action from Monitor::selectedEdgeItem() which is an int, and also includes values that are not ElectricBorderAction.

crossi added a comment.Mar 9 2020, 4:46 PM

It's only a first step right? We'll move toward addConfig in a further step? Wondering what the plan is there.

Since there is no link between the widgets and the settings, I don't know how to move to addConfig.

crossi planned changes to this revision.Mar 10 2020, 10:47 AM
crossi updated this revision to Diff 77423.Mar 11 2020, 3:46 PM

Move ui class to separate file.
Move from inheritance to delegating.
No longer write directly to settings, only on save.

crossi retitled this revision from KCM KWinTouchScreen port to KConfigXT to [WIP] KCM KWinTouchScreen port to KConfigXT.Mar 11 2020, 3:48 PM
crossi planned changes to this revision.Mar 11 2020, 3:53 PM

Plan to make a common component that will be used by kwinscreenedges as well.

ervin requested changes to this revision.Mar 16 2020, 4:37 PM
ervin added inline comments.
kcmkwin/kwinscreenedges/kwnscreenedgeconfigform.cpp
34 ↗(On Diff #77423)

This is leaked

84 ↗(On Diff #77423)

those static casts are not strictly required, enums convert implicitly to int

154 ↗(On Diff #77423)

ditto

kcmkwin/kwinscreenedges/kwnscreenedgeconfigform.h
42 ↗(On Diff #77423)

= nullptr missing

kcmkwin/kwinscreenedges/touch.cpp
50–51

This would be better in the initialization list. Also this is leaked.

185

Since you're touching those lines anyway, please clean up the c-casts to int

crossi updated this revision to Diff 77997.Mar 19 2020, 1:29 PM

fix leak. rename m_ui to m_form since it's not the ui. remove unnecessary enum cast.

crossi updated this revision to Diff 78009.Mar 19 2020, 2:38 PM
crossi marked 5 inline comments as done.

remove unnecessary cast

crossi marked an inline comment as done.Mar 19 2020, 2:39 PM
crossi added inline comments.
kcmkwin/kwinscreenedges/touch.cpp
50–51

Agree, much better to put in initialization list.
It's not leaked, I renamed it to m_form since it's not the ui thing and the name was confusing.

ervin accepted this revision.Mar 27 2020, 4:53 PM
ervin added inline comments.
kcmkwin/kwinscreenedges/touch.cpp
50–51

Ah indeed, stupid me.

This revision is now accepted and ready to land.Mar 27 2020, 4:53 PM
zzag added a comment.Mar 30 2020, 8:47 AM

Is it still WIP?

kcmkwin/kwinscreenedges/kwnscreenedgeconfigform.cpp
24 ↗(On Diff #78009)

I don't see where it's used. Remove?

In D27862#637793, @zzag wrote:

Is it still WIP?

Yes, still in progress.
I want to make the monitor thing reusable for KCM KWinScreenEdge as this part is identical.

crossi planned changes to this revision.Mar 31 2020, 7:31 AM
crossi updated this revision to Diff 78972.Mar 31 2020, 10:01 AM

rework to allow code reuse in KCM KWinScreenEdges

This revision is now accepted and ready to land.Mar 31 2020, 10:01 AM
crossi retitled this revision from [WIP] KCM KWinTouchScreen port to KConfigXT to KCM KWinTouchScreen port to KConfigXT.Mar 31 2020, 10:05 AM
crossi requested review of this revision.Mar 31 2020, 10:10 AM

From previous revision, I moved code related to the Monitor part in the ui in a separate class KWinScreenEdge, that will be used by KCM KWinScreenEdges to remove duplicated code and ease the port to KConfigXT.

zzag accepted this revision.Mar 31 2020, 10:55 AM
This revision is now accepted and ready to land.Mar 31 2020, 10:55 AM
crossi updated this revision to Diff 79020.Apr 1 2020, 8:17 AM

rename include guard

crossi updated this revision to Diff 79116.Apr 2 2020, 9:39 AM

fix defaults

meven accepted this revision.Apr 7 2020, 7:13 AM
ervin requested changes to this revision.Apr 7 2020, 3:51 PM

Two little things.

kcmkwin/kwinscreenedges/kwinscreenedge.cpp
35

That KWin namespace is redundant here.

77

Either:
auto list = QList<int>();
or
QList<int> list;

This revision now requires changes to proceed.Apr 7 2020, 3:51 PM
crossi updated this revision to Diff 79591.Apr 7 2020, 4:11 PM

Fix two little things

crossi marked 2 inline comments as done.Apr 7 2020, 4:11 PM
ervin accepted this revision.Apr 7 2020, 4:13 PM
This revision is now accepted and ready to land.Apr 7 2020, 4:13 PM
crossi updated this revision to Diff 80073.Apr 14 2020, 9:45 AM

Fix action Present Windows - Current desktop that was saved as All desktop and vice versa

crossi edited the summary of this revision. (Show Details)Apr 14 2020, 9:52 AM
crossi edited the test plan for this revision. (Show Details)
ervin accepted this revision.Apr 21 2020, 6:30 PM
This revision was automatically updated to reflect the committed changes.

This seems to cause a KWin build failure for me:

/home/nate/kde/src/kwin/kcmkwin/kwinscreenedges/kwinscreenedge.cpp: In static member function ‘static int KWin::KWinScreenEdge::electricBorderToMonitorEdge(KWin::ElectricBorder)’:
/home/nate/kde/src/kwin/kcmkwin/kwinscreenedges/kwinscreenedge.cpp:170:5: error: control reaches end of non-void function [-Werror=return-type]
  170 |     default: // ELECTRIC_COUNT and ElectricNone
      |     ^~~~~~~
ervin added inline comments.Apr 22 2020, 8:42 PM
kcmkwin/kwinscreenedges/kwinscreenedge.cpp
166 ↗(On Diff #79020)

Ah indeed, in non-debug builds Q_ASSERT is a noop so you still need to return something after it. Also now I realize that using Q_UNREACHABLE would have probably been better too.