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

[WIP] Implement color themes - closes #28 #65

Merged
merged 11 commits into from
May 19, 2020

Conversation

MCord
Copy link
Contributor

@MCord MCord commented May 17, 2020

This PR refactors all color related code and creates an style that can be modified to create different color themes for the program. it also provides serialization to and from a config file. changes in this PR

  1. created Theme struct to represent different colors and some helper functions.
  2. replaced all hard coded colors with references to the global THEME variable.
  3. add serde and ron crates for serialization. and lazy_static for static init.
  4. on start the program tries to load ~/.gitui/theme.ron to load the theme or creates one with default values if not found.

This PR alone at the current state is not enough to provide theming since many components don't specify any style and rely on defaults to draw elements. I have not yet looked into how to change these defaults.

src/components/diff.rs Outdated Show resolved Hide resolved
src/ui/style.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@extrawurst
Copy link
Owner

Looking at the theme.ron I am worried that we went too far here. afterall in most cases we only use one of the colors and no modifier. maybe we can take another angle at it and only load colours and pass a color scheme down to any component that uses styling. this component then could generate the styles on the fly as before but based on the color schema.

this way me might end up with less content in the theme.ron and less todo for anyone getting their hands on it.

The big benefit for us is aswell that it is easier to reason about what colors we already use in the scheme.

What do you think?

@MCord
Copy link
Contributor Author

MCord commented May 18, 2020

@extrawurst can you provide an example theme.ron file so that I better understand the model you have in mind ?

@extrawurst
Copy link
Owner

pretty sure I missed some:

(
    selection_bg: Rgb(0,0,100), // also used for commands background
    selected_tab: Yellow,
    command_disabled: LightGrey,
    diff_line_add: Green,
    diff_line_delete: Red,
    diff_file_added: LightGreen,
    diff_file_removed: LightRed,
    diff_file_moved: LightMagenta,
    commit_hash: Magenta,
    commit_time: Blue,
    commit_author: Green,
    commit_tag: Yellow,
)

the trick is then to consider every new entry and reuse any color that is already in there (of course that will drive appropriate naming virtually impossible. But this way looking at gitui someone right aways know: "ok this yellow I see here is bad in my term color scheme, so I change yellow in the file and it is fixed in all places at once"

@MCord
Copy link
Contributor Author

MCord commented May 18, 2020

I think having specific names for colors like 'selected_tab' can be confusing once we have used this color in other places but I agree that we don't have enough cases to try to generalize now. I will implement specific case for the moment as you specified.

src/components/diff.rs Outdated Show resolved Hide resolved
@MCord MCord force-pushed the issue28_color_theme branch from 3b269b3 to 42447da Compare May 19, 2020 10:27
src/app.rs Outdated Show resolved Hide resolved
@MCord
Copy link
Contributor Author

MCord commented May 19, 2020 via email

src/spinner.rs Outdated Show resolved Hide resolved
src/ui/style.rs Outdated Show resolved Hide resolved
src/ui/style.rs Outdated Show resolved Hide resolved
src/ui/style.rs Outdated Show resolved Hide resolved
@extrawurst
Copy link
Owner

some styles seem to be lost in translation:

before:
image

after:
image

@MCord
Copy link
Contributor Author

MCord commented May 19, 2020

strange, the difference is not visible on windows. I try to reproduce on Linux.

@MCord
Copy link
Contributor Author

MCord commented May 19, 2020

i have tried to reproduce it by looking at the diff. can you try again ? (feel free to fix if you know what's wrong)

@extrawurst
Copy link
Owner

looks good. I checked how the default looks on a light terminal:
image
that is not bad as a default actually.

my only remaining concern is that we now rely on an ENV variable to set when one wants to switch the theme.

what if we only put the dark theme in the binary and dont parse the ENV but document in the readme what to paste into the generated ron file?

@MCord
Copy link
Contributor Author

MCord commented May 19, 2020

The idea was to include two themes and a way to auto detect which to use given terminals background color but I failed to find a way to auto detect so there is not much value in keeping both unless we find a way to auto detect.

@extrawurst
Copy link
Owner

Yeah I reached out to the crossterm guys on discord, there is not good way for auto detecting. I would love to have that. but I think for now this is out of scope of this PR

@MCord
Copy link
Contributor Author

MCord commented May 19, 2020

gone !

src/components/msg.rs Outdated Show resolved Hide resolved
src/spinner.rs Outdated Show resolved Hide resolved
src/tabs/revlog/mod.rs Outdated Show resolved Hide resolved
@extrawurst extrawurst merged commit 4ec1a4e into extrawurst:master May 19, 2020
@extrawurst
Copy link
Owner

It is in! Thank you so much for all the effort!🚀

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