search

tinted-theming / base16-emacs

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

Unification of colour and settings application. #67

Closed keithrussell42 closed 6 years ago

keithrussell42 commented 6 years ago

This change combines the existing colour key transformation with the settings application. The hope is that this makes it easier to add new user settings in the future. I had wanted to extend the mode line emphasis to allow a choice of either a box or a darker foreground colour. This would have meant putting the setting specific code in two separate functions (settings for the box and colour-key for the foreground). Before I add that option I figured this refactoring would be worthwhile to try out. I've tested it with the default and tomorrow themes with both custom settings and the behaviour is identical to what is currently on master.

belak commented 6 years ago

Wow, thanks for taking the time to do this. I'll try to take a closer look in the next few days.

keithrussell42 commented 6 years ago

Glad to help. I have the change ready for providing a choice for the mode line emphasis. As you mentioned before not everyone is a fan of the box. At the moment that change is dependent on this one but if this one doesn't go in I can rework the other fairly easily. I'll wait until this one is closed one way or the other before submitting the "choice" change. And I should be the one thanking you for maintaining this. I'm a big fan of the tomorrow theme so having an up to date version is awesome!

On Wed, Jan 17, 2018, at 21:48, Kaleb Elwert wrote:

Wow, thanks for taking the time to do this. I'll try to take a closer look in the next few days.> — 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/67#issuecomment-358520920
  2. https://github.com/notifications/unsubscribe-auth/AhxVRC4ernj3-qhUAtTnCHgKOFrOWCm7ks5tLrETgaJpZM4RiRnO
belak commented 6 years ago

Sorry it took me longer than I'd like to get to this. I'd like to see if we can come up with a better way of handling settings in general (there's a suggestion above, but I'd love to hear feedback). It still doesn't handle larger mutations to the faces, but it should simplify the code a little bit (and it would avoid having to change all the baseXX variables to :baseXX)

keithrussell42 commented 6 years ago

Sorry for the delay. I've been tied up with other things. I'm going to take a crack at this from the other direction as you suggested: leveraging the existing colour mapping to also handle options. I'll update this branch with a proposal shortly.

belak commented 6 years ago

Sounds good to me! I've been busier than I'd like as well, so I totally understand.

keithrussell42 commented 6 years ago

I've reverted the initial change to be replaced with this one. After a couple of abortive attempts at a purely lookup based approach I settled for having the color-key function handle all conversions from a symbol to a color. The problem I kept running into with a pure lookup solution was that if the lookup for the settings was to work right the values would need to be looked up in the color table for the theme after being looked up as settings or the settings values would have to be included in every theme's color table. Either way it seemed cumbersome. I thought maybe this is a satisfactory middle ground: put all of the color look-up in the same function. Now "base16-transform-color-key" will take anything which is a value to a keyword in the spec and return a valid color by either expanding special settings symbols, looking up the color in the theme's table or returning the value verbatim if neither of the other situations apply. This means the settings are at least applied in the same place as the color lookup and the application of the symbol to result in the cond is a form of table lookup if you look at it from a ridiculously abstract point of view. Basically this is what I had wanted to do originally but couldn't make work. I hope it will allow cleaner application of settings or at least more easily understood settings. I realize it's not the approach you had in mind but at this point I think that would require a much deeper change to the code which I don't know if either of us had the time to tackle right now :-). Thanks.

belak commented 6 years ago

This is quite a bit cleaner and I think it'll definitely work for now... I'll have to see if there's a way to make this more flexible, but I do appreciate your work on this!