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

Loading palette or spec before colorscheme disrupts colors #362

Closed
tmillr opened this issue Aug 7, 2024 · 0 comments · Fixed by #363
Closed

Loading palette or spec before colorscheme disrupts colors #362

tmillr opened this issue Aug 7, 2024 · 0 comments · Fixed by #363
Labels
bug Something isn't working

Comments

@tmillr
Copy link
Member

tmillr commented Aug 7, 2024

Description

Basically if (as a user) you require() the palette before loading the colorscheme, then you load the colorscheme, some of the colors will be off and will be different than if you had never required/loaded the palette.

Explanation

I've mentioned this bug before (where it was affecting the colors of my Airline status-line: see the comments on #290) and it has to do with:

  1. Our direct, or indirect (e.g. via methods such as :alpha_blend()), use of the global-ish static/class properties of the Color class (i.e. Color.{WHITE,BLACK,BG})
  2. How/where/when those properties are set (currently these are set at the top-level of each theme's palette file at lua/github-theme/palette/*.lua)
  3. The fact that loading the colorscheme causes compilation (sometimes), and compilation in-turn require()'s each and every theme's palette file consecutively.
  4. The fact that, when requiring a file in Lua, the top-level code of that file is only run once (on the initial require).

The colors affected will be any color which depends on the aforementioned static properties (e.g. any color using alpha_blend() that is not defined at the top-level of a palette file).

All that's needed to trigger the bug is to directly or indirectly require a palette prior to compiling. Since the global/static Color.* fields are set at the top-level of each palette file, and since previously-required files are cached in Lua, they only get set once the first time a palette is required. Later, when the colorscheme is loaded and a compilation happens, these fields won't be set again for the themes whose palette was previously-required (cached) (but they will be set for/by non-previously-required palettes). So, when the palette/spec that the user required is then loaded by the colorscheme (during compilation), it will use incorrect values for Color.{WHITE,BLACK,BG} (which will still be set to whatever the last non-cached palette file set them to). Hopefully that all makes sense.

Solution

There's probably different ways to solve this, but I think we ought to go about this by simply stopping the internal use Color.{WHITE,BLACK,BG} and alpha_blend() altogether (both now, and in the future). Using these shared, global, static/class fields of Color, it's just too easy to shoot ourselves in the foot and cause bugs (and bugs which are hard to track-down, and even hard to describe), and requiring a palette file should not have side effects like this. The only question is whether we leave them defined for users for convenience, although they can run into similar issues (e.g. by depending on those fields and falsely assuming that they are set to correct values before the colorscheme has been loaded).

@tmillr tmillr added the bug Something isn't working label Aug 7, 2024
@tmillr tmillr changed the title Using palette or spec before loading colorscheme causes bug Loading palette or spec before colorscheme causes bug Aug 8, 2024
tmillr added a commit to tmillr/github-nvim-theme that referenced this issue Aug 8, 2024
tmillr added a commit to tmillr/github-nvim-theme that referenced this issue Aug 8, 2024
tmillr added a commit to tmillr/github-nvim-theme that referenced this issue Aug 8, 2024
tmillr added a commit to tmillr/github-nvim-theme that referenced this issue Aug 8, 2024
tmillr added a commit to tmillr/github-nvim-theme that referenced this issue Aug 9, 2024
tmillr added a commit to tmillr/github-nvim-theme that referenced this issue Aug 9, 2024
tmillr added a commit to tmillr/github-nvim-theme that referenced this issue Aug 9, 2024
@tmillr tmillr changed the title Loading palette or spec before colorscheme causes bug Loading palette or spec before colorscheme disrupts colors Aug 9, 2024
tmillr added a commit to tmillr/github-nvim-theme that referenced this issue Aug 9, 2024
Fix bug where the colorscheme colors are off/incorrect when a palette is
require()'d, whether directly or indirectly, anytime before compilation
occurs (e.g. compilation that occurs during colorscheme load). For
details, see issue projekt0n#362.

Also, add a test (in `test/`) which disallows the direct or indirect use
of the global static/class fields of the `Color` class (e.g.
`Color.BG`).

Fixes: projekt0n#362
tmillr added a commit to tmillr/github-nvim-theme that referenced this issue Aug 9, 2024
Fix bug where the colorscheme colors are off/incorrect when a palette is
require()'d, whether directly or indirectly, anytime before compilation
occurs (e.g. compilation that occurs during colorscheme load). For
details, see issue projekt0n#362.

Also, add a test (in `test/`) which disallows the direct or indirect use
of the global static/class fields of the `Color` class (e.g.
`Color.BG`).

Fixes: projekt0n#362
@tmillr tmillr closed this as completed in 742c701 Aug 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant