search

nashamri / spacemacs-theme

Light and dark theme for spacemacs that supports GUI and terminal
GNU General Public License v3.0
605 stars 114 forks source link

Green diff overlays, make blue text almost invisible #79

Closed duianto closed 6 years ago

duianto commented 7 years ago

When a file with a merge conflict is opened, then it shows red and green colored diff sections.

Blue comment/keyword text becomes almost invisible in the bright green sections. And green text in the bright red sections, makes the text almost "vibrate".

blue and cyan text almost invisible under green overlay

This is what the text looks like without the green diff overlay:

without green diff colors

System Info :computer:

nashamri commented 7 years ago

Thanks @duianto I think I fixed it: smerge

smerge-light

nashamri commented 7 years ago

Oh and another thing, now spacemacs has its own copy of the theme. So, until these new changes get commited to spacemacs codebase, you can get these updates from MELPA.

duianto commented 7 years ago

That looks great (as usual), thanks 👍

Yeah I know about Spacemacs using a local copy. The reason seems to be, so that the theme is available earlier in the startup process, and to improve reliability if there are any startup or package install issues.

Source: spacemacs-theme updates available, but I'm getting "All packages are up to date" #8397

It looks like the local spacemacs-theme is being tracked on the Spacemacs repository as well. If it's not a lot of extra work for you, then maybe you could make a PR when there's an update. I'm sure you want Spacemacs to use the latest version. TheBB is actively merging PRs so it might get merged quickly.

I'm not in a hurry, to get this feature implemented (because i don't make merge conflicts 😜😆), but someone else might also find this useful.

If your busy, then it might be better to ping the Spacemacs team, to merge all changes since the last update, possibly once a week/month, or if there's a major update.

I'm not sure how to use the MELPA version at the same time as the local version. The link above shows some issues I ran into while trying to install the MELPA version.

nashamri commented 7 years ago

I was actually looking at the last commit merged with spacemacs to prepare a PR :smile: I'll send one right now and hopefully a new version of spacemacs will be released soon :beers:

duianto commented 7 years ago

I got a merge conflict that shows the smerge-refined-changed face:

merge conflict

Problem:

It isn't obvious that the merge conflicts changed (blue) text, isn't just part of the regular text, since it looks so similar to the function and keyword text above the merge conflict.

Possible cause:

I noticed that two of the spacemacs-theme colors are almost identical: green-bg #293235 which is used by smerge-other, and blue-bg #293239 which is used by smerge-refined-changed.

And the smerge-refined-{added,removed} faces, use colors with -s at the end: green-bg-s #29422d and red-bg-s #512e31, but smerge-refined-changed uses the color blue-bg:

`(smerge-refined-added ((,class (:background ,green-bg-s :foreground ,green))))
`(smerge-refined-changed ((,class (:background ,blue-bg :foreground ,blue))))
`(smerge-refined-removed ((,class (:background ,red-bg-s :foreground ,red))))

Possible solution:

spacemacs-common.el doesn't have a definition for a blue-bg-s color. If it was added as a brighter blue, then it could be used as the smerge-refined-changed background.

That would probably make it more obvious that the blue text in the merge conflict is different from the regular function/keyword text.

Additional notes:

The hue for the green-bg (138) and blue-bg (146) should probably be separated more.

The ;; colors section in spacemacs-common.el should probably be sorted alphabetically. The non-alphabetically ordered colors are:

nashamri commented 7 years ago

Yup! good notes. Will fix that :smile:

nashamri commented 6 years ago

Added a new color blue-bg-s that should solve this issue.

Thanks @duianto :+1: