highlight path separators

[m0vie]

This commit is a squash of the work done by
Jorge Israel Peña (blaenk) in #136,
rebased on the latest master branch.

For now, this is disabled in tests, because they all would need to be adjusted.

TODO: If the ZSH_HIGHLIGHT_DISABLE_PATHSEP option should stay, it needs to be documented.

[m0vie]

Updated the commit. Removed the global option and adjusted the test framework to disable separator highlighting by default.

Tests that actually test the separator handling override these defaults.

[m0vie]

Rebased on latest master for changes to the tests.

[danielshahaf]

1. I think highlighting path separators differently by default would be too colorful. I think [path_separator] should default to $ZSH_HIGHLIGHTING_STYLES[path] if unspecified. (Not to underline but literally to $ZSH_HIGHLIGHTING_STYLES[path], so as to not break existing zshrc's that override the [path] style.)

   Once we have themes (Add support for themes #241), you could add a theme that enables path separators by default, if you like.

2. I'm concerned about the performance of the arithmetic for loop, particularly with large paths. Does it scale well?

I haven't tested this nor reviewed the code in context; so far, I only read the diff.

[danielshahaf]

Re the arithmetic for loop: see #295 (comment)

[m0vie]

I think highlighting path separators differently by default would be too colorful.

Agreed. Changed to use the same colors as the regular path highlighter.

I'm concerned about the performance of the arithmetic for loop, particularly with large paths. Does it scale well?

Tested by loading zprof, entering a very long path, hitting backspace till the buffer was cleared, ran zprof:

num  calls                time                       self            name
-----------------------------------------------------------------------------------
 1)  159         813.66     5.12   27.05%    804.95     5.06   26.76%  _zsh_highlight_main_highlighter_check_path
 2)  181        2984.27    16.49   99.20%    746.50     4.12   24.81%  _zsh_highlight_call_widget
 3)  165         449.48     2.72   14.94%    449.48     2.72   14.94%  _zsh_highlight_brackets_highlighter
 4)  156         458.99     2.94   15.26%    395.64     2.54   13.15%  _zsh_highlight_main_highlighter_highlight_path_separators
 5)  164        1621.76     9.89   53.91%    311.19     1.90   10.34%  _zsh_highlight_main_highlighter
 6)  181        2237.03    12.36   74.36%    129.89     0.72    4.32%  _zsh_highlight

I added a further check to return early from the function if the override style is the same as the original and the loop would be useless:

num  calls                time                       self            name
-----------------------------------------------------------------------------------
 1)  159         799.90     5.03   32.04%    790.95     4.97   31.68%  _zsh_highlight_main_highlighter_check_path
 2)  177        2478.52    14.00   99.28%    676.63     3.82   27.10%  _zsh_highlight_call_widget
 3)  165         478.17     2.90   19.15%    478.17     2.90   19.15%  _zsh_highlight_brackets_highlighter
 4)  164        1167.72     7.12   46.78%    321.76     1.96   12.89%  _zsh_highlight_main_highlighter
 5)  177        1801.26    10.18   72.15%    118.70     0.67    4.75%  _zsh_highlight
...
12)  156           8.07     0.05    0.32%      8.07     0.05    0.32%  _zsh_highlight_main_highlighter_highlight_path_separators

Regarding the arithmetic for loop:
Using for pos in {${start_pos}..${end_pos}} ; do isn't any faster.

[danielshahaf]

Thanks for the performance investigations! It seems the updated version has next-to-nil performance effect on people who don't opt-in to the new feature, which is good.

I'm concerned about the test changes that stop setting [path_prefix] to $unused_highlight. This may undermine the existing tests' functionality; you could just use a $unused_highlight2 for [path_prefix_pathseparator] and keep the existing setting of [path_prefix].

What do you think of my suggestion from earlier? I see you haven't applied it but you haven't explained why.

I think [path_separator] should default to $ZSH_HIGHLIGHTING_STYLES[path] if unspecified. (Not to underline but literally to $ZSH_HIGHLIGHTING_STYLES[path], so as to not break existing zshrc's that override the [path] style.)

[m0vie]

I'm concerned about the test changes that stop setting [path_prefix] to $unused_highlight. This may undermine the existing tests' functionality; you could just use a $unused_highlight2 for [path_prefix_pathseparator] and keep the existing setting of [path_prefix].

Done

What do you think of my suggestion from earlier? I see you haven't applied it but you haven't explained why.

I think [path_separator] should default to $ZSH_HIGHLIGHTING_STYLES[path] if unspecified. (Not to underline but literally to $ZSH_HIGHLIGHTING_STYLES[path], so as to not break existing zshrc's that override the [path] style.)

I totally misread that. Fixed now.

[danielshahaf]

(When pushing a PR'd branch, please also add a comment to the PR so that a github notification would be generated to those following the PR.)

[danielshahaf]

I've fixed a whitespace issue and merged this in 6cd39e7. Thank you for your patience and thanks to @phy1729 for reviewing this!

[blaenk]

Good work!

[danielshahaf]

Please add documentation for the new feature. (At least the style names need to be mentioned in main.md)

[m0vie]

I updated the documentation and also made a small change to the style initialization: Set the default styles to the empty string. This has the same initial effect and the advantage, that the main styles can be changed in a running shell without the need to change the _separator styles too, to keep the feature disabled.

[danielshahaf]

1. Typo: "unsed"

2. The fact that ${foo}_pathseparator being unset causes $foo to be used should be documented.

3. +1 to 9a934d2, good call. (The previous way wasn't backwards compatible.)

[m0vie]

Fixed 1) and 2)

[danielshahaf]

I've changed a comma to semicolon and merged. Thanks!

[danielshahaf]

(By the way: I wouldn't have capitalized the "P" of "Path separators" in the first line of the log message, since it's neither a proper noun nor a sentence start; it's just a topical noun phrase. But I didn't want to drive you crazy with minor changes..)