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

Store extended colors in v:colornames dict. #8761

Closed
wants to merge 1 commit into from

Conversation

dvogel
Copy link

@dvogel dvogel commented Aug 15, 2021

Problem solved: Remembering RGB colors is difficult.
Solution: Allow user to (re-)define new color names.

This patch is a follow-up to the discussion on #8502.

A central question in that discussion was how users would put the new v:colornames dict to use and whether a single shared dict was appropriate. My own opinion is that a single shared dict is simultaneously the most approachable and the most usable. I've extended my previous work in an attempt to demonstrate why I think that is the case.

  • I've expanded the documentation with examples of how to update the dict in different scenarios.
  • I've created a proof-of-concept vim-colorwheel plugin that demonstrates how users might collaborate on color schemes using this shared dict.
  • I've fully removed the rgb.txt file while retaining the same list of colors.
    • That special case file has been replaced with a more generic color list script convention. In order to guard against users mistakenly thinking they should remove colors from v:colornames, the rgb.txt colors are put into a default color list, which is re-executed when a user would have expected the rgb.txt colors to be valid.
    • I've updated the Haiku color code to use the common gui color code. I've built this on Haiku and conducted simple manual tests but I don't have much expertise with Haiku, so it is possible I've missed some nuance there.

Finally, the color name code has been moved into highlight.c as suggested by @brammool. Thanks to Bram, @chrisbra, and @romainl for the helpful feedback on the last PR. I'm looking forward to any feedback you all have on this update/alternative.

@dvogel dvogel requested a review from chrisbra as a code owner August 15, 2021 20:32
@codecov
Copy link

codecov bot commented Aug 16, 2021

Codecov Report

Merging #8761 (5adc3a2) into master (844fb64) will increase coverage by 0.02%.
The diff coverage is 67.16%.

❗ Current head 5adc3a2 differs from pull request most recent head 9b8d88b. Consider uploading reports for the commit 9b8d88b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8761      +/-   ##
==========================================
+ Coverage   90.10%   90.13%   +0.02%     
==========================================
  Files         151      151              
  Lines      169669   169787     +118     
==========================================
+ Hits       152879   153030     +151     
+ Misses      16790    16757      -33     
Flag Coverage Δ
huge-clang-none 89.10% <66.66%> (+0.90%) ⬆️
huge-gcc-none 89.67% <67.16%> (-0.04%) ⬇️
huge-gcc-testgui 88.30% <61.19%> (-0.05%) ⬇️
huge-gcc-unittests 2.45% <1.49%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/term.c 83.47% <ø> (-0.14%) ⬇️
src/highlight.c 89.34% <66.66%> (-1.43%) ⬇️
src/evalvars.c 96.43% <100.00%> (ø)
src/libvterm/src/unicode.c 70.83% <0.00%> (-18.75%) ⬇️
src/xxd/xxd.c 75.00% <0.00%> (-2.09%) ⬇️
src/spellsuggest.c 93.54% <0.00%> (-1.60%) ⬇️
src/if_xcmdsrv.c 88.68% <0.00%> (-1.11%) ⬇️
src/help.c 89.76% <0.00%> (-1.08%) ⬇️
src/message.c 87.63% <0.00%> (-1.00%) ⬇️
src/netbeans.c 84.04% <0.00%> (-0.53%) ⬇️
... and 90 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 844fb64...9b8d88b. Read the comment docs.

@dvogel
Copy link
Author

dvogel commented Sep 14, 2021

@chrisbra I finally had a chance to fix the test failures on this branch. Mind approving the test run again?

@chrisbra
Copy link
Member

of course!

@brammool
Copy link
Contributor

For default.vim, instead of calling has_key() for every entry, you can use extend(v:colornames, {dict with values}, 'keep')
That should be much faster.

I notice that the loopup in rgb_table[] just uses a linear search. A binary search would be much better.
It is unrelated to v:colornames, could leave this as a todo item.

Otherwise, I'll leave this open for remarks. Is using v:colornames the best solution?

@dvogel
Copy link
Author

dvogel commented Sep 17, 2021

My original motivation for using has_key in default.vim was to remain vi compatible. I'd be fine using extend there however I tried it recently and it is significantly slower, to the point of causing the CI jobs to time out: https://github.com/dvogel/vim/actions/runs/1238981661.

@dvogel
Copy link
Author

dvogel commented Sep 18, 2021

I've pushed an additional commit that requires uses extend for better performance. The trade-off is that the default colors now require vi-incompatible line continuations. This was at the heart of why the tests were hanging with my first attempt to use extend the Test_background_foreground test relies on the color ivory. Launching that test in nocompatible mode allows it to run without hanging the test suite.

@brammool
Copy link
Contributor

See the runtime files for examples of how the 'cpo' option is changed and restored to allow for line continuation. Basically:

let s:cpo_save = &cpo
set cpo&vim

commands

let &cpo = s:cpo_save
unlet s:cpo_save

@brammool
Copy link
Contributor

Setting and restoring 'cpo' will also be needed in csscolors.vim

@brammool
Copy link
Contributor

Drew - have you looked into fixing the line continuation?

@dvogel
Copy link
Author

dvogel commented Oct 23, 2021

Yeah the color lists included in the patch all use the line continuations with the compatibility flag save/restore pattern you suggested. I haven't had a chance to look into the merge conflicts that arose from the refactor of the highlighting code yet.

Problem solved: Remembering RGB colors is difficult.
Solution: Allow user to (re-)define new color names.
@dvogel
Copy link
Author

dvogel commented Oct 23, 2021

Looks like it was just a conflict due to a tiny comment correction I made. I've rebased this branch atop the master branch, squashing my commits in the process.

@brammool brammool closed this in e30d102 Oct 24, 2021
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

3 participants