Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid side effects on install/build #325

Open
colonelpanic8 opened this issue Feb 14, 2019 · 7 comments
Open

Avoid side effects on install/build #325

colonelpanic8 opened this issue Feb 14, 2019 · 7 comments

Comments

@colonelpanic8
Copy link

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
Copy link
Contributor

conao3 commented Nov 2, 2019

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

@thomasf
Copy link
Collaborator

thomasf commented Nov 3, 2019

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
Copy link

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
Copy link
Contributor

conao3 commented Dec 3, 2019

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
Copy link
Collaborator

thomasf commented Dec 3, 2019

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
Copy link
Collaborator

thomasf commented Dec 4, 2019

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants