Skip to content
Snippets Groups Projects

Don't remove an existing monitor from an output if another monitor is added

Merged Don't remove an existing monitor from an output if another monitor is added
mwyraz/xserver:feature/split-screen into master
Merged Michael Wyraz requested to merge mwyraz/xserver:feature/split-screen into master

In commit 7e1f86d4 monitor support was added to randr. At this time it seemed to be reasonable not to have more than one (virtual) monitor on a particular physical display. The code was never changed since.

Nowadays, extremely large displays exists (4k displays, ultra-wide displays). In some use cases it makes sense to split these large physical displays into multiple virtual monitors. An example are ultra-wide screens that can be split into 2 monitors. The change in this commit makes this work.

Besides that, removing a monitor in a function that is called "RRMonitorAdd" is bad practice and causes unexpected behaviour.

More information and discussion can be found at https://gitlab.gnome.org/GNOME/gtk/-/issues/2013#note_1564528 .

Are there possible impact to people that rely on this feature?

Potentially, that change breaks scripts that adds a monitor to a display where already a monitor exists. In my opinion it is very unlikely that such situations exists. There may be scripts that add a single monitor to a screen so that the monitor has a name different from the implicit default monitor name that is assigned if no user defined monitor is added to a screen. This still works as before with my change. Besides that, it is always possible to delete existing monitors with randr/xrandr.

Edited by Michael Wyraz

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Michael Wyraz added 1 commit

    added 1 commit

    Compare with previous version

  • Michael Wyraz added 100 commits

    added 100 commits

    Compare with previous version

  • Olivier Fourdan
    Olivier Fourdan @ofourdan ·
    Maintainer

    I am not necessarily the best placed to review this, but FWIW, here follows my comments:

    1. The two commits should be squashed, the second being a clean up up after the first.
    2. The commit message needs to contain a meaningful explanation

    This removes existing code, that was, I assume, added for a purpose in commit 7e1f86d4, so this would need to be explained.

    I understand that it fixes an issue for you, but are there other people relying on the current implementation that gets removed?

    Basically:

    • What is the problem with the existing code?
    • How does the change you introduce fixes that issue?
    • Does that change the current behaviour?
    • What are the possible side effects of removing that code?

    When that gets fully documented in the commit messages (and possibly in this MR), that would possibly help other people understand the purpose of your change, and its implications.

  • Olivier Fourdan added randr label

    added randr label

  • Michael Wyraz added 1 commit

    added 1 commit

    • 21e569ef - Removing the code that deletes an existing monitor in RRMonitorAdd

    Compare with previous version

  • Michael Wyraz changed the description

    changed the description

  • Michael Wyraz
    Michael Wyraz @mwyraz ·
    Author Contributor

    @ofourdan Thank you very much for your feedback. I have squashed the commit and updated the description. Do you think it is good enough now or should I add more aspects/details?

  • Michael Wyraz
    Michael Wyraz @mwyraz ·
    Author Contributor

    I have asked a while ago (before the PR) on the mailing list and got the following response from @keithp , the original author of the commit that introduced monitor support.

    That's actually required in the RandR spec:

    For each output in 'info.outputs, each one is removed from all pre-existing Monitors. If removing the output causes the list of outputs for that Monitor to become empty, then that Monitor will be deleted as if RRDeleteMonitor were called.

    The notion of splitting one physical output into multiple virtual monitors was not considered when this extension was defined, which is why it doesn't work. I don't see any particular reason for not supporting your use case.

    However, there are subtleties here. We want to remove any automatically created 'Monitor' objects when mapping user-specified monitors to them, and we want to re-generate automatically generated 'Monitors' when all virtual monitors associated with an output are removed.

    I think what we want is:

    • If no user-specified Monitors map to a particular Output, then automatically create a Monitor for that Output

    • If any user-specified Monitors map to a particular Output, then remove the automatically generated Monitor for that Output.

    In the current spec, there's no real separation between user-specified and automatically-generated Monitors, I think that would be necessary to make this work?

    I have found the relevant spec at https://github.com/freedesktop/xorg-xorgproto/blob/master/randrproto.txt#L1696 . I guess it makes sense to change this in this PR too (Edit: just saw that it's a different repo, so i will add a PR there too // Edit 2: I added the PR at xorg/proto/xorgproto!64 (merged)).

    I also have checked the suggestions about the behaviour and found that it actually behaves exactly as desired:

    Without my patch:

    #> xrandr --listmonitors
Monitors: 1
 0: +HDMI-A-0 3440/820x1440/346+0+0  HDMI-A-0

#> xrandr --setmonitor VIRTUAL-LEFT 1520/0x1440/1+0+0 HDMI-A-0
#> xrandr --listmonitors
Monitors: 1
 0: VIRTUAL-LEFT 1520/0x1440/1+0+0  HDMI-A-0

#> xrandr --setmonitor VIRTUAL-LEFT 1520/0x1440/1+0+0 HDMI-A-0
#> xrandr --listmonitors
Monitors: 1
 0: VIRTUAL-LEFT 1520/0x1440/1+0+0  HDMI-A-0

#> xrandr --setmonitor VIRTUAL-RIGHT 1920/0x1440/1+1520+0 HDMI-A-0
#> xrandr --listmonitors
Monitors: 1
 0: VIRTUAL-RIGHT 1920/0x1440/1+1520+0  HDMI-A-0

#> xrandr --delmonitor VIRTUAL-RIGHT
#> xrandr --listmonitors
Monitors: 1
 0: +HDMI-A-0 3440/820x1440/346+0+0  HDMI-A-0

    So there is already a default monitor that is replaced by the user defined monitor. Adding another user defined monitor removes the first user defined monitor. Removing the user defined monitor restores the default monitor.

    Now with my patch my patch:

    #> xrandr --listmonitors
Monitors: 1
 0: +HDMI-A-0 3440/820x1440/346+0+0  HDMI-A-0

#> xrandr --setmonitor VIRTUAL-LEFT 1520/0x1440/1+0+0 HDMI-A-0
#> xrandr --listmonitors
Monitors: 1
 0: VIRTUAL-LEFT 1520/0x1440/1+0+0  HDMI-A-0

#> xrandr --setmonitor VIRTUAL-LEFT 1520/0x1440/1+0+0 HDMI-A-0
#> xrandr --listmonitors
Monitors: 1
 0: VIRTUAL-LEFT 1520/0x1440/1+0+0  HDMI-A-0

#> xrandr --setmonitor VIRTUAL-RIGHT 1920/0x1440/1+1520+0 HDMI-A-0
#> xrandr --listmonitors
Monitors: 2
 0: VIRTUAL-LEFT 1520/0x1440/1+0+0  HDMI-A-0
 1: VIRTUAL-RIGHT 1920/0x1440/1+1520+0  HDMI-A-0

#> xrandr --delmonitor VIRTUAL-LEFT
#> xrandr --delmonitor VIRTUAL-RIGHT
#> xrandr --listmonitors
Monitors: 1
 0: +HDMI-A-0 3440/820x1440/346+0+0  HDMI-A-0

    Also here it starts with the default monitor that is replaced by the user defined monitor. Adding another user defined monitor does not remove the first user defined monitor anymore. Removing both user defined monitor restores the default monitor.

    Edited by Michael Wyraz
  • Michael Wyraz mentioned in merge request xorg/proto/xorgproto!64 (merged)

    mentioned in merge request xorg/proto/xorgproto!64 (merged)

  • Keith Packard
    Keith Packard @keithp ·
    Owner

    The proto patch indicates that an automatically generated monitor should be removed, but this patch doesn't appear to do that part? Or am I just missing something?

  • Michael Wyraz
    Michael Wyraz @mwyraz ·
    Author Contributor

    The proto patch reflects the actual behaviour of randr (except the ability to have >1 monitor). The auto generated monitor is already removed when adding a user defined one an and re-created if that user defined one is deleted.

    With my shell example above, this can easily be verified.

  • Michael Wyraz
    Michael Wyraz @mwyraz ·
    Author Contributor

    @keithp Did you see my answer? In fact the auto generated monitor is already removed when a new monitor is added and it is re-added when the user defined monitor is removed. So no code needs to be changed to make this work.

    Is there anything else missing to get my PR approved?

    Kind regards, Michael.

  • Olivier Fourdan mentioned in merge request !1113 (merged)

    mentioned in merge request !1113 (merged)

  • Michael Wyraz added 74 commits

    added 74 commits

    • 21e569ef...9a55c402 - 73 commits from branch xorg:master
    • de3591f8 - Removing the code that deletes an existing monitor in RRMonitorAdd

    Compare with previous version

  • Michael Wyraz added 61 commits

    added 61 commits

    • de3591f8...722e464b - 60 commits from branch xorg:master
    • 01c0622b - Removing the code that deletes an existing monitor in RRMonitorAdd

    Compare with previous version

  • Matt Turner added 76 commits

    added 76 commits

    • 01c0622b...e2eeaab2 - 75 commits from branch xorg:master
    • 818130bb - Removing the code that deletes an existing monitor in RRMonitorAdd

    Compare with previous version

  • Matt Turner enabled an automatic merge when the pipeline for 818130bb succeeds

    enabled an automatic merge when the pipeline for 818130bb succeeds

  • Matt Turner merged

    merged

  • Michael Wyraz
    Michael Wyraz @mwyraz ·
    Author Contributor

    @Matt, thank you so much !!!

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply