Skip to content
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

Choose first embedded screen instead of first screen #32

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rb3ckers
Copy link

@rb3ckers rb3ckers commented Oct 20, 2020

When I have my laptop (Dell XPS) connected to a second screen sometimes icc-brightness picks that screen instead of the embedded screen.

This change fixes that. See also this closed PR that had the same fix but included a bunch of other changes as well: #30.

@tartansandal
Copy link

@rb3ckers I ran into a similar problem with my XPS. Unfortunately, the order of the display data was unpredictable for me. You may want to check out my fix on #34.

@mradke
Copy link

mradke commented Apr 27, 2021

The patch works on my HP Spectre. I think it is sensible to choose the first embedded screen.
@tartansandal why is the order important with this approach here? This would - as far as I understand it - only be problematic if your device has multiple embedded displays.

@tartansandal
Copy link

tartansandal commented Apr 28, 2021

@mdrake

why is the order important with this approach here?

A bit of poor communication on my behalf. Apologies. I was just reiterating the coding issue that both PRs were addressing in different ways.

If you are only interested in controlling the brightness on your laptop, then yes, choosing the first embedded screen is the most sensible approach. However, if you want to keep open the potential for controlling external monitors using the same tool, then that would be problematic.

edit: Though, now that I've had my morning coffee and a think about it, picking the first embedded screen if there is no explicit target does make sense. I'll incorporate that into my MR. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants