Bug 438197

Summary:	Window operations menu desktop selection menu closes when clicking a desktop checkbox
Product:	[Plasma] kwin	Reporter:	Oded Arbel <oded>
Component:	platform-wayland-nested	Assignee:	KWin default assignee <kwin-bugs-null>
Status:	REPORTED ---
Severity:	normal	CC:	azrdev, nate
Priority:	NOR	Keywords:	usability
Version:	git master
Target Milestone:	---
Platform:	Other
OS:	Linux
See Also:	https://bugs.kde.org/show_bug.cgi?id=438198
Latest Commit:		Version Fixed In:
Sentry Crash Report:
Attachments:	screencast showing a possible fix

Description Oded Arbel 2021-06-07 10:01:03 UTC

SUMMARY
As originally reported in bug 438148, the window operations menu "Desktops" sub menu, on wayland, uses a checkbox UI (compared to a "radio button" UI in X11) which seems to suggest that users can check and uncheck multiple boxes - but whenever the user clicks a checkbox, the menu closes and has to be reopened again (RMB on title, navigate to "Desktops") to continue working with the checkboxes.

STEPS TO REPRODUCE
1. Open window operations menu (by RMB or keyboard shortcut).
2. Choose "Desktops".
3. Click on any of the desktop entry checkbox.

OBSERVED RESULT
The window operation menu closes.

EXPECTED RESULT
The window operations menu should maybe stay open so that the more checkboxes can be toggled?

SOFTWARE/OS VERSIONS
KDE Plasma Version: 5.22.80
KDE Frameworks Version: 5.83.0
Qt Version: 5.15.3

ADDITIONAL INFORMATION
I haven't used wayland a lot until recently, but I recall the behavior on X11 (where only one desktop entry can be chosen) used to be that the desktop selection menu wouldn't close until it was dismissed, allowing the user to change their mind and choose a different target desktop - but the result was that when moving the window away from the current desktop, you'd be left with an open window operations menu for a window that is no longer on screen and that was a bit weird.

That being said, the current behavior on wayland is that after "unchecking" the current desktop (and assuming another desktop entry is checked), the window is unmapped from the current desktop but is still active - so the window operations menu keyboard shortcut will bring the menu up for the window that is not on the screen... Maybe that is another bug?

The alternative suggested behavior from bug #438148 was to not close the menu nor update the virtual desktop mapping until the user clicks some kind of "apply" action in the menu. Another option is to just drop the wayland-specific "allow a window on multiple but not all desktops" and revert to the less problematic switching behavior and keep the multiple desktops support using a window rule or some other UI that can take that complexity.

Comment 1 Nate Graham 2021-06-08 19:42:35 UTC

This is a core behavior of QMenu from Qt which cannot be changed by us. The real solution is probably to just fix Bug 438198. :)

Comment 2 Oded Arbel 2021-07-05 15:53:49 UTC

(In reply to Nate Graham from comment #1)
> This is a core behavior of QMenu from Qt which cannot be changed by us. The
> real solution is probably to just fix Bug 438198. :)

This assertion doesn't seem to be correct. For once, QAction::triggered() documentation - https://doc.qt.io/qt-5/qaction.html#triggered - notes that triggered() is not emitted by toggle(). But also notably, kwin's relevant code - here: https://invent.kde.org/odeda/kwin/-/blob/770875a76e662a1ed7728da769f8f7d019623701/src/useractions.cpp#L585 - connects the checkbox's clicked() signal to the QAction:triggered() slot. It didn't have to do that - that signal can be connected to something in UserActionsMenu that understands that a checkbox was clicked and not trigger the QAction.

Comment 3 Oded Arbel 2021-07-05 18:37:59 UTC

Created attachment 139877 [details]
screencast showing a possible fix

Here's a hack I've done on the activities menu (I'm currently on X11 so I have radio buttons for virtual desktops, but the code is kind of the same on activities, so I messed with that): 

I replaced the connect() call in activityPopupAboutToShow() with the following code:

connect(box, &QCheckBox::clicked, [this,action](bool checked){ Q_UNUSED(checked); this->slotToggleOnActivity(action); });

and the result can be seen in the animation - the checkboxes can be toggled without closing the menu.

Comment 4 Nate Graham 2021-07-23 20:56:40 UTC

Interesting! I had no idea.

Can you submit that as a MR that I and others can test and review?

Comment 5 Oded Arbel 2021-07-23 23:46:31 UTC

(In reply to Nate Graham from comment #4)
> Can you submit that as a MR that I and others can test and review?

Its probably not a good idea - the behavior is not great (it goes back to how it used to be in the past where if you unchecked the current activity, the window would disappear from the screen while the menu is still visible) and the rest of the code is a bit of a mess (actually, I lied - the line I've shown is not the only line to be changed - a couple of things need to be moved around, though once you try to use that code you'd see immediately for yourself).

When I find a bit more time (I'm kind of busy over my head with other tasks atm), I'll make a clean patch that tries to address the "unchecking yourself" edge case.