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

feat: make invert_diff configurable #291

Closed
wants to merge 2 commits into from
Closed

Conversation

eeeXun
Copy link
Contributor

@eeeXun eeeXun commented Oct 18, 2023

Some user may still need old style of diff. #239 (comment)

@eeeXun
Copy link
Contributor Author

eeeXun commented Oct 25, 2023

This should close #290

Comment on lines +364 to +366
DiffDelete = config.invert_diff and { fg = colors.bg0, bg = colors.red } or { bg = colors.dark_red },
DiffAdd = config.invert_diff and { fg = colors.bg0, bg = colors.green } or { bg = colors.dark_green },
DiffChange = config.invert_diff and { fg = colors.bg0, bg = colors.aqua } or { bg = colors.dark_aqua },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style not quite a revert of the style, if the user has config.inverse = true.

Suggested change
DiffDelete = config.invert_diff and { fg = colors.bg0, bg = colors.red } or { bg = colors.dark_red },
DiffAdd = config.invert_diff and { fg = colors.bg0, bg = colors.green } or { bg = colors.dark_green },
DiffChange = config.invert_diff and { fg = colors.bg0, bg = colors.aqua } or { bg = colors.dark_aqua },
DiffDelete = config.invert_diff and { fg = colors.bg0, bg = colors.red, reverse = config.inverse } or { bg = colors.dark_red },
DiffAdd = config.invert_diff and { fg = colors.bg0, bg = colors.green, reverse = config.inverse } or { bg = colors.dark_green },
DiffChange = config.invert_diff and { fg = colors.bg0, bg = colors.aqua, reverse = config.inverse } or { bg = colors.dark_aqua },

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, I think there are actually two things happening in the new configuration:

  • The colors changed.
  • And the reverse is unconditionally false.

I want both the old colours and reverse = true.

Whereas you only want the old colours?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right

@ambroisie ambroisie mentioned this pull request Oct 26, 2023
ambroisie added a commit to ambroisie/nix-config that referenced this pull request Oct 26, 2023
I dislike the new style of diff [1].

Thankfully somebody wrote a patch to configure it [2] (though not
completely to my liking, so the local patch here is a bit different).
I used it as a basis, but made it always revert, rather than
configurable.

[1]: ellisonleao/gruvbox.nvim#290
[2]: ellisonleao/gruvbox.nvim#291
ambroisie added a commit to ambroisie/nix-config that referenced this pull request Oct 26, 2023
I dislike the new style of diff [1].

Thankfully somebody wrote a patch to configure it [2] (though not
completely to my liking, so the local patch here is a bit different).
I used it as a basis, but made it always revert, rather than
configurable.

[1]: ellisonleao/gruvbox.nvim#290
[2]: ellisonleao/gruvbox.nvim#291
@ellisonleao
Copy link
Owner

hey @eeeXun thanks for the PR but this seems like a personal preference choice. Have you tried just using the overrides option to get the colors of your preference?

@eeeXun
Copy link
Contributor Author

eeeXun commented Oct 26, 2023

Hi @ellisonleao, I don't think it's a personal preference. Although I use the new diff style. But there are many sounds #239 (comment) and #290. Not everybody like the new style.
Why not keep the option that user could easily access instead of overrides by their own?

@ellisonleao
Copy link
Owner

Not everybody like the new style.

It is impossible to please everyone

Why not keep the option that user could easily access instead of overrides by their own?

because it's a setting for only one highlight which IMO doesn't make sense. Imagine if everyone creates a settings for a small group or single highlights? With the overrides function we can basically give you the ability to use your own personal preference, together with override_colors, if they also don't make sense four your choice.

ambroisie added a commit to ambroisie/nix-config that referenced this pull request Oct 26, 2023
I dislike the new style of diff [1].

After somebody wrote a patch [2] I finally started experimenting with
what looked best to me.

This is using the old vibrant colours, which I like better. And avoids
using `reverse = true` to not break high-lighting during visual
selection.

This is using an overlay as it is _much_ easier to refer to the internal
colours in a `dark`/`light` agnostic way that way instead of the
intended "use the palette way" (due to breaking changes in [3] which,
incidentally, is the MR which changed diff high-lighting).

[1]: ellisonleao/gruvbox.nvim#290
[2]: ellisonleao/gruvbox.nvim#291
[3]: ellisonleao/gruvbox.nvim#280
@eeeXun
Copy link
Contributor Author

eeeXun commented Oct 26, 2023

because it's a setting for only one highlight which IMO doesn't make sense.

I'm not sure if I misunderstand the one highlight you said. But this PR changes three highlights. DiffDelete, DiffAdd and DiffChange. DiffText doesn't count. Because not find a good color for new style yet. But one day we may find it.
And as I said, there are users who want the old diff style. That's the point to make it could be easily config.

@ellisonleao
Copy link
Owner

@eeeXun i guess you didn't read the entire sentence, but let me quote it again:

Imagine if everyone creates a settings for a small group or single highlights?

Because not find a good color for new style yet. But one day we may find it.

Excatly. But in the meantime overrides is the way to go and not more configs

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.

3 participants