fix(window-state): compare position with available monitors bounds

[ImUrX]

When you disconnect the main monitor, your secondary one may become the main one and change it's position. Which makes the window go out of bounds.
I added a position comparison to check if the position is the same as before. I also added a size check which would only be necessary to check if the resolution of the main monitor has changed because the position is always x0y0

[Beanow]

For future generations:

• Remember monitor data tauri-plugin-window-state#52
• GUI stays on second monitor even if it doesn't exist SlimeVR/SlimeVR-Server#433
• https://discord.com/channels/616186924390023171/1058076647196205156

[Beanow]

@ImUrX going to have a look.
Am I right that this is the main reproduction of the issue I should be looking at?
SlimeVR/SlimeVR-Server#433 (comment)

Did you find any other interesting edge cases / problems?

[Beanow]

So far I've not reproduced the original issue.
At least on (X)Wayland monitors are named, and adding/removing them is detected by name.

Current = "XWAYLAND6"
Monitor { name: Some("XWAYLAND6"), size: PhysicalSize { width: 2560, height: 1440 }, position: PhysicalPosition { x: 0, y: 0 }, scale_factor: 1.0 }
Monitor { name: Some("XWAYLAND8"), size: PhysicalSize { width: 1280, height: 720 }, position: PhysicalPosition { x: 2560, y: 0 }, scale_factor: 1.0 }

Whenever I try to use something else (like changing resolution) to place the app window out of bounds, it gets correctly placed to a sensible location.

I've yet to try on Xorg though, did you perhaps test on Xorg?

[ImUrX]

I’ve only reproduced this on windows

[Beanow]

@elibroftw and @amrbashir were the last to look at this section.
Perhaps they can help review 😄

Keep in mind I've not been able to reproduce the issue yet with either

tauri-plugin-window-state = "0.1.0"

Or

tauri-plugin-window-state = { git = "https://github.com/tauri-apps/plugins-workspace", rev = "refs/heads/dev", package = "tauri-plugin-window-state" }

Because I tested on Linux. Haven't got Windows set up atm.

[Beanow]

This may need a refactor for readability soon.
Essentially we're invalidating the cache, based on a more detailed fingerprint of what the monitor setup was when we saved the state. Vs when we're restoring the state.

Importantly we're only ever checking if it's exactly equal to the aspects we fingerprinted.

[ImUrX]

and thats okay, the problem is that for some reason Window lets you go out of the physical screen.
I mainly use linux too but I dont have a dual monitor setup on it

[elibroftw]

Is the issue this PR fixes that if I disconnect a monitor while using the app, the app is off screen instead of being moved to the remaining monitor?

[ImUrX]

No

[elibroftw]

What's the exact reproduction steps for the issue this fixes then?

[ImUrX]

The plugin that I'm fixing is about saving window states, so it's related to when the application is first executed. Not while it's running

[elibroftw]

I'm asking what the reproduction steps are. I have a multi monitor setup which is why I'm asking. I'll let someone else review this then.

[ImUrX]

I'm sorry I been trying to repair something related to embedded and I'm at the end of my patience.

You need to close the app inside your secondary monitor, then disconnect the main monitor and then open the app again. Without this fix, it will be opened outside of the monitor because the position of your secondary monitor changed.

[Beanow]

@amrbashir were you able to reproduce / confirm the fix?

[amrbashir]

Yes, the problem seems to be that monitor.name will be \\\\.\\DISPLAY1 for the main monitor and will be \\\\.\\DISPLAY2 for the secondary monitor, but after disconnecting the main monitor, the secondary monitor will still NOT be considered the main monitor and so it will keep the name \\\\.\\DISPLAY2 which will match to our last saved monitor but even if the name hasn't changed, the position of the monitor has changed which will result in placing the window in the position saved last time which is out of the current monitor position.

[Beanow]

So it looked similar on XWayland, though I didn't see the issue as Wayland just seems to detect this off screen placement and move it around for you.

The removing of monitors didn't change the numbers. It's re-initializing them that did increment display numbers. So initially my setup starts as: XWayland1 & XWayland2. But after enough adding and removing eventually got to: #48 (comment)
XWayland6 & XWayland8. 😆

If this is the same on other OSes, using this name as an identifier / fingerprint may be fundamentally flawed. As the number seems more of an accidental result of the initialization order of displays.

Same as how using /dev/sda1 as a device reference for like fstab is considered bad practice and you should use UIDs instead.

[amrbashir]

Correct, but unfortunately the current monitor api is limited and doesn't provide a hardware id (not sure if it possible at all).

[Beanow]

@amrbashir in that case may be back to the drawing board.

At least for this particular issue I can think of another approach.
(On Windows) we should never place windows outside of the available space.
We can also check what the bounds are just from the current setup, no persistence at all, and verify against that.

But I think there may have been another case which the previous PR addressed?
tauri-apps/tauri-plugin-window-state#48

[amrbashir]

(On Windows) we should never place windows outside of the available space.

yeah, that's what we are trying to do, we are trying to see if the last saved monitor exists by comparing the name (with this PR, also the position) and use the saved position if it does but if it doesn't, let it for the OS defaults.

We can also check what the bounds are just from the current setup, no persistence at all, and verify against that.

Could you elaborate more on that, not sure I got exactly what you mean? Do you mean only restore the position if there is a monitor bound that could contain it?

[Beanow]

Could you elaborate more on that, not sure I got exactly what you mean? Do you mean only restore the position if there is a monitor bound that could contain it?

Yes, the current monitor properties at state restoration time can be checked.

Monitor { ... size: PhysicalSize { width: 2560, height: 1440 }, position: PhysicalPosition { x: 0, y: 0 }, scale_factor: 1.0 }
Monitor { ... size: PhysicalSize { width: 1280, height: 720 }, position: PhysicalPosition { x: 2560, y: 0 }, scale_factor: 1.0 }

If we have an extension like monitor.contains(PhysicalPosition) and we iterate over the monitors, we can detect an out-of-bounds placement without persisting monitor properties at all.

[amrbashir]

I think I used this solution at some point before but can't remember why I decided to just use the monitor's name. Let's try this approach anyways and keep an eye on it if it introduces any regressions.

[amrbashir]

Pushed! @Beanow @ImUrX @elibroftw could you all give this PR another test and see if there is any unwanted behavior with my last commit?

[Beanow]

I believe the way to do so is use this as your cargo depencency:

tauri-plugin-window-state = { git = "https://github.com/tauri-apps/plugins-workspace", rev = "refs/pull/48/head", package = "tauri-plugin-window-state" }

@amrbashir of course Linux never had the issue but I'll poke at it see if there's anything that seems to misbehave.

[Beanow]

So I can kind of see the intended behavior is working.
Knowing that, if I send the OS an out of bounds position, it will attempt to do a "nearby" placement. While if the plugin invalidates the position and asks the OS to do a default placement, it'll be "somewhere top-lefty".

And I can confirm that if the target position still exists it will go there. While it will be reset to a default placement otherwise. Regardless of why that position became unavailable. So now it's also robust against resolution changes, monitor offsets, and not "forgetting" a placement if the monitor number happens to change.