feat(extra): add colorscheme picker

[pkazmier]

• I have read CONTRIBUTING.md
• I have read CODE_OF_CONDUCT.md

[pkazmier]

First draft. Next steps:

1. Use 'mini.colors' (if available) to get and restore colorscheme. This will restore any changes a user has made in between setting their original colorscheme and running this picker. Also, if a colorscheme does not set vim.g.colors_name.
2. Add tests.

[pkazmier]

With regards to the placement of the one-time autocommand (didn't know these were a thing, very cool). I placed it in the items callback as that ensures we are already in a colorscheme picker, so presumably we are guaranteed that the MiniPickStop will be executed. Just trying to make sure we don't leave any unexecuted one-time auto commands. For example, if the user somehow interrupted between the setting of the auto command and pick_start. I don't really know if that is a possibility or not, but that was my logic.

And, I was unsure of how defensive you like to get, but I suppose to be really cautious, I could create a auto command group that clears any old unexecuted auto commands when the picker is created. That prevents stacking of more than one auto commands that would load a theme. And, also, to be extra safe, I could check the source name in the callback to make sure it was from this colorscheme picker. Again, I'm just not sure how defensive you want to be and if it is really necessary.

I'll defer to you as you are the expert.

[echasnovski]

Thanks for the PR! For posterity, this is a result of #951 discussion.

With regards to the placement of the one-time autocommand ...

I think the whole autocommand approach is unnecessary. Any picker waits for the user to finish action and resumes code execution afterwards. So the load_colorscheme equivalent can be done after H.pick_start call (while still the whole picker returning the output of H.pick_start).

[pkazmier]

I think the whole autocommand approach is unnecessary. Any picker waits for the user to finish action and resumes code execution afterwards. So the load_colorscheme equivalent can be done after H.pick_start call (while still the whole picker returning the output of H.pick_start).

Even better. I removed the autocommand and now preserve the background setting of the user. Tomorrow will look at the mini.color integration as lunch time is over 😄

[pkazmier]

Added 'mini.color' support. Two versions of the picker are included: colorschemes_1 and colorschemes_2.

The first version is more concise and does not try to avoid setting the colorscheme more than once. For example, in that version, if you open the picker and don't preview or select anything, when you quit, your original colorscheme is set. Likewise, if you preview a colorscheme (results in setting the colorscheme) and then select it, it will set the colorscheme again.

The second option is more verbose, but it avoids setting the colorscheme unnecessarily. So, my question to you is what is more important in this specific case: conciseness or avoiding the extra calls to set the colorscheme? As a novice, I guess I don't see the big deal if the colorscheme is set multiple times and I think I favor the conciseness of version 1 given the spirt of mini being as small as possible.

Just looking for your feedback. Thanks.

[echasnovski]

Yeah, let's try variant 1 first. There indeed shouldn't be too much harm (other than couple of milliseconds of delay) in applying color scheme.

[pkazmier]

Thanks for the feedback and suggestions!

[pkazmier]

Wow. Your 'mini.test' library is very cool and the 'TESTING.md' document was perfect to get me started. I've now added some initial tests, but I need help the "cancel without mini.colors" test. When that test runs, I need to make sure that 'mini.colors' is not available or loaded. I tried different things but could not get it working. The point of this test is to prove that a custom change to a highlight group is not persisted when mini.colors is not present.

[pkazmier]

Updated the tests to use minicyan and minischeme. Thanks for the suggestion on how to prevent 'mini.colors' from loading in the child. Now I'm able to demonstrate restoring the original colorscheme both with and without 'mini.colors'. Of course, the former has the benefit of preserving any custom changes a user has made (and the tests demonstrate that).

[echasnovski]

Thanks, I will take a closer look after 0.16.0 release.

Approved CI to run. Don't worry about commit style checks, those will be squashed. Nice job for making tests pass on all supported versions!

[pkazmier]

Thanks for giving me the opportunity to collaborate with you. I learned a LOT even with this very small contribution. And I always thought testing in Neovim would be a nightmare, but I was pleasantly surprised. The screenshot capability is very cool as is the use of the child.

Now back to revamping my mini configuration, which I’m redoing based on yours. I like using the top-level directories in the config folder. There is nothing much left in my Lua directory, which is so different than any config I’ve ever had since moving to vim from emacs three years ago.

[echasnovski]

Thanks for giving me the opportunity to collaborate with you. I learned a LOT even with this very small contribution. And I always thought testing in Neovim would be a nightmare, but I was pleasantly surprised. The screenshot capability is very cool as is the use of the child.

Thanks for taking interest! There are more "good-first-issue" type of feature requests, if you have time and want to continue (which I'll gladly accept).

Now back to revamping my mini configuration, which I’m redoing based on yours. I like using the top-level directories in the config folder. There is nothing much left in my Lua directory, which is so different than any config I’ve ever had since moving to vim from emacs three years ago.

Yeah, using 'plugin/' for user config is somewhat counter-intuitive, but nice overall. It is not without its issues, though. For example, nvim -u path/to/init.lua won't work (as it will still source '~/.config/nvim/plugin/') and needs nvim --noplugin -u path/to/init.lua. And "bisecting config" is a bit more verbose as in its strictest form requires commenting out all lines in all files in 'plugin/' one-by-one. The other approach is to :source files one-by-one, but that is not 100% replacement.

[pkazmier]

There are more "good-first-issue" type of feature requests, if you have time and want to continue

Yes. I would like to help where I can after I've revamped my configuration. When done I'll take a look at your issues list for something that looks easy. Obviously, your definition of easy will be quite different from mine 😄

[echasnovski]

When done I'll take a look at your issues list for something that looks easy. Obviously, your definition of easy will be quite different from mine 😄

#651 and #1790 look fairly straightforward. Others - not so much.

[pkazmier]

Shoot. I meant to submit that last commit as part of a different PR. I'll revert later as I have to head out now.

[abeldekat]

When using the mini.nvim repository, mini.colors is always present. Perhaps it is better to only use mini.colors if it has been setup. Otherwise, mini.colors always parses the current colorscheme, which seems unnecessary.

Suggested change

	local has_colors, colors = pcall(require, 'mini.colors')
	local selected_cs = has_colors and colors.get_colorscheme() or vim.g.colors_name
	local selected_cs = MiniColors ~= nil and MiniColors.get_colorscheme() or vim.g.colors_name

[abeldekat]

I am looking forward to using this new picker!

Would it be possible to have a local_opt, specifying a list of strings that represent colors I don't want to see? I currently remove builtin colors.

[echasnovski]

Would it be possible to have a local_opt, specifying a list of strings that represent colors I don't want to see? I currently remove builtin colors.

Hmm... Interesting idea. I wonder if the ignore_builtins option is better here instead of custom array of color scheme names. Although with a more flexible name array approach the ignore_builtins can be a documented example (and easily transformable into "show only builtins").

I plan to polish (still need to decide if there needs to be a more informative preview text) and merge this week.

[echasnovski]

I wanted to merge it today to backlog to try out and add local_opts parts which I decided to add, but there were merge conflicts. And as this PR is from the main branch, I'd not want to mess around by force pushing there.

So I squashed and merged this locally, plus added (quite a few, unfortunately) changes. The current result is in backlog branch. Will take a fresh look tomorrow before merging.

[echasnovski]

(A version of) This is now merged into latest main. I decided to add couple of local_opts that I personally find useful. This visibly increased complexity and line count, but I am certain I'll use these features in the future when creating new highlight groups to test against popular color schemes.

Thanks again @pkazmier for the initial implementation and PR!

[pkazmier]

Nice! Looking forward to adding it to my config later today. Good idea on previewing with highlight groups. Seems so obvious in hindsight.