Bug 354090

Plasma must have focus for this to work, so first step to reproduce is a click on desktop.

Is this 352933 ? I hope so, we couldn't find a way to accurately reproduce it.

I tried your steps, they didn't seem to work for me. Does that method break it 100% reliably for you?

can you xprop the main desktop view between steps 4 and 5?

Created attachment 95050 [details]
xprop on desktop view before the bug

Yes, it's 100% reliable. 

I'm uploading the output from xprop of the desktop view - before/after on the working virtual desktop, other virtual desktops (black) don't have a plasma window at all (xprop on black returns root X window).

The only thing that changed in those logs is _NET_WM_DESKTOP = 4294967295 before / 1 after

Created attachment 95051 [details]
xprop on desktop view after the bug

And yes, it pretty much looks like 352933, because the issue is that the desktop view and panels lose the "on all desktops" flag.

The part I'm interested in is knowing when and why that xprop changes.

Two more things then:
 - can you run xprop -spy | grep _NET_WM_DESKTOP

leave it somewhere visible and work out when during steps 1-5 it changes?

 - does it happen if we use openbox?

*** Bug 352933 has been marked as a duplicate of this bug. ***

_NET_WM_DESKTOP changes right after opening the dialog.

I cannot reproduce it with openbox.

Bisected to 

commit 254887155c820284b7d9dc68f171a2c7c0ec45c6
Author: Martin Gräßlin <mgraesslin@kde.org>
Date:   Fri Mar 13 12:54:11 2015 +0100

    Implement virtual desktop handling in AbstractClient

I'm not able to reproduce.

Created attachment 95052 [details]
Open modal dialog with Quicklaunch -> Add Launcher

Sorry, looks like git kicker doesn't trigger this bug anymore (Plasma/5.4 does). 

Applying the quicklaunch-modal.diff to kdeplasma-addons and then adding Quicklaunch widget -> right click -> Add Launcher should trigger the bug.

I just found easier way to reproduce it, open Kicker settings -> use custom image -> open file dialog.

@David R., do you have commit b19935677402a9fd14f58a2ad0d495fd57d7cb87 ?

Yes, I can reproduce it with current master (93b5e13308bfc8e5b546fff63d926f26ec3c4a54).

Oh, and in case you can try patches, check the range check in abstract_client.cpp, AbstractClient::setDesktop(int desktop)

It smells like
   desktop = qMin(numberOfDesktops, rules()->checkDesktop(desktop));
does not translate to
   -1 = qMin(numberOfDesktops, rules()->checkDesktop(-1));
for you.

>  Add Launcher should trigger the bug.

nope, I'm not able to trigger it. There must be something more in the equation to trigger it.

Created attachment 95054 [details]
Only move parent of modal windows when not on all desktops

@Thomas Thanks for the pointer, this patch fixes it. But now sometimes the dialog is not correctly positioned - eg. the file dialog is not in the center of kicker settings, but sometimes is in the center of the screen.

I'm pretty sure that the modal dialog needs to effectively change the VD (ie. like start on VD #1 and be moved to VD #2 to be visible, triggering a VD update of the main client, ie. the desktop, and accidentally moves that to VD #2 instead of sufficiently keeping it on VD #-1)

The problem I could envision is about the type resolution of the enum, ie. NET::AllDesktops is converted to 4294967295 instead of -1, either because int is 64bit on that system or we're ending up with uints - both would be *extremely* odd, though.

(In reply to Thomas Lübking from comment #18)
> The problem I could envision is about the type resolution of the enum, ie.
> NET::AllDesktops is converted to 4294967295 instead of -1, either because
> int is 64bit on that system or we're ending up with uints - both would be
> *extremely* odd, though.

kwindowsystem/src/platforms/xcb/netwm.cpp:3290
        const uint32_t data[5] = {
            desktop == OnAllDesktops ? 0xffffffff : desktop - 1, 0, 0, 0, 0
        };

Eh, disregard my last comment (it gets translated to OnAllDesktops when reading back).

Anyway, I think the issue is simply that it forces the desktop for parent (as can be seen in the patch).

I'm still curious on whether the constrainment fails (and on what values) - we would have to fix that *there* and not in "some" calling code (and not in other places ;-)

The dialogs should be positioned centered on their leader.
However, there's also a dead-ugly Qt5 bug. It centers modal (and only modal!) dialogs on screen after re-mapping them (eg. shade a Qt5 modal dialog, unshade and whatch it jumping to the screens center....)

=> the window will likely do a show/hide/show dance on your system (along the VD change)?

(In reply to Thomas Lübking from comment #21)
> I'm still curious on whether the constrainment fails (and on what values) -
> we would have to fix that *there* and not in "some" calling code (and not in
> other places ;-)
> 

I don't see anything wrong with it.

desktop = qMin(numberOfDesktops, rules()->checkDesktop(desktop));

desktop = [number of desktop where the modal window is]
numberOfDesktops = 4 [from my config]
rules()->checkDesktop(desktop) = this just applies window rules (from window management kcm?) - won't change it in my case and just returns desktop

so it gets called as 
desktop = qMin(4, desktop);
and then it forces the plasmashell window to stay on /desktop/.

> The dialogs should be positioned centered on their leader.
> However, there's also a dead-ugly Qt5 bug. It centers modal (and only
> modal!) dialogs on screen after re-mapping them (eg. shade a Qt5 modal
> dialog, unshade and whatch it jumping to the screens center....)

Yeah, this one is really annoying.

Also having this issue my bug was closed as duplicate, I could not seem to force it happen using Davids steps however I agree it sometimes happens after I open a context menu and change some settings. This is what I see and it only alters the main display. https://www.youtube.com/watch?v=ROWDp3Xot_Y

Ok, thinking it through: everything's just fine with that code.

=> What should *not* happen (in this context) is that the desktop modal is moved to a certain desktop.

The reason is that modals need to be on the (all!) VD(s) their leader is on (as they're locking their access)


Now for a "regular" client, that's no problem:

Let's say I have some kwrite window on all desktops and open a "open" dialog for it.
=> It's (supposed to be and is) on all desktops as well.

Now I unstick the modal dialog on VD #2 because I want to do something else on VD #1 and the dialog annoys me.
=> The kwrite window is unsticked as well (and on VD #2 only)

That may be unwanted, but it will be even more unwanted to have a "dead" kwrite window on VD #1, that I cannot interact with for an unknown reason.
=> I deal with the situation, open a file and may or may not re-stick the kwrite editor window afterwards (or even the dialog)



If we however apply those pattern on the (plasmashell¹) desktop, things take a bad road:
I've my desktop on all VDs, open a modal dialog, somehow unstick/move that to VD #2
=> The (plasmashell¹) desktop ends up on VD #2 (only) and there's no way for me to re-stick it.


The two questions are
a) what sets the modal dialog on a special VD ("automatically")?
b) how do we handle this?

Ad (a): any chance to add a breakpoint or an abort like below to get a backtrace to the cause?

diff --git a/abstract_client.cpp b/abstract_client.cpp
index d34cab0..27752a4 100644
--- a/abstract_client.cpp
+++ b/abstract_client.cpp
@@ -403,8 +403,11 @@ void AbstractClient::setDesktop(int desktop)
         // the (just moved) modal dialog will confusingly return to the mainwindow with
         // the next desktop change
     {
-        foreach (AbstractClient * c2, mainClients())
-        c2->setDesktop(desktop);
+        foreach (AbstractClient * c2, mainClients()) {
+            if (c2->isOnAllDesktops() && !isOnAllDesktops())
+                abort();
+            c2->setDesktop(desktop);
+        }
     }
 
     doSetDesktop(desktop, was_desk);


Ad (b):
[1] we cannot just force _every_ desktop type window to be on all desktops, because other desktop shell implementations (and at some point even plasmashell) may use different windows per VD (to allow different wallpaper etc.)
Otoh, if the modal dialog is decorated, the user is able to explicitly move it to a VD - taking the desktop there (and only there) as well.

=> As a a possible solution, plasmashell could track the VD of its desktop and set it to "-1" whenever it changes?

----
Thanks for reading until here, sorry for the textwall.

Created attachment 95076 [details]
bt when changing virtual desktop of plasmashell

(In reply to Thomas Lübking from comment #24)
> Ad (a): any chance to add a breakpoint or an abort like below to get a
> backtrace to the cause?
> 

See attachement. Btw is there a switch to disable kwin auto restart after crash?

> => As a a possible solution, plasmashell could track the VD of its desktop
> and set it to "-1" whenever it changes?

No, that's not a solution. This is regression in KWin.

No it's not, you're missing the point:
Leaving the particular case that "somehow" triggers this condition (from the backtrace it seems the client has an "info->desktop()"?, see manage.cpp:~214), there is and always /has/ been a problem.

If there's a dialog, modal for the desktop window¹, and the user sets it on a particular virtual desktop, the desktop window ends upon that virtual desktop *only* as well.

Ie. the "new" problem is that this condition gets triggered, but it's been available all the time.

kwin_x11 used to have a --nocrashhandler setting when it was KUniqueApplication, but afaics no more, sorry.

[1] I frankly didn't find one - no idea what "kicker" is supposed to be, that used to be the KDE3 panel? "kickoff" doesn't seem to have a custom image setting?

> => As a a possible solution, plasmashell could track the VD of its desktop and set it to "-1" whenever it changes? 

When reading your comment I also came to that conclusion before I reached down to where you wrote that.

Well, if it is not a regression (worked fine in 5.3 though), then it certainly is a bug in kwin (it works fine with openbox).
Why would plasmashell workaround it?

From my testing I have found that sometimes the modal dialog is correctly set to be on all desktops (same as the parent plasmashell), in which case it doesn't trigger the bug.

With kicker, I meant Application Menu. Sorry, in repo it is in applets/kicker directory.

> it works fine with openbox

that's not really an argument, sorry :-)

> Why would plasmashell workaround it?

Because any other window is allowed to change the virtual desktop of any other window. If plasmashell doesn't want that other windows set it to a specific desktop it needs to monitor itself. Making sure that KWin doesn't do that is just cosmetics.

A possible solution and what can be done in KWin is shipping a default window rule to force plasmashell to all desktops.

But this possibly breaks any window that is on all desktops and opens a modal dialog, not just plasmashell.

(In reply to Martin Gräßlin from comment #29)
> A possible solution and what can be done in KWin is shipping a default
> window rule to force plasmashell to all desktops.

+1 (thought so too)

(In reply to David Rosca from comment #30)
> But this possibly breaks any window that is on all desktops and opens a
> modal dialog, not just plasmashell.

No?
As explained, setting "regular" windows on a specific virtual desktop is far less problematic than leaving a window "inaccessible" because it's locked by a modal dialog on another virtual desktop. The desktop window is special here in that there's no GUI to control its VD and the particular desktop shell expects its windows to be on all desktops)

Also, this should really _only_ ever be a problem if a specific virtual desktop is explicitly demanded by the modal dialog. By default, the (modal) dialog (any transient) inherits the desktop from its leader (including "all desktops")

Whether this is a bug in kwin, introduced by the client abstraction level, in this very case is still to be determined (see comment #26) - the abstraction may simply expose a sync problem (ie. the modal dialog starts out with no explicit desktop set and then gets one somewhen later and previously kwin just looked "early enough™"  - I concern the "happens sometimes" aspect here)

Right now I did:

1. opened qtcreator (should not matter)
2. set it on all desktops and verified that it was set correctly
3. pressed Ctrl+O (open file or project)
4. qtcreator lost "on all desktops" and was set to current desktop

I'll try to create a minimal example app.

Does it also happen with Qt4 clients (or gtk+ or anything for that matter)?

Also not reproducible with qt-designer (Qt5) here (loads KDE platform theme + uses KDE file opening dialog)
Do you use a Qt 5.6 beta?

Qt 5.5.0 + no frameworkintegration (= standard Qt file dialog)

Cannot reproduce with anything else than Qt 5 (tried Qt 4, Gtk3, Gtk2, Firefox, FileZilla).

=> Qt5 bug.
May be ("likely") related to the "I reposition myself to the screen center on mapping" thing.

Please file a bug (I tried loading the lxqt platform theme which uses the stock Qt5 file dialog - no change)

@Martin, it may still be reasonable to disobey the VD hint for transients, since they shall show up on their leader and not shuffle that around (eg. if you work on VD2 and FF on VD1 spams "Important notice: I AM SUPER!", that should not appear on VD2, let alone moving FF2)
I assume we can ignore valid asn messages for transients as well.

diff --git a/manage.cpp b/manage.cpp
index b3b9dd7..52c9281 100644
--- a/manage.cpp
+++ b/manage.cpp
@@ -210,11 +210,12 @@ bool Client::manage(xcb_window_t w, bool isMapped)
 
             if (maincl)
                 setOnActivities(maincl->activities());
+        } else { // a transient shall appear on its leader and not drag that around, #354090
+            if (info->desktop())
+                desk = info->desktop(); // Window had the initial desktop property, force it
+            if (desktop() == 0 && asn_valid && asn_data.desktop() != 0)
+                desk = asn_data.desktop();
         }
-        if (info->desktop())
-            desk = info->desktop(); // Window had the initial desktop property, force it
-        if (desktop() == 0 && asn_valid && asn_data.desktop() != 0)
-            desk = asn_data.desktop();
 #ifdef KWIN_BUILD_ACTIVITIES
         if (Activities::self() && !isMapped && !noborder && isNormalWindow() && !activitiesDefined) {
             //a new, regular window, when we're not recovering from a crash,

This patch doesn't help anything. Actually, the bug with qtcreator is only reproducible with 5.4.2 kwin, not current git.

The case with plasmashell (opening icondialog from application menu settings) is reproducible with both 5.4.2 and git (with and without Thomas's patch).

Humm?
The backtrace ended in ::manage(), so either this patch prevents it or
a) you're in "session" mode???
b) the client is not ::isTransient() (ie. the Qt5 does not set a desktop, but is late on the transiency hint)

It is transient, but the parent is Application Menu Settings window which is just set to show on one desktop. And in the placing code (manage.cpp:194) the plasmashell desktop from mainClients() is ignored (because isSpecialWindow) so it decides to place the dialog only on one current desktop.
And then the setDesktop is called which then moves the plasmashell desktop, because now it doesn't skip specialWindows.

If the modal dialog is transient for another window (the Menu Settings window) mainClients.count() is supposed to be "1" (thus the specialWindow case not tested)

If it's not (and special window is tested and the desktop, for which the Menu Settings window lis likely transient) that means m_transientForId (controls "isTransient") and m_transientFor (controls the transientFor ;-) got out of sync, ie. we have an ID, but no attached client.

=> That smells like *big* trouble (or a stupid bug)

Yes, in the case where the bug occured, the mainClients.count() == 2 and first tested true for specialWindow and second false, so I assume the first one was plasmashell desktop and second Application Menu Settings.

The modal dialog could be a group transient (ie. modal for everything, not only the settings window; did you actually check the WM_TRANSIENT_FOR?)

In that case the behavior would "correct" again, except that the stronger (on all desktops) window should probably dominate the modals VD, regardless of its type (does the modal dialog block interaction with the desktop for you?)

---------------

diff --git a/manage.cpp b/manage.cpp
index b3b9dd7..42db5a5 100644
--- a/manage.cpp
+++ b/manage.cpp
@@ -193,7 +193,7 @@ bool Client::manage(xcb_window_t w, bool isMapped)
             for (auto it = mainclients.constBegin();
                     it != mainclients.constEnd();
                     ++it) {
-                if (mainclients.count() > 1 && (*it)->isSpecialWindow())
+                if (!(info->state() & NET::Modal) && mainclients.count() > 1 && (*it)->isSpecialWindow())
                     continue; // Don't consider toolbars etc when placing
                 maincl = *it;
                 if ((*it)->isOnCurrentDesktop())
@@ -210,11 +210,12 @@ bool Client::manage(xcb_window_t w, bool isMapped)
 
             if (maincl)
                 setOnActivities(maincl->activities());
+        } else { // a transient shall appear on its leader and not drag that around
+            if (info->desktop())
+                desk = info->desktop(); // Window had the initial desktop property, force it
+            if (desktop() == 0 && asn_valid && asn_data.desktop() != 0)
+                desk = asn_data.desktop();
         }
-        if (info->desktop())
-            desk = info->desktop(); // Window had the initial desktop property, force it
-        if (desktop() == 0 && asn_valid && asn_data.desktop() != 0)
-            desk = asn_data.desktop();
 #ifdef KWIN_BUILD_ACTIVITIES
         if (Activities::self() && !isMapped && !noborder && isNormalWindow() && !activitiesDefined) {
             //a new, regular window, when we're not recovering from a crash,

Bingo! I can't seem to break it now, so I would say this fixes it. Thanks :)

I have checked the WM_TRANSIENT_FOR, but it only contains one window id that doesn't tell me anything, sorry.
It doesn't block interaction with the desktop (but it also didn't block it before this patch).

You can check that win ID by "xprop -id 0x1234567"

Either that's the settings window or some dummy that will at best have
_KDE_NET_WM_USER_CREATION_TIME(CARDINAL) = 123456789
WM_CLIENT_LEADER(WINDOW): window id # 0x1234567

In the latter case, we'd have a group transient that is not actually a group transient.

xprop -id 0x1c00002
_KDE_NET_WM_USER_CREATION_TIME(CARDINAL) = 47123577
WM_CLIENT_LEADER(WINDOW): window id # 0x1c00002

Falsely set group-transient then.

Git commit 7339e9639ff53955862da74e48d5440ff465dbca by Thomas Lübking.
Committed on 23/10/2015 at 07:37.
Pushed by luebking into branch 'Plasma/5.4'.

Improve virtual desktop selection for transients

a) When a group-transient is modal, it still needs to be
   on the current or all virtual desktops if *any* of the
   blocked clients is
FIXED-IN: 5.5

b) ignore demanded virtual desktop for transients. Notably modal transients
   should appear where their parent is, and not drag that around. All others
   also better show up above their parent and not a distant virtual desktop

REVIEW: 125758

M  +9    -6    manage.cpp

http://commits.kde.org/kwin/7339e9639ff53955862da74e48d5440ff465dbca

Notice that
* if you've a dialog modal for the desktop, you can "explicitly" still move the latter to a single virtual desktop with the modal (plasmashell problem, might be resolved with a kwin rule)

* the dialog should be modal for the settings dialog, but not the window group
   This is either a bug in plasmashell (or rather Qt, i doubt plasmashell will explicitly manipulate the transient_for property) or kwin (if the settings window and the modal dialog have disjunct WM_TRANSIENT_FOR / WM_CLIENT_LEADER values, ie. are not actually the same group)

I think the problem is how Qt 5 creates file dialogs, we had quite some issues with it in frameworkintegration too.

Basically it creates 2 dialogs, first with Qt::WA_DontShowOnScreen and second actual dialog that is parented to the first. For more info, please see https://git.reviewboard.kde.org/r/125208/

*** Bug 354264 has been marked as a duplicate of this bug. ***

*** Bug 354526 has been marked as a duplicate of this bug. ***

*** Bug 354591 has been marked as a duplicate of this bug. ***