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

[themes-megapack] Add ewal-spacemacs-themes to themes-megapack la… #12330

Closed

Conversation

cyruseuros
Copy link
Contributor

…yer.

Currently, the theme also modifies spacemacs-dark if also loaded in
dotspacemacs-themes. A
fix for this is on it's
way upstream to spacemacs-theme, but the maintainer already gave their
blessing.

@smile13241324
Copy link
Collaborator

@jjzmajic Can you add a short example for loading the new theme? Currently I am trying

   dotspacemacs-themes '(spacemacs-dark spacemacs-light leuven ewal-spacemacs-themes)

but this does not work with your PR.

@cyruseuros
Copy link
Contributor Author

ewal-spacemacs-themes is just the name of the package. It exposes 4 themes, such as ewal-spacemacs-modern for example.

Unfortunately, due to problems that will soon adressed upstream (nashamri/spacemacs-theme#144), colours in spacemacs-theme are computed at library load time/macro expansion time, so this package must always be loaded first. On Spacemacs, this is unfortunately still impossible without user intervention.

I did however make the changes (which, as discussed, have already been informally approved by the maintainer) locally and the package ran butter-smooth. I'm ok with this PR sitting untill that is patched upstream. I made it early because I was not sure I was going to have enough time this summer.

@smile13241324 smile13241324 removed their assignment Jun 9, 2019
@smile13241324
Copy link
Collaborator

Unassigned this to give someone else the chance to check this occasionally. Can only be merged when upstream fix has been merged.

@smile13241324
Copy link
Collaborator

smile13241324 commented Jun 13, 2019

Upstream merged the spacemacs-theme fix. Tested this locally but it seems not to work properly.
I am getting: error: Invalid face underline, :style, wave, :color, nil

@jjzmajic please check fix the conflicts in your branch, I will check again later.

For the records the following new themes should be available:

  • ewal-spacemacs-modern
  • ewal-spacemacs-modern-high-contrast
  • ewal-spacemacs-classic
  • ewal-spacemacs-classic-high-contrast

@smile13241324
Copy link
Collaborator

I have tested this again.
When I selected one of the new styles the style of spacemacs-dark was used. When I was restarting emacs afterwards the start fails with below error message.

When I remove the commit it works again.
image

I am afraid the upstream fix did not solve the issue.

@cyruseuros
Copy link
Contributor Author

Almost forgot about this, the upstream change doesn't matter, Spacemacs keeps a local repo of spacemacs-theme as a fallback

@duianto
Copy link
Collaborator

duianto commented Jul 13, 2019

I posted a comment that this PR is waiting for the upstream fix, in the spacemacs-theme gitter channel: https://github.com/nashamri/spacemacs-theme

The author is travelling at the moment. nashamri/spacemacs-theme#149 (comment)
but I'm sure it's on the todo list to submit a PR with the latest updates.

@smile13241324
Copy link
Collaborator

I have updated the local version of spacemacs-dark and using the new themes is causing:
Error was: (error Invalid face underline :style wave :color nil)

However emacs does not longer stop to load when the new themes have been loaded so this is a step forward.

@jjzmajic care to have another look? On develop you now have the latest version of spacemacs-themes available.

@smile13241324 smile13241324 removed their assignment Aug 3, 2019
@cyruseuros
Copy link
Contributor Author

Amazing. I'll take a look today. Moved away from Spacemacs, but as far as I'm concerned it's still an amazing project I wanna contribute to. Emacs is the dark side, Spacemacs is the cookies.

…yer.

Currently, the theme also modifies `spacemacs-dark` if also loaded in
`dotspacemacs-themes`. A
[fix](nashamri/spacemacs-theme#144) for this is on it's
way upstream to `spacemacs-theme`, but the maintainer already gave their
[blessing](nashamri/spacemacs-theme#139).
@cyruseuros
Copy link
Contributor Author

All I actually did was modify the README. It worked out of the box for me, I just had to add ewal-spacemacs-modern to dotspacemacs-themes. It sounds like it can't find your .cache/wal/colors.json file. I just made it the default to use built-in palettes when that happens. Previously, the user had to do that, but this seems like a sane default. Give MELPA an hour to build it and lmk how it goes.

@smile13241324
Copy link
Collaborator

Thanks for having another look, yeah now it looks much better.
Just one issue remains, when ewal-* has been activated once and one cycles back to spacemacs-dark the mode line still keeps the style from ewal-*.

Leuven however formats it correctly. I will try to find out why, but when you have a hint I'd be happy for :).

@smile13241324
Copy link
Collaborator

Now I now whats going on, your theme changes spacemacs-theme-custom-colors which is not a variable which custom.el can automatically restore for a theme.

You should consider applying the changes directly in your theme, these customised variables are normally defined globally in the user-config and are not meant to be changed by non user-config code i.e. emacs will not be able to reverse the changes automatically.

Anyway for spacemacs I have tweaked the way themes are cycled to restore this variable manually to the value it had after running the user-config.

This solves the problem.

Thank you for contributing to spacemacs 💜. I have cherry-picked your changes to develop.

@pangdaxing23
Copy link

pangdaxing23 commented Aug 17, 2019

I have a feeling this commit is responsible for the following error I am getting, which is stopping Spacemacs from loading

Symbol's value as variable is void: spacemacs-theme-custom-colors

Only appeared after I updated on develop.
However, I noticed that if I use an ewal theme, in this case ewal-spacemacs-modern as the startup theme, the error disappears and Spacemacs loads normally.

@duianto
Copy link
Collaborator

duianto commented Aug 17, 2019

@pangdaxing23 I think it was fixed in the next commit:
Improve theme handling to support themes using spacemacs-theme custom api 5f308b0

Because I'm not seeing that error and I updated Spacemacs after that commit.

Update:

I spoke to soon, when I change from the default themes (spacemacs-dark or spacemacs-light), to for example:

   dotspacemacs-themes '(tango)

Then I also see the error. There's an open issue about it:
"Symbol’s value as variable is void: spacemacs-theme-custom-colors" on startup #12630

@duianto
Copy link
Collaborator

duianto commented Aug 17, 2019

The broken commit has been reverted. Update Spacemacs.

@cyruseuros
Copy link
Contributor Author

Dang. I let bound the custom colors now, and I don't see the issue. Completely get it if this isn't worth the extra hassle...

@duianto
Copy link
Collaborator

duianto commented Aug 21, 2019

There's no hassle, when a bug is discovered then we'll fix it. 😄

spacemacs-theme-custom-colors is still defined as a variable (defvar spacemacs-theme-custom-colors) in:
https://gitlab.com/jjzmajic/ewal/blob/5a34be048759549937f7c701ec529b0e2d1b5798/spacemacs-themes/ewal-spacemacs-themes.el#L36

And it's set in the ewal-spacemacs-themes-get-colors function:
(setq spacemacs-theme-custom-colors ewal-spacemacs-themes-colors)
https://gitlab.com/jjzmajic/ewal/blob/5a34be048759549937f7c701ec529b0e2d1b5798/spacemacs-themes/ewal-spacemacs-themes.el#L163

@cyruseuros
Copy link
Contributor Author

Figured I'd keep the :apply keyword for convenience (the package wasn't using it), but you're right, it makes no sense. Removed! Therese also ewal-evil-cursors which would make the modeline integrate better with the theme, but Spacemacs sets the relevant variables so early in the init process I have no clue how to apply it from a layer. My previous two attempts failed. Any pointers?

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

Successfully merging this pull request may close these issues.

None yet

4 participants