Prioritize reusing existing virtual monitors when setting video layout. [chromium/src : main]
Jamie Walch (Gerrit)
Jamie Walch added 1 comment![]( "Open in Gerrit")
auto it = available_screen_ids.begin();I'm not sure if it's worth trying to pick the id that corresponds to the monitor with greatest overlap with the new one or not. The 99% use-case for this is restoring a 2-display layout that was saved by the display layout dialog immediately after adding a display, and therefore has no id for that display. For this use-case the extra code would be overkill.
If we wanted to pick the "best" display, is there sufficient information in `preferred_monitors_config_`?
Related details
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang added 2 comments![]( "Open in Gerrit")
auto it = available_screen_ids.begin();I'm not sure if it's worth trying to pick the id that corresponds to the monitor with greatest overlap with the new one or not. The 99% use-case for this is restoring a 2-display layout that was saved by the display layout dialog immediately after adding a display, and therefore has no id for that display. For this use-case the extra code would be overkill.
If we wanted to pick the "best" display, is there sufficient information in `preferred_monitors_config_`?
Do you mean with greatest overlap in desktop geometry?
Am I correct that the CUJ you try to fix here is that when the user connected, they didn't immediately restore the previous display layout (which should just work since it is requesting a new display), but they later added a new display and decided to restore the previous display layout? It sounds like this might be easier to achieve on the client side (i.e. the client finds the best matching existing display for a non-existent display in the stored layout)? But in any case, it doesn't seem like we gain much by matching the display with the greatest overlap.
If we wanted to pick the "best" display, is there sufficient information in preferred_monitors_config_?
It probably has. It has the expected sizes and positions of all the displays that are managed by CRD.
// SetResolutionAndPosition() calls below know whether they should be`streams_being_removed_` is now set after the `SetResolutionAndPosition` calls at line 294. Based on the comment on [do_after_stream_removal_](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/linux/gnome_desktop_resizer.h;l=167;drc=9b82a5f081c62c4d20cdb0dd996c5504097f1dbf), if you first change the resolution of stream A then immediate remove stream B, then GNOME has a chance to crash. Is this no longer reproducing in GNOME 49?
Maybe a safer approach is to queue up the `SetResolutionAndPosition` calls (as a vector of callbacks?) then call them after this block.
Related details
- Jamie Walch
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jamie Walch (Gerrit)
Jamie Walch added 2 comments![]( "Open in Gerrit")
auto it = available_screen_ids.begin();Yuwei HuangI'm not sure if it's worth trying to pick the id that corresponds to the monitor with greatest overlap with the new one or not. The 99% use-case for this is restoring a 2-display layout that was saved by the display layout dialog immediately after adding a display, and therefore has no id for that display. For this use-case the extra code would be overkill.
If we wanted to pick the "best" display, is there sufficient information in `preferred_monitors_config_`?
Do you mean with greatest overlap in desktop geometry?
Am I correct that the CUJ you try to fix here is that when the user connected, they didn't immediately restore the previous display layout (which should just work since it is requesting a new display), but they later added a new display and decided to restore the previous display layout? It sounds like this might be easier to achieve on the client side (i.e. the client finds the best matching existing display for a non-existent display in the stored layout)? But in any case, it doesn't seem like we gain much by matching the display with the greatest overlap.
If we wanted to pick the "best" display, is there sufficient information in preferred_monitors_config_?
It probably has. It has the expected sizes and positions of all the displays that are managed by CRD.
Do you mean with greatest overlap in desktop geometry?
Am I correct that the CUJ you try to fix here is that when the user connected, they didn't immediately restore the previous display layout (which should just work since it is requesting a new display), but they later added a new display and decided to restore the previous display layout?
When saving a layout proto on the client, each display may come from the host, or it might be a "pending" display that only exists in the layout config dialog. The difference is basically whether or not the user added a display before checking the "restore layout" option. Both cases have a problem where the current state of the host might not match what it was when the setting was saved:
- If the display came from the host, it might not exist any more. In that case, https://chromium-review.googlesource.com/c/chromium/src/+/7557558 takes the view that although we can't guarantee the display id in the request, we can at least create a display with the right size and position.
- If the display was pending when the proto was saved, but now exists, then without this CL we will delete the previous display and create a new one. It will have the right size and position, but the window manger might not restore move windows back onto it.
It sounds like this might be easier to achieve on the client side (i.e. the client finds the best matching existing display for a non-existent display in the stored layout)?
We don't want to wait for the host to send its initial layout before replacing it with the saved layout. It can be fixed on the client, but only by setting a "layout pending" flag, and saving the next layout that the host sends (which will have the correct display ids). That seems more complex and error-prone, which is why I preferred this host-side approach.
// SetResolutionAndPosition() calls below know whether they should be`streams_being_removed_` is now set after the `SetResolutionAndPosition` calls at line 294. Based on the comment on [do_after_stream_removal_](https://source.chromium.org/chromium/chromium/src/+/main:remoting/host/linux/gnome_desktop_resizer.h;l=167;drc=9b82a5f081c62c4d20cdb0dd996c5504097f1dbf), if you first change the resolution of stream A then immediate remove stream B, then GNOME has a chance to crash. Is this no longer reproducing in GNOME 49?
Maybe a safer approach is to queue up the `SetResolutionAndPosition` calls (as a vector of callbacks?) then call them after this block.
Callbacks seemed a hit heavyweight, so I refactored it a bit to preprocess the displays instead so that things should still happen in the same order as without this CL.
Related details
- Yuwei Huang
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yuwei Huang (Gerrit)
Yuwei Huang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yuwei Huang
| Code-Review | +1 |
1 comment
auto it = available_screen_ids.begin();Yuwei HuangI'm not sure if it's worth trying to pick the id that corresponds to the monitor with greatest overlap with the new one or not. The 99% use-case for this is restoring a 2-display layout that was saved by the display layout dialog immediately after adding a display, and therefore has no id for that display. For this use-case the extra code would be overkill.
If we wanted to pick the "best" display, is there sufficient information in `preferred_monitors_config_`?
Jamie WalchDo you mean with greatest overlap in desktop geometry?
Am I correct that the CUJ you try to fix here is that when the user connected, they didn't immediately restore the previous display layout (which should just work since it is requesting a new display), but they later added a new display and decided to restore the previous display layout? It sounds like this might be easier to achieve on the client side (i.e. the client finds the best matching existing display for a non-existent display in the stored layout)? But in any case, it doesn't seem like we gain much by matching the display with the greatest overlap.
If we wanted to pick the "best" display, is there sufficient information in preferred_monitors_config_?
It probably has. It has the expected sizes and positions of all the displays that are managed by CRD.
Do you mean with greatest overlap in desktop geometry?
Am I correct that the CUJ you try to fix here is that when the user connected, they didn't immediately restore the previous display layout (which should just work since it is requesting a new display), but they later added a new display and decided to restore the previous display layout?
When saving a layout proto on the client, each display may come from the host, or it might be a "pending" display that only exists in the layout config dialog. The difference is basically whether or not the user added a display before checking the "restore layout" option. Both cases have a problem where the current state of the host might not match what it was when the setting was saved:
- If the display came from the host, it might not exist any more. In that case, https://chromium-review.googlesource.com/c/chromium/src/+/7557558 takes the view that although we can't guarantee the display id in the request, we can at least create a display with the right size and position.
- If the display was pending when the proto was saved, but now exists, then without this CL we will delete the previous display and create a new one. It will have the right size and position, but the window manger might not restore move windows back onto it.
It sounds like this might be easier to achieve on the client side (i.e. the client finds the best matching existing display for a non-existent display in the stored layout)?
We don't want to wait for the host to send its initial layout before replacing it with the saved layout. It can be fixed on the client, but only by setting a "layout pending" flag, and saving the next layout that the host sends (which will have the correct display ids). That seems more complex and error-prone, which is why I preferred this host-side approach.
We don't want to wait for the host to send its initial layout before replacing it with the saved layout.
Ah, I think this is what I was missing. This LGTM now. Thanks for explaining!
Related details
- Jamie Walch
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jamie Walch (Gerrit)
Jamie Walch voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Prioritize reusing existing virtual monitors when setting video layout.
When a new video layout is received, this change modifies the logic to
avoid removing displays and adding others, preferring to reuse existing
displays instead. It can be thought of as a counterpart to
whereas that CL handled the case where the client attempts to restore
the geometry of a display that no longer exists, this one handles the
case where the layout saved by the client includes "new display"
requests that have subsequently been actioned. In particular, when
adding new displays using client UI, they don't have an id until the
host creates them.
- M remoting/host/linux/gnome_desktop_resizer.cc
- M remoting/host/linux/gnome_desktop_resizer_unittest.cc
Code-Review: +1 by Yuwei Huang
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |