XRandR: Allow multiple but not all CRTCs to be redshifted #81

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@lennartS
Contributor

Previously only one CRTC could be set in the configuration file for
redshifting when XRandR mechanism was being used. That is fine for
a setup with two displays but breaks when three or more displays
are in use and one of those shouldn't be redshifted (e.g. two
computer displays and one TV connected to the computer).

The config value 'crtc' for method xrandr can now be entered as
comma separated list of multiple CRTCs. All CRTCs in the list will
be redshifted while all those not in the list will not be touched.

Signed-off-by: Lennart Sauerbeck devel@lennart.sauerbeck.org

@maandree
Contributor

A comma separated list is not implemented. But you
can use multiple CRTC:s if you use pull request #61.

I left comma separated lists out because you probably
want to use gamma correction settings that are not
different for all monitors. So personally I think this
pull request is unnecessary.

@jonls
Owner
jonls commented May 12, 2014

Thanks for looking into this @lennartS, I agree that Redshift should be able to control CRTCs separately. @maandree, can you explain why you think the comma separated list is a bad idea? It seems reasonable to me. I can't easily comment or merge your patch @maandree since it is tied up to a lot of other changes that I can't easily review.

@maandree
Contributor

#61 over this via redshift.conf with the addition that the gamma correction can be set for each monitor individually and introducing a command seperated list for redshift.conf holds minimal value since it just cuts down a minimal when your are not using gamma correction.
As for in the command line. #61 does not support multiple -m, but it should if Redshift is extended to support running multiple adjustments methods concurrently. If this is done with a comma separated list is not as flexible because it does not support individual gamma corrections. But it is a bit more comformable of you are not using gamma corrections.
I think it give unnecessary bloat with minimal benefit. It is only useful if you do not use gamma correction. And if you are not using gamma correction it is really only usable for the command line.
And if you really want the benefit it offers in the command line it can be easily done by adding it to (e.g.) ~/.bashrc, in which case you can even remove the need to specify the adjustment method.


#61 supports display option for randr and vidmode which requires :. Because of this a changed the option delimited from : to , in #61. This because a problem if , is used for CRTC delimiting. The only solution I can think of that does not make it messy to use the command line is to use + for CRTC delimiting. Does anyone have a better idea how to resolve this? Note: If multiple -m gets supported, it makes since to use the gamma option in -m, which also requires :. But I personally think : is even worse than + for CRTC delimiting.


If this is implement, I think it makes sense to also add it to screen and display.

@maandree
Contributor

@jonls I will look into simplifying the task for you.

@lennartS
Contributor

@maandree, I didn't look at all open pull requests, just at the source code in the master branch before implementing this feature.

In regard to a gamma correction values for each monitor and gamma correction in general: I have no idea about this stuff. I just started to use redshift recently (and love it, thanks for this little program, @jonls) but noticed that the movies I watched on the TV were tinted as well, which I didn't like at all :)

#61 was too big for me to overview and evaluate completely. Might I suggest merging my commit until #61 can be merged to allow multiple monitor setups to work more cleanly? If I interpret this correctly #61 works with the dbus changes? For which versions are those planned?

@maandree
Contributor

The dbus changes are implemented separately and the goals
is it should work. I am working on simplifying the commit history
for #61. If jonls merge this first I will make sure it gets placed
in the right place for #61 which has one place — gamma-common.c
— where it can be implement so that it is supported for all adjustment
methods.

@jonls
Owner
jonls commented May 12, 2014

@maandree Thanks, it seems to me that you had #61 split into separate pull requests previously but that you merged it all into one pull request at some point. I would prefer if we could go back to smaller pull requests with one "feature" at a time. On a related note, I added some suggestions to HACKING recently about pull requests and code style.

Regarding the separator change from : to , I think we should discuss that in a pull request that you should open for that specific issue. We have to take into account that , is already used in some locales as a decimal separator and it is therefore problematic to use when delimiting float values.

@maandree
Contributor

@jonls #61 only pulled in fake-w32gdi so that I could test gamma-w32gdi. The features of all other pull requests it replace are rewritten, not merge. This is because I wanted to write gamma-common with everything in mind so that it does not have to be written all the time. Most of these features are minor in size, the big thing is really that all adjustment methods share logic and that multiple CRTC:s, screens, graphics cards and displays can be selected. Hopefully the new commits will have almost fully linear commit logic; if possible, I will split it up separate pull requests.

@jonls
Owner
jonls commented May 12, 2014

@maandree in that case maybe you could implement gamma-common as a first step? It seems it would be pretty easy to merge if it is just moving existing code around. We would then have a common base to work from. Next pull request could be to add mutliple CRTCs/etc on top of that. I'm just throwing things out there right now since I don't really have a clear picture of what your changes do, maybe it makes more since in a different order.

@lennartS Sorry for hijacking this issue comment thread with unrelated stuff. I will keep your patch in mind and possibly merge it if we need to do a release soon or use it for reference when merging the patch @maandree is providing.

@lennartS
Contributor

@jonls No problem. I'll leave it open so you can close it when you know how you want to proceed.

@lennartS lennartS closed this Aug 28, 2016
@davorb
davorb commented Aug 28, 2016

@lennartS has this functionality been added to redshift?

@lennartS
Contributor
lennartS commented Sep 4, 2016

@davorb Sorry, but I don't think so. I'll re-open this for now. I was just doing some housekeeping and saw this was still open.

@lennartS lennartS reopened this Sep 4, 2016
@davorb
davorb commented Sep 4, 2016

Thanks. I would love for this functionality to be added, but it seems like redshift might be abandoned. There have been no commits since February.

@jonls
Owner
jonls commented Sep 4, 2016

@davorb Definitely not abandoned. Though, the time that I have been able to invest lately has been very limited. Let me know if you are interested in getting the code back on track for merge.

@lennartS
Contributor

@jonls Do you mean the patch would be acceptable in principle now? If so, I can take a look at it to get it mergable again.

@jonls
Owner
jonls commented Oct 15, 2016

@lennartS Yes, the other solution that was proposed did not move forward. I would like to merge this instead.

@lennartS lennartS XRandR: Allow multiple but not all CRTCs to be redshifted
Previously only one CRTC could be set in the configuration file for
redshifting when XRandR mechanism was being used. That is fine for
a setup with two displays but breaks when three or more displays
are in use and one of those shouldn't be redshifted (e.g. two
computer displays and one TV connected to the computer).

The config value 'crtc' for method xrandr can now be entered as
comma separated list of multiple CRTCs. All CRTCs in the list will
be redshifted while all those not in the list will not be touched.

Signed-off-by: Lennart Sauerbeck <devel@lennart.sauerbeck.org>
50671b2
@lennartS
Contributor

@jonls That's great to hear. I rebased my change onto your master and fixed the conflicts. Quick testing didn't reveal any bugs on my machine, it still works the same as before the rebase.

@jonls
Owner
jonls commented Jan 8, 2017

@lennartS I made some changes to your pull request to make parsing the list of CRTCs more robust and to provide an error message when the CRTC numbers are incorrectly specified. Can you please test the code in #421?

@lennartS
Contributor

@jonls I confirmed that #421 works as expected and commented over there as well. You added some good changes to my original patch :)

Should I close this pull request now?

@jonls
Owner
jonls commented Jan 14, 2017

@lennartS 👍 Merged.

@jonls jonls closed this Jan 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment