Skip to content

Conversation

@thiagokokada
Copy link
Contributor

@thiagokokada thiagokokada commented Jan 23, 2021

Description

Nowadays services.{redshift,gammastep} modules are really similar. They should, since Gammastep is a fork of Redshift with the main objective is to support Wayland.

So instead of trying to maintain two separate modules, this commit unify the options in lib/options.nix file, while the implementation of the module itself ends up being really simple (just calling the common options with the necessary parameters to differentiate between them).

My reasoning for this is to make maintaining the modules easier, but also I want to update the modules to use config file instead of passing parameters via command-line, since this will allow it to support options not supported currently. Modifying both modules separately would be very error prone, so I decided to first unify them and afterwards I can open another PR migrating them to the config file.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@thiagokokada thiagokokada requested a review from rycee as a code owner January 23, 2021 22:17
@thiagokokada thiagokokada force-pushed the unify-gammastep-redshift-modules branch 2 times, most recently from b39a03c to 9361770 Compare January 23, 2021 22:19
Nowadays services.{redshift,gammastep} modules are really similar. They
should, since Gammastep is a fork of Redshift with the main objective is
to support Wayland.

So instead of trying to maintain two separate modules, this commit unify
the options in lib/options.nix file, making the implementation of the
module itself ends up being really simple (just calling the common
options with the necessary parameters to differentiate between them).
@thiagokokada thiagokokada force-pushed the unify-gammastep-redshift-modules branch from 9361770 to 7de0d07 Compare January 26, 2021 04:40
@thiagokokada
Copy link
Contributor Author

Did some improvements in tests so we make sure that this commit is not breaking anything.

@thiagokokada
Copy link
Contributor Author

CC @rycee @petabyteboy .

@rycee rycee merged commit 7de0d07 into nix-community:master Jan 27, 2021
@rycee
Copy link
Member

rycee commented Jan 27, 2021

Thanks! Merged to master.

I suspect that if gammastep supports both X and Wayland then we may want to consider removing the redshift module entirely but perhaps it's best to wait a while to see how the two projects develop.

In any case, thanks for the contribution!

@thiagokokada thiagokokada deleted the unify-gammastep-redshift-modules branch January 27, 2021 18:48
@thiagokokada
Copy link
Contributor Author

I suspect that if gammastep supports both X and Wayland then we may want to consider removing the redshift module entirely but perhaps it's best to wait a while to see how the two projects develop.

redshift seems to be focusing in having a dbus integration nowadays: jonls/redshift#54. gammastep itself doesn't seem to have much interested on it: https://gitlab.com/chinstrap/gammastep/-/issues/11. So yeah, the projects can diverge in the future, and some people may prefer to use one (for example, redshift for dbus support) or another (gammastep for wayland support).

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.

2 participants