tinted-theming / base16-emacs

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

Adding setting for emphasizing the mode line. #66

Closed keithrussell42 closed 6 years ago

keithrussell42 commented 6 years ago

There are certain themes (e.g. tomorrow) where the active and inactive mode lines are hard to tell apart. This change adds a setting to enable a thin box to be drawn around the active mode line making it subtly stand out.

I also just realized this may help with issue #51. I guess I should have looked at the issues first :-)

keithrussell42 commented 6 years ago

I don't know the code for base16-theme well enough to say for sure but I believe it may be possible to have this function I added be a generic settings "applier". If the colour values in the definition were replaced with the keywords instead of having base16-transform-color-key convert symbols to keywords then the fringe-bg transform could take place in the new function. I was thinking it may make applying future settings easier. I'd be willing to put up another pull request with this idea but since this is my first contribution I didn't want to step on any toes.

belak commented 6 years ago

As far as I can tell, this looks alright. It would be a decent place to fit in other settings as well.

I'm curious what you mean about the transformation... It's been a while since I've messed with this code so I don't remember it as well as I'd like.

belak commented 6 years ago

I just realized the wasn't anything in the readme about this... I'm not at my computer at the moment so I can't add anything right now, but I'll try to remember later today.

belak commented 6 years ago

Also thanks for your contribution! This is something I wasn't very sure how to fix. I'm not a huge fan of the border, but providing a setting is a good idea.

keithrussell42 commented 6 years ago

First off, you're quite welcome. I'm glad to be able to help given how much I like the base16 themes :-) And my bad for not updating the readme. I forgot to include that. If I propose any more user-visible changes I'll include readme updates in the pull request. Sorry about that.

I know not everyone likes the box but it should be possible to add different emphasis styles. I noticed that base16-default-dark uses a much brighter foreground for the active mode line so that could be another easy change. No reason that the option can't be a choice of different emphasis styles.

The change I was proposing was to roll the colour key transformation into the apply settings function. If I'm reading the code right the colour key bit applies its transform to the alist's value substituting fringe-bg if found otherwise returning : + value which is then used as the colour lookup if the key is :color. If the transform were to happen earlier (which I did for the apply function) then the conversion from symbol to : + symbol would either still need to happen or the symbols in the definitions would all change to keywords. I'm not sure it would be worthwhile, though. The change I made has the drawback of not being able to do anything other than modify the value of a key in an alist. It solves the particular problem I had but doesn't allow one to, say, add :box for a definition where it doesn't already exist. I don't know how much of a limitation that is in reality. I guess it depends on what other settings you would want to add.

belak commented 6 years ago

Ah, I think I see what you're saying now. I think merging those two functions would make sense because they're doing very similar things.

And don't worry too much about the documentation. It's partially my fault for missing it. ;)

keithrussell42 commented 6 years ago

OK. I can put together another pull request with my proposal. It's easier to describe in code. Thanks, Keith

On Mon, Jan 15, 2018, at 14:36, Kaleb Elwert wrote:

Ah, I think I see what you're saying now. I think merging those two functions would make sense because they're doing very similar things.> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub[1], or mute the thread[2].>

Links:

  1. https://github.com/belak/base16-emacs/pull/66#issuecomment-357771330
  2. https://github.com/notifications/unsubscribe-auth/AhxVRBRGMED8t1CnRovUPpHJ9P8hACk2ks5tK6iwgaJpZM4RdrAz