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

Bug: Signify added displayed in black #107

Closed
shadmansaleh opened this issue Feb 20, 2021 · 4 comments
Closed

Bug: Signify added displayed in black #107

shadmansaleh opened this issue Feb 20, 2021 · 4 comments
Labels
enhancement General improvements or improvement request

Comments

@shadmansaleh
Copy link
Member

introduced in 3fecc95

The 'DiffAdd' group is for background highlighting it doesn't have a proper foreground property.
doing hi DiffAdd shpw only ctermbg and guibg properties . The default 'Diffchanged' & 'Diffdelete' are also background highlighter.

Funfact my colorscheme overrites them and makes the bg gray with foreground highlights . Colorchemes can make those colors look realy weird that's why I didn't want to extract colors from highlights :/

@hoob3rt
Copy link
Contributor

hoob3rt commented Feb 22, 2021

True. Although most colorschemes define these colors our implementation does not handle it correctly.
This should probably be something like

color = get_color(DiffAdd, fg) or get_color(DiffAdd, bg)

do you agree?

@hoob3rt hoob3rt added the enhancement General improvements or improvement request label Feb 22, 2021
@shadmansaleh
Copy link
Member Author

shadmansaleh commented Feb 22, 2021

color = get_color(DiffAdd, fg) or get_color(DiffAdd, bg)

I donn't think get_color is returning nil if it was we would already be defaulting to default_colors provided inside signify component. Though I havesn't yet checked what it's returning. If fg doesn't return nil it wonn't work either.

@hoob3rt
Copy link
Contributor

hoob3rt commented Feb 22, 2021

I know. Do you have a better solution though? We cannot possibly support all colorschemes. I think we should implement my suggestion and if that doesn't handle all edge cases then the user can configure colors themself

@shadmansaleh
Copy link
Member Author

I know. Do you have a better solution though? We cannot possibly support all colorschemes

I still think providing our defaults is better than trying to get colors from colorcheme when we have no idea what they look like . In one colorscheme I tested the signify components added and modified colors fg completely blended with bg so what I had was a big empty area and after that removed count.

We should just provide a set of colors if users donn't like it they can change it atlest the colors we provide wonn;t produce weird behiviors like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement General improvements or improvement request
Projects
None yet
Development

No branches or pull requests

2 participants