Closed Bug 1620101 Opened 3 years ago Closed 3 years ago

Session restore to virtual desktop does not work on KDE Plasma

Categories

(Firefox :: Session Restore, defect, P2)

75 Branch
Unspecified
Linux
defect
Points:
3

Tracking

()

VERIFIED FIXED
Firefox 75
Iteration:
76.1 - Mar 9 - Mar 22
Tracking Status
firefox75 --- fixed
firefox76 --- fixed

People

(Reporter: johnp, Assigned: mikedeboer)

References

Details

Attachments

(2 files)

With KDE Plasma 5.17.5 on Fedora 31 and the latest Nightly containing changeset 323e2a212629, session restore does not save the workspaceID to sessionstore.jsonlz4.

Using xprop I‌ see the correct _NET_WM_DESKTOP property. I‌ verified the runtime dynamically linked libgdk-3.so.0 has the required symbols and I don't see a "... not available" log message with MOZ_LOG='Widget:10' (not sure if this is correct).

I‌ was hoping this would just work on Plasma since the implementation looks DE-generic :/

Hm, that's unfortunate! So I was hoping it'd magically work too! So maybe this would probably work properly when using properties, which the gdk_x11_window_*_desktop methods are most likely using under the hood. I tried looking for the GDK source and the implementation of these library methods, but my Google-skills failed me somehow.
I opted for using these methods, because getting & setting X11 properties looks very cumbersome, compared to using a GDK library method.

I'd be happy with help/ pointers here!

Also note that the current implementation doesn't work with Wayland. It may, if Wayland has a shim implemented, but that seems unlikely to me.

That's just it, I‌ checked and the gdk implementation simply uses _NET_WM_DESKTOP internally. My google-skills also failed me until I‌ stumbled upon this commit (I‌ haven't found an official changelog / release note entry explaining the API/ABI change). The gdk_x11_window_*_desktop functions are still in the 3.24 (just the version my Fedora 31 has) gtk branch in this file. I suppose Google doesn't index old GitHub branches and GitHub search also doesn't seem to index those and additionally, the current GTK master, which is indexed, doesn't carry a deprecation note or anything whatsoever pointing to the rename I‌ linked to initially. Fwiw, XWayland does seem to correctly set the properties, so your implementation should also work in that specific case. Let's leave a native Wayland implementation to a future bug, if it turns out to be necessary.

Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 75.2 - Feb 24 - Mar 8
Points: --- → 3
Priority: -- → P2

Running from gdb (after installing all the debuginfo-packages gdb asked for) and breaking on gdk_x11_window_get_desktop I see that XGetWindowProperty returns 0 in all return variables:

(gdb) n
get_netwm_cardinal_property (name=0x7ffff656e2f8 "_NET_WM_DESKTOP", window=0x7fffb4dea3e0) at gdkwindow-x11.c:2211
2211      if (type == XA_CARDINAL)
(gdb) list
2206                          GDK_WINDOW_XID (window),
2207                          gdk_x11_get_xatom_by_name_for_display (GDK_WINDOW_DISPLAY (window), name),
2208                          0, G_MAXLONG,
2209                          False, XA_CARDINAL, &type, &format, &nitems,
2210                          &bytes_after, &data);
2211      if (type == XA_CARDINAL)
2212        {
2213          prop = *(gulong *)data;
2214          XFree (data);
2215        }
(gdb) print type
$5 = 0
(gdb) print nitems
$6 = 0
(gdb) print format
$7 = 0
(gdb) print data
$8 = (guchar *) 0x0
(gdb) print bytes_after
$9 = 0

Unfortunately I'm far out of my depth here and this looks like maybe an X11/KWin(?) issue? I can step into the XGetWindowProperty function, but Firefox and GDB frequently hangs there until I send a SIGINT (interrupts __lll_lock_wait (futex=futex@entry=0x7ffff7900020, private=0) at lowlevellock.c:52; edit: debugging Firefox XWayland from a Wayland session running konsole with gdb natively works around the issue).

I manually returned different values > 0 from gdk_x11_window_get_desktop and was able to see the workspaceID being persisted to sessionstore.jsonlz4, but restoring from that session also didn't restore to the correct virtual desktops, so the restore path is also broken :(

(I also saw a "warning: Corrupted shared library list: 0x7fffa5d8e000 != 0x7fffa0f5b800" by gdb once, but that seems unrelated?)

I think I got it!

Stumbling around in gdb, I replaced window with window->parent within get_netwm_cardinal_property and got the correct return value.
So it seems that under KDE Plasma child windows don't inherit properties, while under Gnome (I assume that's what this was tested on) they do inherit them. Not sure what the spec says, if there even is one. It could also be that the window structure under Gnome is flatter (under Plasma, window->parent and window->parent->parent both exist and only window->parent has the property).

This also likely means that the workspaceID has to be restored to that parent window as well.

Furthermore, under i3 the current implementation kind of works (i3 assigns virtual desktops not according to workspace numbers, but in order of opened workspaces, because there can't be gaps in the EWHM virtual desktops, so if workspaces are closed/empty the numbering changes) and window->parent->parent is a null-pointer, so the layout is flatter there.

We can get the correct window via gdk_window_get_toplevel / gdk_window_get_effective_toplevel (not sure which one is more correct here), both return window->parent on KDE Plasma and just window on i3. This brings to question whether we are using the wrong mGdkWindow elsewhere in Firefox or if this the only place were it matters...

Ayayay, I noticed your comments only after spending a significant amount of time searching how to make things work by using freedesktop.org properties.
I somehow ended up over at https://github.com/brummer10/gxtuner/blob/792d453da0f3a599408008f0f1107823939d730d/deskpager.cpp#L50 when I noticed that simply setting the property didn't work. Indeed I'm running stock Ubuntu + Gnome and saw that a 'hack' might be possible.
So now I'm sending an event and it works like a charm. I'm keeping my fingers crossed for KDE and pals! (Please note that I'm a fan of KDE and have used it a lot before, albeit many years ago.)

Thanks for your investigation, Johannes, I hope you'd be able to test the resulting patch?

So when I simply set the property on the window and flushed the display, I didn't
manage to get the desired effect.
Then I found this 'gxtuner' project that sends an event, which is correctly
picked up by Gnome and hopefully others.

Pushed by shindli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5923bf78df55
Changing over to using freedesktop.org window properties to implement support for virtual desktops. r=stransky

I'll check out the autoland build, but I suspect gdk_screen_get_root_window will return the wrong window (I just checked in my gdb session open from yesterday and it returns mGdkWindow->parent->parent under Plasma, which doesn't have the property. Only the window "in-between" (as in mGdkWindow->parent) (whatever gdk means with "toplevel" I suppose, which is a different concept from "root" window apparently) carries the property there.

edit: the autoland build doesn't make it work :/

edit2: Disregard my first sentence; I looked at the GdkX11ScreenGetNumberOfDesktops/MoveToWorkspace functions. The GetWorkspaceID is still broken in KDE Plasma, because it operates on mGdkWindow instead of gdk_window_get_(effective?)_toplevel(mGdkWindow).

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 75

Issue is not fully fixed yet (see comment 8).

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

I also verified that MoveToWorkspace as well requires the toplevel window within the xevent, as in:

xevent.xclient.window = GDK_WINDOW_XID(gdk_window);

does not work, but replacing gdk_window with gdk_window_get_toplevel(gdk_window) via gdb moves the window to the correct workspace.

(In reply to Johannes Pfrang [:johnp] from comment #11)

does not work, but replacing gdk_window with gdk_window_get_toplevel(gdk_window) via gdb moves the window to the correct workspace.

That sounds fantastic! So I'd be happy to submit a patch that does this and I'll test it on Gnome too. It's really great to hear that this works on KDE for sure - thanks!!

No problem, I'm happy to finally see this feature being worked on. Just to re-iterate: The same applies to the gdk_property_get call within nsWindow::GetWorkspaceID. It only works under KDE Plasma if it's being passed the toplevel GdkWindow (tested via gdb as well).

This patch also tweaks the behavior on Ubuntu Unity slightly to work with the
adaptive workspaces.

Iteration: 75.2 - Feb 24 - Mar 8 → 76.1 - Mar 9 - Mar 22
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/efee09a6eb39
Apply the _NET_WM_DESKTOP property on the root window instead, to make workspace switching work on KDE and others. r=stransky
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED

Comment on attachment 9132621 [details]
Bug 1620101 - Apply the _NET_WM_DESKTOP property on the root window instead, to make workspace switching work on KDE and others. r?stransky

Beta/Release Uplift Approval Request

  • User impact if declined: Restoring windows to the correct virtual desktop should work on all Linux desktop environments, but on 75 I was only able to make it work properly on Gnome and Unity. This patch adds support for any other, like KDE, including improved support for Unity adaptive virtual desktops.
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's not risky, since this patch reduces the complexity of the existing implementation.
  • String changes made/needed:
Attachment #9132621 - Flags: approval-mozilla-beta?
Attachment #9131312 - Flags: approval-mozilla-beta?

Verified fixed on Nightly. Thanks!

Status: RESOLVED → VERIFIED

Comment on attachment 9131312 [details]
Bug 1620101 - Changing over to using freedesktop.org window properties to implement support for virtual desktops. r?stransky

make restoring windows on the right virtual desktop work in more environment, approved for 75.0b4

Attachment #9131312 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9132621 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-75b-p2]

THANK YOU SO MUCH!!!!

I've seen it in the trunk builds I made, and I was so happy.

Just last week I thought "Ression restore considering virtual desktops would be my biggest wish for Firefox right now". And a week later, it worked. Can you imagine how happy I was?

Thank you so much!

If we see each other in person, I'll buy you a meal.

Consider yourself hugged virtually.

THANKS SO MUCH!!!!

This was really great and kind. Thank you :-)

Ben

ヽ(•‿•)ノ

You need to log in before you can comment on or make changes to this bug.