bbatsov / solarized-emacs

The Solarized colour theme, ported to Emacs.
775 stars 176 forks source link

Avoid side effects on install/build #325

Open colonelpanic8 opened 5 years ago

colonelpanic8 commented 5 years ago

Solarized is one of the only themes that seems to have side effects that somehow result from an update/install. I haven't had time to investigate why this is, but this should probably be avoided.

conao3 commented 5 years ago

What side effect you seen? You tell me more information, I can try work on it.

thomasf commented 5 years ago

It applied facedefs while byte compiling/loading the -theme files, idk if it was fixed or not.. Not really a big practical issue. I havent seen it myself for many years IIRC, I don't know what you have to do to experience it as a problem.

(let ((custom--inhibit-theme-enable nil)) could potentially have brought it back if it was fixed, I don't really understand what the documentation of that variable implies.

tangxinfa commented 4 years ago

I encountered the same problem, run the code snippet below:

(require 'solarized-theme)
(load-theme 'solarized-dark-high-contrast nil t)

The solarized-dark-high-contrast theme will pollution my current emacs theme, even if i asked it just load and not enable.

I am writing a theme switch tool which loaded all themes but didn't enable any one, so i can generate a menu with theme name and documentation information, so the user can select one to enable, it looks like bellow:

afternoon                                Dark color theme with a deep blue background
brin                                     Space Grey theme for Emacs
dorsey                                   A dark grunge color theme
fogus                                    A bluecolor theme
graham                                   A dark color theme for Emacs
tango-dark                               Face colors using the Tango palette (dark background).
tango                                    Face colors using the Tango palette (light background).
tsdh-dark                                A dark theme used and created by Tassilo Horn.
tsdh-light                               A light Emacs theme.
wheatgrass                               High-contrast green/blue/brown faces on a black background.
whiteboard                               Face colors similar to markers on a whiteboard.
wombat                                   Medium-contrast faces with a dark gray background.

Many popular themes are works well, except solarized-theme, all it's themes not support load and not enable behavior.

conao3 commented 4 years ago

When you run load-theme you end up running a lot of custom-set-faces and custom-set-variables. Therefore, the effect will remain if the theme that is subsequently activated does not successfully overwrite the value set by solarized. This is an Emacs specification.

Now, the original issue was that there were side effects when update/install (maybe at byte-compiling). The thread owner and you reported are different.

thomasf commented 4 years ago

I have a feeling that (let ((custom--inhibit-theme-enable nil)) prohibits the NO-ENABLE argument of load-theme to work correctly (load-theme THEME &optional NO-CONFIRM NO-ENABLE). I'm not sure if this is currently easy to resolve given the way child themes currently works.

It seems like we could change the order of child theme and base solarized face defs are run but I'm not sure if that causes other unwanted scenarios instead (base anc child facesdes not being merged?).

I have a feeling that the proper solution would be to write code that merges custom-theme-set-faces and custom-theme-set-variables section from child theme and parent before they are actually executed.

I am not sure I want to make invasive changes to how that works right now since we just change it recently so if we are going to resolve it now it has to be compatible with how child themes are set up now.

thomasf commented 4 years ago

I encountered the same problem, run the code snippet below:

(require 'solarized-theme)
(load-theme 'solarized-dark-high-contrast nil t)

The solarized-dark-high-contrast theme will pollution my current emacs theme, even if i asked it just load and not enable.

I am writing a theme switch tool which loaded all themes but didn't enable any one, so i can generate a menu with theme name and documentation information, so the user can select one to enable, it looks like bellow:

afternoon                                Dark color theme with a deep blue background
brin                                     Space Grey theme for Emacs
dorsey                                   A dark grunge color theme
fogus                                    A bluecolor theme
graham                                   A dark color theme for Emacs
tango-dark                               Face colors using the Tango palette (dark background).
tango                                    Face colors using the Tango palette (light background).
tsdh-dark                                A dark theme used and created by Tassilo Horn.
tsdh-light                               A light Emacs theme.
wheatgrass                               High-contrast green/blue/brown faces on a black background.
whiteboard                               Face colors similar to markers on a whiteboard.
wombat                                   Medium-contrast faces with a dark gray background.

Many popular themes are works well, except solarized-theme, all it's themes not support load and not enable behavior.

You can probably work around the issue at by unloading the theme again after loading it. IIRC that removes any faces applied during the load theme phase regardless of how it's set. It will probably still flash new faces temporarily, maybe it's possible to inhibit fontification (or whatever applies faces to displays) while loading/unloading to hide visual side effects .

tangxinfa commented 4 years ago

I have a feeling that (let ((custom--inhibit-theme-enable nil)) prohibits the NO-ENABLE argument of load-theme to work correctly (load-theme THEME &optional NO-CONFIRM NO-ENABLE). I'm not sure if this is currently easy to resolve given the way child themes currently works.

It seems like we could change the order of child theme and base solarized face defs are run but I'm not sure if that causes other unwanted scenarios instead (base anc child facesdes not being merged?).

I have a feeling that the proper solution would be to write code that merges custom-theme-set-faces and custom-theme-set-variables section from child theme and parent before they are actually executed.

I am not sure I want to make invasive changes to how that works right now since we just change it recently so if we are going to resolve it now it has to be compatible with how child themes are set up now.

@thomasf You are right, the sentence (let ((custom--inhibit-theme-enable nil)) in solarized.el prohibits the NO-ENABLE argument of load-theme to work correctly.