Skip to content

Don't skip disconnected X11 outputs #1071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 28, 2023

Conversation

luk1337
Copy link
Contributor

@luk1337 luk1337 commented Mar 25, 2023

Description

This change lets me use HDMI-0 (with HDMI dummy plugged in) as a Sunshine exclusive output when using the following do/undo commands:

  • xrandr --output HDMI-0 --mode 1920x1080 --right-of DP-0
  • xrandr --output HDMI-0 --off

Screenshot

n/a

Issues Fixed or Closed

n/a

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency update (updates to dependencies)
  • Documentation update (changes to documentation)
  • Repository update (changes to repository files, e.g. .github/...)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated the in code docstring/documentation-blocks for new or existing methods/components

Branch Updates

LizardByte requires that branches be up-to-date before merging. This means that after any PR is merged, this branch
must be updated before it can be merged. You must also
Allow edits from maintainers.

  • I want maintainers to keep my branch updated

@ReenigneArcher
Copy link
Member

ReenigneArcher commented Mar 27, 2023

@KuleRucket if you get a chance, could you give a review on this one? Pretty simple change, but want to make sure it won't cause issues in other scenarios.

@KuleRucket
Copy link
Contributor

I like the idea that sunshine can start with the display off, and then be able to use it later if it is switched on. It makes sense and the change works for it.

The RR_Connected state is checked twice in the class, once in x11_display_names() and then once in init(). For Sunshine to start up properly x11_display_names() must return at least one connected display and if none are found then there is an error and x11 get selected as a valid capture method. So the situation described above if relying on the DP-0 to be present. I wonder if this could go one step further and allow 0 displays to be connected and allow them to be switched on later.

The config web page might need to be adjusted though. It say to user "xrandr --listmonitors" which only gives you connected. A new way to retrieve the number you want should be provided. In future we could consider using the monitor name instead of the number.

@luk1337
Copy link
Contributor Author

luk1337 commented Mar 27, 2023

When you're using just a single monitor, it's usually always connected, so I don't think it's really that important to support "the only monitor is disconnected" case.

@luk1337
Copy link
Contributor Author

luk1337 commented Mar 27, 2023

But, if you want, I can update this change to remove the remaining RR_Connected check as that shouldn't really break anything (I hope?)

@ReenigneArcher

This comment was marked as resolved.

@KuleRucket
Copy link
Contributor

But, if you want, I can update this change to remove the remaining RR_Connected check as that shouldn't really break anything (I hope?)

I think the most important thing is to change the documentation to explain how to get the display number. The easiest way to do this is to change x11_display_names() to list them all with numbers and then the documentation can tell people to check the log file for the display numbers.

I can't see anything wrong with removing the RR_Connected check from x11_display_names(). It will make it easier to print the full list of displays.

@luk1337 luk1337 force-pushed the luk/x11_iter branch 2 times, most recently from 6d54460 to fb82c97 Compare March 28, 2023 07:40
@KuleRucket
Copy link
Contributor

Can you add (Connected)/(Disconnected) to the log message using RR_Connected status? If someone it looking for selecting a connected monitor this would help.

@luk1337
Copy link
Contributor Author

luk1337 commented Mar 28, 2023

Can you add (Connected)/(Disconnected) to the log message using RR_Connected status? If someone it looking for selecting a connected monitor this would help.

Done.

This change lets me use HDMI-0 (with HDMI dummy plugged in) as a
Sunshine exclusive output when using the following do/undo commands:
* xrandr --output HDMI-0 --mode 1920x1080 --right-of DP-0
* xrandr --output HDMI-0 --off
@luk1337
Copy link
Contributor Author

luk1337 commented Mar 28, 2023

Does something like this look good for docs/web update?

diff --git a/docs/source/about/advanced_usage.rst b/docs/source/about/advanced_usage.rst
index d22555f..19fe237 100644
--- a/docs/source/about/advanced_usage.rst
+++ b/docs/source/about/advanced_usage.rst
@@ -299,13 +299,18 @@ output_name
    .. Tip:: To find the name of the appropriate values follow these instructions.
 
       **Linux**
-         .. code-block:: bash
+         During Sunshine startup, you should see the list of detected monitors:
 
-            xrandr --listmonitors
+         .. code-block:: text
 
-         Example output: ``0: +HDMI-1 1920/518x1200/324+0+0  HDMI-1``
+            Info: Detecting connected monitors
+            Info: Detected monitor 0: DVI-D-0, connected: false
+            Info: Detected monitor 1: HDMI-0, connected: true
+            Info: Detected monitor 2: DP-0, connected: true
+            Info: Detected monitor 3: DP-1, connected: false
+            Info: Detected monitor 4: DVI-D-1, connected: false
 
-         You need to use the value before the colon in the output, e.g. ``0``.
+         You need to use the value before the colon in the output, e.g. ``1``.
 
       .. Todo:: macOS
 
diff --git a/src_assets/common/assets/web/config.html b/src_assets/common/assets/web/config.html
index 9e664ee..9524755 100644
--- a/src_assets/common/assets/web/config.html
+++ b/src_assets/common/assets/web/config.html
@@ -560,9 +560,17 @@
           v-model="config.output_name"
         />
         <div class="form-text">
-          xrandr --listmonitors<br />
-          Example output:
-          <pre>   0: +HDMI-1 1920/518x1200/324+0+0 HDMI-1</pre>
+          During Sunshine startup, you should see the list of detected monitors, e.g.:<br />
+          <br />
+          <pre style="white-space: pre-line;">
+            Info: Detecting connected monitors
+            Info: Detected monitor 0: DVI-D-0, connected: false
+            Info: Detected monitor 1: HDMI-0, connected: true
+            Info: Detected monitor 2: DP-0, connected: true
+            Info: Detected monitor 3: DP-1, connected: false
+            Info: Detected monitor 4: DVI-D-1, connected: false
+          </pre>
+          You need to use the value before the colon in the output, e.g. <b>1</b>.
         </div>
       </div>
     </div>

@ReenigneArcher
Copy link
Member

@luk1337 I think that would be fine.

@luk1337
Copy link
Contributor Author

luk1337 commented Mar 28, 2023

@luk1337 I think that would be fine.

ok, I pushed it.

@ReenigneArcher ReenigneArcher merged commit 1ab1b79 into LizardByte:nightly Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants