Compatibility with Fullscreen Avoider

[Lythenas]

Fixes #703

This adds support for the Fullscreen Avoider extension: https://github.com/Noobsai/fullscreen-avoider This extension moves the topbar to a secondary monitor (if avilable) if the primary monitor has a fullscreen window.

The main functionality is working so it is technically usable, but there are still some issues:

• Topbar overlaps with window position bar on secondary monitor
• Topbar on seconary monitor still shows information from the original monitor
• Sometimes there is additional spacing when the topbar is on a secondary monitor
• I stopped hiding the topbar, not sure if there are any edge cases now but it seems to work fine for me even without Fullscreen Avoider
• fixTopBar is not effectively useless and could be removed
• topbar get's darker when it is on secondary monitor and that monitor gets focused
• check what happens when monitors change (unplug, replug)
• when switching workspace the wrong workspace is shown in the topbar until you focus another monitor and focus back
• There is extra space below the topbar after switching workspaces (corrected after focusing other monitor and focusing back)

[jtaala]

@Lythenas, that this type of patching needs to be done in patches.js and using the patterns there, otherwise this wouldn't pass EGO review) - see examples there for patching prototypes (which will then be automatically reverted on PaperWM disable).

But in principal I can see what you've done here.

[jtaala]

All good though - we can do that after.

[Lythenas]

Didn't know about the patches.js file. Will move it there.

But I think we can't use the pattern there, since monitor.prototype is undefined. Also I am not overwriting a normal function or property. I am setting a getter function for a property. And since that does not exist not sure how to restore it. I guess we would just safe the undefined.

Edit: I think the code in patches works directly on classes, right? So I don't think we can use it since monitor are just plain JS objects as far as I can tell.

The prototype seems to apply to all monitor objects but no other objects for some reason. I also don't know what happens when you plug in or unplag a monitor afterwards or change the monitor configuration. It is possible that the prototype will be lost when the objects change.

Maybe there is a better way to patch this but that is what I found works.

[Lythenas]

Also note that fullscreen avoider does some interesting stuff for patching by calling toString on the function and string replacing some things: https://github.com/Noobsai/fullscreen-avoider/blob/master/src/extension.js#L91

[jtaala]

I see, given what we're doing in this case, we can add it into Utils.js upgradeGnomeMonitors:

https://github.com/paperwm/PaperWM/blob/release/utils.js#L526-L536

This is nice since it's a dbus service hook that will get called whenever monitors change (e.g. if we add new monitors and remove monitors). We use this to upgrade monitor objects to add connector information (e.g. HDMI-1, eDP-1 etc.).

[jtaala]

anyway, can leave as is for the time being (since it's working fine) and can focus on the other issues (that you mentioned).

[jtaala]

Edit: I think the code in patches works directly on classes, right? So I don't think we can use it since monitor are just plain JS objects as far as I can tell.

No, Monitor is indeed a class - it's here:
https://gitlab.gnome.org/GNOME/gnome-shell/-/blob/main/js/ui/layout.js?ref_type=heads#L158-L171

You're essentially overriding the prototype class getter get inFullscreen().

Interestingly, Gnome haven't exported this so that's the main reason why we can't use it directly in patches. You're approach of getting a concrete implementation of one and overriding the prototype (which effects the Monitor class) will work though.

[Lythenas]

Ah ok. I thought is is not a class because it only shows up as an [object Object] in looking glass. But .constructor.name indeed returns Monitor.

[jtaala]

I'll try have a look at this tonight. Cheers.

[Lythenas]

My code in patches.js probably needs to be reorganized, but I first want to know what to do with the prototype overrides of the monitor objects. In tiling.js we already have a monitorChanged listener that also adds a property clickOverlay to the monitor object. https://github.com/paperwm/PaperWM/blob/release/tiling.js#L1939

We could also set the inFullscreen getter there (instead of on the prototype). Maybe that would be easier to pass the EGO review since we already have similar code there.

[jtaala]

We could also set the inFullscreen getter there (instead of on the prototype). Maybe that would be easier to pass the EGO review since we already have similar code there.

That would also work - for EGO the rule is "if you add or remove anything to a gnome object (prototype or not), then you must undo that when disabling your extension)".

[Lythenas]

That would also work - for EGO the rule is "if you add or remove anything to a gnome object (prototype or not), then you must undo that when disabling your extension)".

Since you pointed out it is indeed a class and not just an object, I think it fits better in patches.js. There it is also easier to undo the changes. But as you said, I will probably leave this for now and focus on the other issues first.

[Lythenas]

It still needs some cleanup but I think this PR is ready for testing. See the open todos in the PR description for things to look out for.

check what happens when monitors change (unplug, replug)

when disconnecting the second monitor (where the topbar was moved to) and reconnecting it, the topbar stays on the primary monitor that has a fullscreen window. But that might be a limitation of Fullscreen Avoider, since it only does something on in-fullscreen-changed signals, which probably don't get triggered when a display is connected. Un-fullscreening and re-fullscreening fixes the issue.

@jtaala should we also mention the compatibility with fullscreen avoider in the readme?

[jtaala]

@jtaala should we also mention the compatibility with fullscreen avoider in the readme?

Well, we could just list it in (with a few comments) in the end Recommended extensions section.

[jtaala]

Cool, will start testing this tomorrow (my Saturday here in Australia).

[Lythenas]

When running games (through Steam) I noticed the following:

• Sometimes (not always the same, not even for the same game...) it does not immediately detect that there is now a fullscreen window. Fixed by either focusing another monitor and then focusing back, or changing the game to windowed and back to fullscreen. I will try to debug this a bit further.
• When focusing the other monitor some games minimize. But the primary monitor is still considered fullscreen. This is fine in my opinion.
  • Sometimes the topbar also jumps back to the primary screen and is hidden once you tab into the game again.

[Rutherther]

Today I experienced a bug when having only one monitor. When I was changing workspaces, the workspace name stayed the same in the top bar. And also some windows didn't want to get tiled. I am not sure whether it's related to this branch or unrelated bug. I might try investigating, replicating during the weekend.

Could also be an issue with one of the other extensions I use (or combination of the extensions)

[Rutherther]

When focusing the other monitor some games minimize. But the primary monitor is still considered fullscreen. This is fine in my opinion.

I learned some time ago to just disable fullscreen in the game video settings, make it windowed, and fullscreen manually with WM/DE. For most of the games I tried it worked better in general with tiling WMs.

[Rutherther]

Okay, when I have more monitors, and I swap workspace between them, I get two topbars on main monitor. And the window shifts down, looks like there is space for two top bars. I have to trigger some sort for update, like changing width of window to fix.

I am swapping with "Swap workspace with monitor N" from PaperWM

[Lythenas]

When I was changing workspaces, the workspace name stayed the same in the top bar.

Okay, when I have more monitors, and I swap workspace between them, I get two topbars on main monitor. And the window shifts down

I also noticed these issues. I added them to the list in the description of the PR.

I think I just need to trigger the update more often. I will try to fix this tomorrow.

[Lythenas]

• when switching workspace the wrong workspace is shown in the topbar until you focus another monitor and focus back

• There is extra space below the topbar after switching workspaces (corrected after focusing other monitor and focusing back)

This should be fixed now.

[Rutherther]

And also some windows didn't want to get tiled.

Sorry about this, I don't think that was right. They were getting tiled, it was just the spacing as you also noticed, and I reported again in the next reply.

This should be fixed now.

Thank you for these new commits, seems like that fixed the issues I reported.

I noticed one more issue, nothing critical. When I fullscreen something on any of the monitors, move cursor to another monitor, I get an animation on the primary like the panel was going away... Ie. the window gets immediately smaller, to make space for the bar, and then this space gradually disappears - like an animation

[Lythenas]

@jtaala note about the changes in extension.js with setting global.paperwm. We can undo them if that is required for EGO, but it was really useful to be able to call e.g. global.paperwm.Tiling.spaces.spaceOfIndex(0).monitor in looking glass to inspect our internal state. I didn't find another way to get at this since it is now all hidden in modules since gnome 45.

[Lythenas]

I believe most bugs should be fixed now. @rutheR @jtaala Can you try this out again?

[jtaala]

I believe most bugs should be fixed now. @rutheR @jtaala Can you try this out again?

Will do!

[Lythenas]

Merging the develop branch seems to have broken some things. I will look into it later.

[Lythenas]

I think I fixed the issues, but now there is some "jumping" of the windows on the secondary monitor when fullscreening on the primary. The reason for this is that we do multiple layouts but the first does not yet know that the topbar moved. I'm not sure how to fix that.

Also I was only having this issue on my PC, I couldn't reproduce it in the VM. Also not after changing which monitor is the primary.

[jtaala]

Thanks @Lythenas,

I've been working on that other PR (#741) and haven't looked at this yet. I've really been waiting for @Rutherther to test this PR since I don't use that extension and @Rutherther seems to have use cases to test against.

but the first does not yet know that the topbar moved. I'm not sure how to fix that.

Happy to have a look. I've made some changes in #741 which fix some layout timing issues (mainly for window resizing though) so once I merge that in I'd be happy to have a look here (will install fullscreen avoider etc.).

[Lythenas]

@jtaala I expect that there are merge conflicts with this and #741 anyway, since we modify some of the same code. So I will wait until that is done and merged and check again after that.

[Rutherther]

Hello,
yes, I have use cases, and I am actually using this branch daily right now, because it would be unbearable without it for me. I haven't pulled the rebased version, so I will do that and test on that as well.

I know of one bug, but I cannot reproduce it reliably. Sometimes the bar stays on the other monitor, I have to fullscreen and defulscreen something on the primary monitor to get it back. But I don't know how it ended on the second monitor. I am also not sure if this is bug coming from Fullscreen Avoider, or from some change from PaperWM

[Rutherther]

Okay so I am trying the newest version of this branch. The problem is that I am getting the half blackscreen bug described in #638 on every fullscreen now. I have tracked it back to this commit 4d829a8. Before that, it wasn't being triggered, after that, it is.

[Rutherther]

I commented out the line added in 4d829a8 for now. Without that, I got the blackscreen part bug on the first fullscreen on login, but not on the next ones. I will test like this for now and let you know of any bugs if I encounter any.

Seems like "Queue layout after panelBox position changed" is supposed to fix hide top bar, I don't use that so I hope that commenting out the line shouldn't break anything else for me.

[Lythenas]

I am also getting the half blackscreen bug, but not in anything I care about^^. And I think in some cases it is actually fixed by the queued layout (because it is briefly shows black and then fixes itself). To me this looks like the half blackscreen bug could be triggered by to many layout calls. And I think we can (and probably should) cut down on those. As said before I will wait for #741 and then try to improve this PR.

But thank you for testing. It's good that it at least mostly works. I also noticed that it sometimes behaves a bit differently when running in an Ubuntu 23 VM and when running on my Arch desktop.

[jtaala]

To me this looks like the half blackscreen bug could be triggered by to many layout calls.

Yeah, in my testing for #741, I've found something common between the apps that cause the half blackscreen bug - they all send multiple fullscreen change signals. For example, osu! sends 8-12 of these change singals during it's startup. Each of these signals triggers the resize_handler, which in turn triggers a layout. #741 helps here quite a bit since we conditionally call queulayout (and we also use a Meta.Later to push the queuelayout excecution into the gnome resize phase (instead of immediately).

Patch 3401 stops the blackscreen bug from occurring - but the changes in #741 will cut down on layout calls either way.

[Lythenas]

For now I only merged develop and resolved the conflicts. I will try to look into the remaining issues tomorrow probably.

[Lythenas]

I think I fixed the issues, but now there is some "jumping" of the windows on the secondary monitor when fullscreening on the primary. The reason for this is that we do multiple layouts but the first does not yet know that the topbar moved. I'm not sure how to fix that.

The jumpiness is fixed now and I believe most things work fine now. But I need to test more for some edge cases.

Feel free to also test if you have time. I think it should be fine to use.

[jtaala]

I think I fixed the issues, but now there is some "jumping" of the windows on the secondary monitor when fullscreening on the primary. The reason for this is that we do multiple layouts but the first does not yet know that the topbar moved. I'm not sure how to fix that.

The jumpiness is fixed now and I believe most things work fine now. But I need to test more for some edge cases.

Feel free to also test if you have time. I think it should be fine to use.

@Rutherther - could you run this through your various use cases and provide feedback here?

Thanks all!

[Rutherther]

Alright, I am trying to test the newest version now. I found out one problem. I am not sure if this was a problem even before, but I noticed it just now.

When I fullscreen on my main monitor, and move the fullscreen window to the monitor with the topbar, the topbar doesn't move back to the main monitor. It gets hidden instead. Not even sure if this could be PaperWM issue (with fullscreen detect function patch I would suppose), or Fullscreen Avoider issue.

[Lythenas]

When I fullscreen on my main monitor, and move the fullscreen window to the monitor with the topbar, the topbar doesn't move back to the main monitor. It gets hidden instead. Not even sure if this could be PaperWM issue (with fullscreen detect function patch I would suppose), or Fullscreen Avoider issue.

I can reproduce that. I will look into it. I'm guessing this is a race condition. I.e. fullscreen-changed is fired when the fullscreen window moves but PaperWM state has not updated yet, so it still considers the old workspace fullscreen instead of the new one.

I think something similar happens when you minimize a fullscreen window and then maximize it again. Probably mostly an issue with games. That happens with Into the Breach because it minimizes when you switch monitors.

[Lythenas]

There are probably also other edge cases that cause problems. Probably swapping workspaces and things like that. I also need to properly test again if nothing broke when Fullscreen Avoider is disabled.

[Lythenas]

Just an update for this: For many cases this PR works fine. However for some games I noticed edge cases that I can't figure out. Mostly there are two things:

1. Topbar is hidden but the fullscreen window starts below the topbar
2. The topbar does not move back after the fullsceen window is closed

I am unsure if this can be properly fixed with the current approach. I currently also do some trickery and resend the "in-fullscreen-changed" signal. This is kind of hacky and it might be better to just reimplement FullscreenAvoiders functionality in PaperWM.

In any case for me I found it more useful to just set the primary monitor in gnome to my "secondary" monitor, since I only really use fullscreen windows (games) on one monitor.

I'm converting this to a draft, since I probably won't finish this PR. If someone else wants to take over I would be happy to review it and test it out.

[jtaala]

Thanks for your work on this @Lythenas!