search

tinted-theming / base16-emacs

Base16 themes for Emacs
MIT License
380 stars 76 forks source link

Support line-number and line-number-current-line faces #52

Closed bbenne10 closed 7 years ago

bbenne10 commented 7 years ago

With the inclusion of line-number and line-number-current-line faces in emacs' master branch, I'm attempting to wean myself off of linum-mode. This would be very simple if we could support these faces in themes.

Currently, I'm using the base16-gruvbox-dark-medium theme and have this in my init.el:

(set-face-attribute 'line-number 
  :foreground (plist-get base16-gruvbox-dark-medium-colors :base03) 
  :background (plist-get base16-gruvbox-dark-medium-colors :base01)
(set-face-attribute 'line-number-current-line
  :foreground (plist-get base16-gruvbox-dark-medium-colors :base04))

This lines up with setting these faces up in base16-theme-define and causes the fringe and line number faces to share the same background as the linum face (and causes a nice highlight on the current line). (I also have the fringe changed to use base01 over base02, as the new line numbers show up to the RIGHT of the fringe now and I just think that it looks weird with the fringe a lighter color, but that's not really related to this - more to the discussion in #30)

I'm not sure how to do this safely for users without the new faces (ignore-errors comes to mind, but the way the file is structured isn't conducive to doing this selectively)

Once the issue of doing this for users without these faces is solved, I'd be happy to try my hand at a PR with this functionality

belak commented 7 years ago

I've added line-number and line-number-current-line faces which should closely match the current behavior but I'm not sure what to do with the fringe because of the location change. I'm currently trying out a background of base01 which matches the line numbers, but changing it changes how it looks for people not running emacs from master.

Anyway, thanks for the suggestion! I didn't know this was being added.

bbenne10 commented 7 years ago

Forgive my ignorance, but is that change to be pushed here? I don't see it yet pushed.

belak commented 7 years ago

Oops, just pushed it (f701a8e191ae9c0bd6ab93926ce993bb18a9e98c)

bbenne10 commented 7 years ago

Since this is pushed, it seems this issue is resolved. Thanks for the quick turnaround!