search

senny / cabbage

get the maximum out of emacs
http://senny.github.com/cabbage/
156 stars 21 forks source link

extract theme-roller into a bundle #209

Closed tonini closed 9 years ago

tonini commented 9 years ago

@senny because of the improvements of the deftheme macro in emacs 24, I think there should be the possibility to choose between theme-roller or themes served via deftheme. There are already a lot of nice themes out there.

What do you think?

senny commented 9 years ago

Wouldn't this PR make lib/themes obsolete? If so, why not remove it?

tonini commented 9 years ago

@senny you're right. I made the update

senny commented 9 years ago

I'm generally OK with this PR, sad thing is that after the update everyone will loose their themes (because theme-roller bundle won't be loaded). Can you think of a less painful upgrade path?

tonini commented 9 years ago

@senny I think this is not that easy to achieve. In this situation the user is actually required to update his config.el.

What we could do it add a message to the install.sh  and update.sh scripts about this change.

senny commented 9 years ago

What about using cabbage-load-bundle to load theme-roller if it hasn't been loaded by the users config.el. If this branch is triggered, we can output a deprecation warning that this will no longer be the case in future releases and that one needs to add theme-roller to the config.el. Adding it would obviously get rid of that message. Also if there is a way to tell wether the user is using a deftheme, we can skip that loading / message stuff as well.

tonini commented 9 years ago

Ok, the theme-roller will be loaded if not already, and a deprecated warning will be shown.

There is actually no way to find out if already a theme is used created by the deftheme macro.

tonini commented 9 years ago

@senny you're right. The warn function is a much better choice because its shown in a separate buffer. I made the change

senny commented 9 years ago

Maybe you can find something about the use of deftheme using the custom-enabled-themes variable. It appears as it would have an entry when you are using load-theme or enable-theme. I can't say for sure though.

tonini commented 9 years ago

I think you're right. The custom-enabled-themes variable will be updated with the theme you loaded via load-theme.

Let me update the code

tonini commented 9 years ago

@senny OK I made the update.

senny commented 9 years ago

@tonini added a few more comments. Sorry for all the nitpicks. Working on rails made me more sensible to a clear and compatible upgrade path :blush:

tonini commented 9 years ago

@senny no worries mate. :-) I like that kind of nitpicks. :-) I'm always willing to learn :+1:

check the update :)

tonini commented 9 years ago

@senny thanks, great summary of how to make deprecated message :+1:

check the update...;-)

senny commented 9 years ago

@tonini thanks for your work and endurance :yellow_heart:

Check out the merge commit (https://github.com/senny/cabbage/commit/8a6bcc6dc73e321652ee8ebe4aea4dd25ff48740) I rephrased the message a little bit and simplified the (and slightly.

tonini commented 9 years ago

@senny great! :) thanks for letting me contribute. ;-)