Details
- Reviewers
hein graesslin - Group Reviewers
KWin - Commits
- R108:8a2a00a4ca80: Add "SkipSwitcher" to API
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.
Following discussion in BUG 375921 , it was decided to add SkipSwitcher to the API. As this is my first submission to KWin/KWayland/KWindowSystem, @hein has volunteered to help me through this.
From what I'm able to tell from the autotests, this patch appears successful. It's part of a set including D11925 and D11924.
As Martin wrote in the bug report, it was reasonably straightforward, following the implementation of SkipTaskbar.
| abstract_client.h | ||
|---|---|---|
| 1042 | Was accidentally duplicated during patching. Moved above Unaffected comment, following example of m_skipTaskbar | |
Sorry for the delay in review, I was on vacations.
| abstract_client.cpp | ||
|---|---|---|
| 147–149 | why do you move it to the top of the FocusChain? This looks like a completely unrelated logical change | |
| abstract_client.h | ||
| 1042 | I don't think the originalSkipSwitcher is needed. | |
| client.cpp | ||
| 878–887 | this is not needed. KWin won't show such hidden windows in the switcher. For the taskbar it's needed as taskbar is in plasmashell and doesn't know about KWin's workaround. | |
| abstract_client.cpp | ||
|---|---|---|
| 147–149 | To the best of my recollection, I was trying to reproduce the code used for skipTaskBar, beginning at original line 165. I confess I don't fully understand what this code does. Please clarify for me where it properly belongs and I'll resubmit the patch. | |
| 166 | SkipSwitcher focus chain borrowed from here. | |
| client.cpp | ||
| 878–887 | Removed both references to skipSwitcher | |
| abstract_client.cpp | ||
|---|---|---|
| 147–149 | The skipTaksBar is not the perfect example, the skipPager seems to be better suited. The main reason for the weird issue is that KWin is doing some adjustments for window tabs (which are currently not available). If the window is not the active tab it gets visually hidden and the skip taskbar is set to remove it from the taskbar. Due to that there is all the original skip taksbar, making it the first in focus chain, etc. For skipSwitcher we don't need all of that. Switcher is KWin internal and KWin knows the state of the window. | |
Martin:
I can no longer get these patches to compile, even without addressing your inline comments from earlier today. I've pulled down a fresh master of KWin and the arc patch D11926 applied cleanly. However, attempting to build it generates
/home/bundito/kde/src/kde/workspace/kwin/abstract_client.cpp: In member function ‘void KWin::AbstractClient::setupWindowManagementInterface()’:
/home/bundito/kde/src/kde/workspace/kwin/abstract_client.cpp:721:8: error: ‘class KWayland::Server::PlasmaWindowInterface’ has no member named ‘setSkipSwitcher’; did you mean ‘setSkipTaskbar’?
w->setSkipSwitcher(skipSwitcher());
^~~~~~~~~~~~~~~
setSkipTaskbar
/home/bundito/kde/src/kde/workspace/kwin/abstract_client.cpp: In lambda function:
/home/bundito/kde/src/kde/workspace/kwin/abstract_client.cpp:737:16: error: ‘class KWayland::Server::PlasmaWindowInterface’ has no member named ‘setSkipSwitcher’; did you mean ‘setSkipTaskbar’?
w->setSkipSwitcher(skipSwitcher());
^~~~~~~~~~~~~~~
setSkipTaskbarThinking it might be a problem related to KWayland, I tried recompiling and reinstalling that as well. I followed the same procedure (fresh master -> arc patch D11925). That also fails, in a more cryptic manner:
/home/bundito/kde/src/frameworks/kwayland/src/server/plasmashell_interface.cpp:172:1: error: invalid conversion from ‘void (*)(wl_client*, wl_resource*, uint32_t) {aka void (*)(wl_client*, wl_resource*, unsigned int)}’ to ‘void (*)(wl_client*, wl_resource*)’ [-fpermissive]
};
^
/home/bundito/kde/src/frameworks/kwayland/src/server/plasmashell_interface.cpp:172:1: error: invalid conversion from ‘void (*)(wl_client*, wl_resource*)’ to ‘void (*)(wl_client*, wl_resource*, uint32_t) {aka void (*)(wl_client*, wl_resource*, unsigned int)}’ [-fpermissive]As the keeper of the KWin/KWayland systems, I hope you can shed some light onto what's happened since I wrote these patches. Have other updates made to the master branch made my patches incompatible?
KWindowSystem, which was also part of this three-part patch (D11924) compiles fine.
Please let me know how I can troubleshoot and rework these patches. If you're still out-of-sync from being on holiday, I can ask @hein if you prefer. I've also had some correspondence with @davidedmundson , if he can be of help.
As the keeper of the KWin/KWayland systems, I hope you can shed some light onto what's happened since I wrote these patches. Have other updates made to the master branch made my patches incompatible?
Can you try a clean rebuild of KWayland? Maybe some of the auto-generated files cause the issue.
| abstract_client.cpp | ||
|---|---|---|
| 139 | Please move the skipSwitcher method back to here. So that the diff shows what really changed | |
| 199 | We don't need any adjustments for the focus chain. Just remove the if and the update | |
| abstract_client.h | ||
| 293 | Empty newline added | |
| 1038 | There's no need to move this variable around | |
| client.cpp | ||
| 884 | Unrelated change | |
| abstract_client.cpp | ||
|---|---|---|
| 145 | we still need the updateWindowRules | |