Also manage KCModule states (isSaveNeeded and isDefaults)
BUG: 405573
Details
Details
- 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
Diff Detail
- Repository
- R108 KWin
- Lint
Lint Skipped - Unit
Unit Tests Skipped - Build Status
Buildable 24556 Build 24574: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
| 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 | |
Comment Actions
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 | |
| 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. | |
Comment Actions
Since there is no link between the widgets and the settings, I don't know how to move to addConfig.
Comment Actions
Move ui class to separate file.
Move from inheritance to delegating.
No longer write directly to settings, only on save.
| 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 | |
| kcmkwin/kwinscreenedges/touch.cpp | ||
|---|---|---|
| 50–51 | Agree, much better to put in initialization list. | |
| kcmkwin/kwinscreenedges/touch.cpp | ||
|---|---|---|
| 50–51 | Ah indeed, stupid me. | |
Comment Actions
Is it still WIP?
| kcmkwin/kwinscreenedges/kwnscreenedgeconfigform.cpp | ||
|---|---|---|
| 24 ↗ | (On Diff #78009) | I don't see where it's used. Remove? |
Comment Actions
Yes, still in progress.
I want to make the monitor thing reusable for KCM KWinScreenEdge as this part is identical.
Comment Actions
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.
Comment Actions
Fix action Present Windows - Current desktop that was saved as All desktop and vice versa
Comment Actions
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
| ^~~~~~~| kcmkwin/kwinscreenedges/kwinscreenedge.cpp | ||
|---|---|---|
| 166 | 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. | |