XRandR: Allow multiple but not all CRTCs to be redshifted

[lennartS]

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]

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]

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]

#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]

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

[lennartS]

@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]

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]

@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]

@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]

@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]

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

[davorb]

@lennartS has this functionality been added to redshift?

[lennartS]

@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.

[davorb]

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]

@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]

@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]

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

[lennartS]

@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]

@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]

@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]

@lennartS 👍 Merged.