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

Color scheme issue #3455

Open
mdziczkowski opened this issue Sep 6, 2024 · 9 comments
Open

Color scheme issue #3455

mdziczkowski opened this issue Sep 6, 2024 · 9 comments

Comments

@mdziczkowski
Copy link

Environment

  • OS: Debian (bookworm)

  • Micro version: 2.0.13

  • Micro commit: 68d88b5

Issue

Taken steps

  • created a colorschemes folder inside of micro configuration folder

  • created a colorscheme json file

Expected result

Apply of the colorscheme

Actual result

Error message:

The colorscheme is invalid

even if the .json file syntax is correct

@JoeKar
Copy link
Collaborator

JoeKar commented Sep 6, 2024

  • created a colorscheme json file [...] even if the .json file syntax is correct

micro's color schemes aren't any json files. They are called [NAME].micro.
See colors.md and colorschemes.

@mrvruj
Copy link

mrvruj commented Sep 6, 2024

I have had a colorscheme working for a long time, recently broken. It is in a folder in .../micro/plug/colors/colorscheme-16.micro. When I launch micro, it yells at me "colorscheme-16 is not a valid colorscheme". When I open micro and press ctrl-e, set colorscheme colorscheme-16, it works properly for that instance. When I close micro and open again, the same issue arises.

@Andriamanitra
Copy link
Contributor

I was able to reproduce the issue by having a plugin set up like this:

-- ~/.config/micro/plug/asd/asd.lua
local config = import("micro/config")

function preinit()
    config.AddRuntimeFile("asd", config.RTColorscheme, "asd.micro")
end
# ~/.config/micro/plug/asd/asd.micro
color-link cursor-line "#FF00FF"

It seems that the issue is caused by micro attempting to load colorschemes before the plugin's preinit function runs to initialize the runtime files. I'm not sure if loading a colorscheme to use on startup from plugin ever worked: I tried out release versions v2.0.10-v2.0.14 and got the same asd is not a valid colorscheme error in all of them. Switching to the colorscheme after startup works as expected.


@mrvruj
I have had a colorscheme working for a long time, recently broken. It is in a folder in .../micro/plug/colors/colorscheme-16.micro. When I launch micro, it yells at me "colorscheme-16 is not a valid colorscheme". When I open micro and press ctrl-e, set colorscheme colorscheme-16, it works properly for that instance. When I close micro and open again, the same issue arises.

For now you can get around this by placing the colorscheme file directly in ~/.config/micro/colorschemes/colorscheme-16.micro instead of loading it from a plugin.

@JoeKar
Copy link
Collaborator

JoeKar commented Sep 7, 2024

It seems that the issue is caused by micro attempting to load colorschemes before the plugin's preinit function runs to initialize the runtime files. I'm not sure if loading a colorscheme to use on startup from plugin ever worked: I tried out release versions v2.0.10-v2.0.14 and got the same asd is not a valid colorscheme error in all of them. Switching to the colorscheme after startup works as expected.

Yes, you're right, since the order of...

micro/cmd/micro/micro.go

Lines 322 to 330 in 5428b3f

err = config.InitColorscheme()
if err != nil {
screen.TermMessage(err)
}
err = config.RunPluginFn("preinit")
if err != nil {
screen.TermMessage(err)
}

...definitely affects this behavior, but it's just half of the truth, because we validate every option already in the moment we parse the settings (4663927) to be sure to work with fully valid settings and in the moment of validating the colorscheme settings we're still far away of adding the scheme via the plugin.
So someone could indeed call it a regression, but this scenario is then hard to fix due to circle dependencies...at least at first sight.

For now you can get around this by placing the colorscheme file directly in ~/.config/micro/colorschemes/colorscheme-16.micro instead of loading it from a plugin.

Yes, I think this is and should be the common approach to load colorschemes.
Doing this via plugins doesn't work right now (any longer).

@aeadio
Copy link

aeadio commented Sep 19, 2024

I just want to say that this appears as a regression in some existing popular color schemes, which were already loading via plugins, such as https://github.com/KiranWells/micro-nord-tc-colors. With this change, it seems to become impossible to distribute color schemes as plugins now?

Can I also make a recommendation to disable the error/warning on startup entirely?

The color scheme is set in settings.json, which I store in my dotfiles repo that I pull down on any new machine(s) I'm working on. I imagine this is a common workflow. I gitignore plugins, because those are separately versioned and may differ per workflow. So when pulling down my config on a new machine, I'm hit with an error on every startup, until I install the associated color scheme / plugin.

On the other hand, if a color scheme is failing to load or is not installed, it should be readily apparent to the user as soon as they see the editor. The warning on startup is not actionable. It's just a nag. Nags are categorically not good UX design.

@JoeKar
Copy link
Collaborator

JoeKar commented Sep 19, 2024

With this change, it seems to become impossible to distribute color schemes as plugins now?

Yes, but due to the design of micro it was never the best idea to mix them up together...it over-complicates a lot of things now.

Can I also make a recommendation to disable the error/warning on startup entirely?

I don't think that it's a wise choice to "ignore" some kind of inconsistency in the config.

The color scheme is set in settings.json, which I store in my dotfiles repo [...] pulling down my config on a new machine, I'm hit with an error on every startup, until I install the associated color scheme / plugin.

Yep, that is the current situation. But there is at least one way to bypass this with your dotfiles too. Why don't you maintain your own .config/micro/colorschemes/ folder in your dotfiles repository, where you store the colorschemes the settings of micro pointing to?

On the other hand, if a color scheme is failing to load or is not installed, it should be readily apparent to the user as soon as they see the editor.

Try to imagine some very simple colorschemes where only a few details should be highlighted, which could be overseen at the first sight.

We can try to handle this "regression", but I find it difficult to prioritize.

@dmaluka:
What's your opinion about this? Seems we've unintentionally increased the plugin vs colorscheme mess.

@aeadio
Copy link

aeadio commented Sep 19, 2024

Try to imagine some very simple colorschemes where only a few details should be highlighted, which could be overseen at the first sight.

But a stop-the-world style interruption at startup feels like a drastic overstep for a pretty niche case. Why doesn't this display as, say, a warning in the gutter after startup? The worst case being solved for here (a color scheme failed to load) is pretty benign, versus the jarring experience of the entire editor stopping in its tracks. It fails the principle of least surprise.

@dmaluka
Copy link
Collaborator

dmaluka commented Sep 20, 2024

I just want to say that this appears as a regression in some existing popular color schemes, which were already loading via plugins, such as https://github.com/KiranWells/micro-nord-tc-colors.

Hmm, this plugin calls config.AddRuntimeFile() from its top-level code, not from init() or preinit() callbacks. Very smart (no). I believe this way of using micro functions from lua was never guaranteed to work, never documented, it works by chance (AddRuntimeFile() happens to be implemented in such a way that it doesn't touch any data structures that are initialized after loading plugins). However, this explains the miracle: as @Andriamanitra correctly noticed, loading colorschemes via plugins at startup in a correct way (i.e. from init() or preinit()) never really used to work, however loading colorschemes via this plugin with this trick used to work until 2.0.14.

I wonder how many other plugins use this trick.

...Actually I've just found out that this trick is not something the plugin author came up with. This trick was suggested by @zyedidia: KiranWells/micro-nord-tc-colors#2 (comment)

More and more interesting.

BTW even before 2.0.14 it wasn't fully working: loading colorscheme specified in settings.json worked, however loading colorscheme specified in micro command line (micro -colorscheme nord-16 foo.txt) didn't. (Whereas colorschemes loaded normally from ~/.config/micro/colorschemes, not via plugins, worked and still work successfully in both cases.)

@dmaluka
Copy link
Collaborator

dmaluka commented Sep 20, 2024

On the other hand, if a color scheme is failing to load or is not installed, it should be readily apparent to the user as soon as they see the editor.

No, it is not apparent to the user why the colorscheme was not applied. foo is not a valid colorscheme at least indicates that micro has successfully read the foo setting from settings.json, before it failed to apply this setting.

But a stop-the-world style interruption at startup feels like a drastic overstep for a pretty niche case. Why doesn't this display as, say, a warning in the gutter after startup?

There may be multiple configuration errors at startup. With a "stop-the-world interruption" we can report them all to the user, one by one, whereas with a warning in the gutter we would only be able to show one of them.

...Let's think how to address the root issue (non-working colorschemes loading via plugins at startup, in particular with existing plugins), then we won't need to think how to hide its consequences.

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

6 participants