color_config now accepts closures as color values

[webbedspace]

Description

Closes #6909. You can now add closures to your color_config themes. Whenever a value would be printed with table, the closure is run with the value piped-in. The closure must return either a {fg,bg,attr} record or a color name ('light_red' etc.). This returned style is used to colour the value.

This is entirely backwards-compatible with existing config.nu files.

Example code excerpt:

let my_theme = {
    header: green_bold
    bool: { if $in { 'light_cyan' } else { 'light_red' } }
    int: purple_bold
    filesize: { |e| if $e == 0b { 'gray' } else if $e < 1mb { 'purple_bold' } else { 'cyan_bold' } }
    duration: purple_bold
    date: { (date now) - $in | if $in > 1wk { 'cyan_bold' } else if $in > 1day { 'green_bold' } else { 'yellow_bold' } }
    range: yellow_bold
    string: { if $in =~ '^#\w{6}$' { $in } else { 'white' } }
    nothing: white

Example output with this in effect:

Slightly important notes:

• Some color_config names, namely "separator", "empty" and "hints", pipe in null instead of a value.
• Currently, doing anything non-trivial inside a closure has an understandably big perf hit. I currently do not actually recommend something like string: { if $in =~ '^#\w{6}$' { $in } else { 'white' } } for serious work, mainly because of the abundance of string-type data in the world. Nevertheless, lesser-used types like "date" and "duration" work well with this.
• I had to do some reorganisation in order to make it possible to call eval_block() that late in table rendering. I invented a new struct called "StyleComputer" which holds the engine_state and stack of the initial table command (implicit or explicit).
• StyleComputer has a compute() method which takes a color_config name and a nu value, and always returns the correct Style, so you don't have to worry about A) the color_config value was set at all, B) whether it was set to a closure or not, or C) which default style to use in those cases.
• Currently, errors encountered during execution of the closures are thrown in the garbage. Any other ideas are welcome. (Nonetheless, errors result in a huge perf hit when they are encountered. I think what should be done is to assume something terrible happened to the user's config and invalidate the StyleComputer for that table run, thus causing subsequent output to just be Style::default().)
• More thorough tests are forthcoming - ran into some difficulty using nu! to take an alternative config, and for some reason let-env config = statements don't seem to work inside nu! pipelines(???)
• The default config.nu has not been updated to make use of this yet. Do tell if you think I should incorporate that into this.

User-Facing Changes

See above.

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

• cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
• cargo clippy --workspace --features=extra -- -D warnings -D clippy::unwrap_used -A clippy::needless_collect to check that you're using the standard code style
• cargo test --workspace --features=extra to check that all tests pass

After Submitting

If your PR had any user-facing changes, update the documentation after the PR is merged, if necessary. This will help us keep the docs up to date.

[webbedspace]

Requesting @zhiburt's thoughts on this…

[fdncred]

fwiw, i think this is a cool change + feature! nice idea!

[rgwood]

Haven't had a chance to look through the code, but this functionality looks very cool+useful.

[zhiburt]

Went through it lightly, seems good.

---

Thought outloud:

Maybe we could pipe more data to the closure?
For example for tables we could provide and index (row, col), table length, etc.
So we could pass like a context.

But is it worth it?

[zhiburt]

Do you think it belongs to nu_table?
I mean it could be used by many, not only for table construction, in which case there's no reason to depend on nu_table.

[webbedspace]

Did some more investigating and realised the perf isn't actually that bad after all - turns out \w in regexes is just really slow. 🐢　However, I still want to speed it up so that one could use a handful of (reasonable) regexes if they wanted to. Had a brainwave while reading the frontmatter of https://the.exa.website/ – maybe table can be parallelised like par-each.

[fdncred]

it seems like the problem we'd have with having a parallel table command is that items would be drawn in random order. although there are some parallel methods that always return a consistent order, I believe, which would be ok. it would be an interesting experiment if it could run in parallel and yet maintain the same order.

[zhiburt]

maybe table can be parallelised like par-each.

We could parallelize an initial width/height calculation which we do for each cell.
Also Value to String conversion could be.
We certainly can't do printing.
Is there anything more which could be?

[webbedspace]

I was thinking more in terms of just the scope of this PR - parallelising the running of each closure. That could be done by shoving the StyleComputer calculation further up the line. I mean, the LS_COLORS colours are applied in a loop at the top of handle_row_stream… If the rest of the colour calculation was crammed up there, then it'd also fix a bug with pre-coloured LS_COLORS strings being passed into closures… Of course, you're right that Value->String conversion would have to be moved there, too… hmm…

[webbedspace]

Managed to parallelise style closure execution in convert_to_table()… had some good perf results. Still want to try and tackle the other table-making fns before I'm satisfied.

[fdncred]

nice job parallelizing it. what type of perf results are you seeing?

[webbedspace]

Well, using a config similar to the first post (with dates and filesizes coloured by size), I benchmarked running ls in a directory with 2438 subdirs, and the results were ≈958ms before → ≈570ms after.

[fdncred]

Wow! That's a great performance improvement.

[zhiburt]

Hi there;

I wonder if you could use Vec::with_capacity and use index instead of extend.
As I see collect_into_vec must do exactly that?
(Presumably must be a little bit efficient.)

[webbedspace]

When I was writing those lines I was making some misassumptions about what Rayon methods would guarantee preserved ordering… I have a better sense of it now and can write this a bit different.

[zhiburt]

(UNRELATED)

We could parallelize an initial width/height calculation which we do for each cell.

Tested it shortly and seem like it slower on a small data set but on a bigger ones a bit faster.

	tabled	tabled_color	tabled_par
test_const_table/1	1974.4±100.72ns	1727.3±24.80ns	20.2±0.49µs
test_const_table/8	26.3±0.27µs	27.8±0.70µs	79.6±4.62µs
test_const_table/32	395.8±2.06µs	398.6±2.96µs	450.6±21.39µs
test_const_table/128	5.4±0.04ms	6.1±0.04ms	5.5±0.28ms
test_const_table/512	91.3±2.43ms	90.7±2.05ms	80.7±2.70ms
test_dynamic_table/1	1470.9±22.17ns	1453.8±28.48ns	19.3±0.53µs
test_dynamic_table/8	22.3±0.24µs	21.7±0.29µs	71.9±3.84µs
test_dynamic_table/32	338.5±3.10µs	336.8±5.03µs	401.4±21.20µs
test_dynamic_table/128	4.6±0.05ms	4.6±0.05ms	4.8±0.35ms
test_dynamic_table/512	79.8±2.25ms	85.6±2.14ms	70.3±3.30ms
test_empty_table/1	1378.8±20.19ns	1408.2±11.60ns	19.7±1.09µs
test_empty_table/8	21.2±0.15µs	18.7±0.17µs	66.6±4.17µs
test_empty_table/32	243.9±1.44µs	244.0±1.50µs	323.0±31.64µs
test_empty_table/128	3.7±0.03ms	3.7±0.03ms	4.0±0.27ms
test_empty_table/512	71.4±1.87ms	70.5±1.85ms	57.2±2.55ms
test_multiline_table/1	5.4±0.22µs	5.4±0.03µs	29.5±0.70µs
test_multiline_table/8	133.7±12.06µs	127.8±1.51µs	194.4±14.10µs
test_multiline_table/32	1777.7±12.99µs	1849.3±17.30µs	1865.5±74.60µs
test_multiline_table/128	28.3±0.18ms	28.3±0.18ms	29.1±1.86ms
test_multiline_table/512	501.1±4.18ms	496.1±2.68ms	450.9±21.15ms

PS: Just didn't know where to put it.

[fdncred]

That perf table makes me wonder if we could do something like,

if rows > 256 then
  run in parallel
else
  run normally
end

But I think that assumes the rows are collected which isn't always the case.

[zhiburt]

That perf table makes me wonder if we could do something like,

Could be worth it if there will be real benefit (hard to tell, I guess need to run in nushell to see if there are any benefits).

But I think that assumes the rows are collected which isn't always the case.

Actually we DO collect rows before the printing.
Which is actually the biggest bottle neck I am aware of.

But we basically can't eliminate it completely.
The best thing we can do is make an assumption about the width/height on a basis of some data sample (e.g. collect only 5 rows and estimate only them and truncate everything else in case it's bigger)

[fdncred]

I'm still interested in moving forward with this as time permits. I just think it's so cool to have a closure used to help determine how to color your output.

[webbedspace]

Thanks for the continued support. #7059 took longer than I expected but I'll get back to this. Hmm… I guess I need to rebase this commit 718ee3d onto it… seems I'd better just do that manually.

Starting to think I should skip further optimisations on this until after merging, if all the table making code is being constantly moved around like this…

[fdncred]

Starting to think I should skip further optimizations...

agreed. i'd wait a bit for more stabilization.

[fdncred]

Having fun playing with this. :)

[webbedspace]

OK, I've fixed up basically all the other stuff I wanted other than optimisation (namely: closure runtime errors are now printed, examples are added to default_config.nu, a few tests were added, and StyleComputer was moved to the nu_color_config crate).

Also, I adjusted the message arrow of the "All arguments to an external command need to be string-compatible" error in run_external because I needed more information from that for testing purposes, and I figured it was a microscopic enough change to leave in. Actually, nevermind.

[webbedspace]

Everything should now be ready to merge (again) (again).

[fdncred]

I agree 100% @webbedspace. I think we've done a miserable job with this PR, specifically around feedback to you and landing it when it was ready. I'm going to land it now. Thanks for all your support and patience.

[fdncred]

@webbedspace I had one more thought about this that we may have to change in a new PR. There are terminals that do not support 24-bit color, namely MacOS's terminal.app. So, things like this, might need to be ported over to indexed color.

Paying attention to only the date column, this is what it looks like in WezTerm that does support 24-bit color.

The same setting in terminal.app look like this.

I mean that dark gray isn't terrible but it's not what the user has configured. So, maybe making an indexed color the defaults in the default_config.nu is better.

[fdncred]

I tried this but it just returns them as light gray or default, i can't tell which.

  date: { (date now) - $in |
    if $in < 1hr {
      "\e[38;5;160m" #'#e61919' # 160
    } else if $in < 6hr {
      "\e[38;5;172m" #'#e68019' # 172
    } else if $in < 1day {
      "\e[38;5;184m" #'#e5e619' # 184
    } else if $in < 3day {
      "\e[38;5;112m" #'#80e619' # 112
    } else if $in < 1wk {
      "\e[38;5;40m" #'#19e619' # 40
    } else if $in < 6wk {
      "\e[38;5;44m" #'#19e5e6' # 44
    } else if $in < 52wk {
      "\e[38;5;32m" #'#197fe6' # 32
    } else { 'light_gray' }
  }

I also tried $"(ansi idx_fg)160m" and it didn't like that either. Maybe we don't have a good way to pass indexed colors?

The only other thing I can think of is adding all the xterm 256 named colors so the color resolves properly, but I'm not sure if that would work. https://www.ditig.com/256-colors-cheat-sheet https://github.com/jonasjacek/colors/blob/master/data.json

Thinking through this. We'd have to allow people to say red3 and that map to "\e38;5;160m" I think. I believe if we can make a named color to use an index, 160 in this case, it may work, but using the color as "#d70000" wouldn't work because that's a rgb color which terminal.app doesn't support.

[webbedspace]

I actually think adding all the xterm colours would be a good idea in itself (mainly because they are already relatively well-known via CSS3).

[fdncred]

I agree 100%.

I just figured out that nu-ansi-term supports these indexed colors as Fixed. So, we should be able to do something like this

    AnsiCode{ short_name: Some("red3"), long_name: "xterm_red3", code: Color::Fixed(160).prefix().to_string()},

although I haven't tested it yet.

[fdncred]

Maybe you can have better luck than I. Something is not working with this and I'm not sure why. I get this when I add it to the ansi command and do ansi --list.

but when I change all the date to be all red3 like this, it doesn't work. any ideas?

  date: { (date now) - $in |
    if $in < 1hr {
      'red3' #"\e[38;5;160m" #'#e61919' # 160
    } else if $in < 6hr {
      'red3' #"\e[38;5;172m" #'#e68019' # 172
    } else if $in < 1day {
      'red3' #"\e[38;5;184m" #'#e5e619' # 184
    } else if $in < 3day {
      'red3' #"\e[38;5;112m" #'#80e619' # 112
    } else if $in < 1wk {
      'red3' #"\e[38;5;40m" #'#19e619' # 40
    } else if $in < 6wk {
      'red3' #"\e[38;5;44m" #'#19e5e6' # 44
    } else if $in < 52wk {
      'red3' #"\e[38;5;32m" #'#197fe6' # 32
    } else { 'red3' }
  }

[webbedspace]

Doesn't the ansi command use completely separate string->colour logic? Everything in nu_color_config uses lookup_style().

[fdncred]

ugh, you're right it does. oh well, i just added them all to ansi anyway. :) Let me see if I can retrofit them into lookup_style. Thanks for the tip!

[fdncred]

aaaaarg! these xterm colors have duplicate names!

[fdncred]

yay! got the colors mapped

[webbedspace]

Just as a point of trivia, the duration-colouring example code is an approximation of a similar feature in the Windows file manager OneCommander:

This is the (loose) basis for the cutoff ranges I selected (1hr, 6hr, 1day etc).

[fdncred]

it's crazy how slow this is, string: { if $in =~ '^#\w{6}$' { $in } else { 'white' } }, especially in explore.

[webbedspace]

It's faster if, as I said upthread, you take out \w and use something like [a-fA-F\d].

[fdncred]

You're right. With explore this is much faster.

  string: { if $in =~ '^#[a-fA-F\d]+' { $in } else { 'white' } }

Here's some sample data just to play around.

['#ff0033', '#0025ee', '#0087aa', 'string', '#4101ff']

So, now, in explore I can get things like this.

[rgwood]

If anyone wants to optimize regex performance in these closures (and elsewhere), there is some low-hanging fruit for optimization:

nushell/crates/nu-protocol/src/value/mod.rs

Lines 2626 to 2628 in 046e46b

	// We are leaving some performance on the table by compiling the regex every time.
	// Small regexes compile in microseconds, and the simplicity of this approach currently
	// outweighs the performance costs. Revisit this if it ever becomes a bottleneck.

I imagine we could do something like a global cache of compiled regexes, based on a FIFO queue.