[-- Attachment #1: Type: text/plain, Size: 1541 bytes --] Hi there, devs! I am using `zsh-syntax-highlighting` and I want my paths to be blue, but the slashes in there to be my terminal's default text color. I've tested it and `zsh-syntax-highlighting` correctly puts, for example, ``` region_highlight=( '0 2 fg=10' '3 16 fg=4' '3 4 fg=default' '9 10 fg=default' ) ``` where `3 16` is the path I'm trying to `cd` to and `3 4` plus `9 10` are the positions of slashes in the path. However, after this assignment occurs, when I `print -r "${(q+)region_highlight[@]}"`, I get as output ``` '0 2 fg=10' '3 16 fg=4' '3 4 none' '9 10 none' ``` and when the ZLE highlights the line, the slashes are rendered in the same blue color as the rest of the path. If I use any other `fg` value than ` default`, then the slashes are colored correctly. This seems like incorrect behavior to me, on two accounts: 1. It seems incorrect to convert `fg=default` to `none`. From reading the [documentation]( http://zsh.sourceforge.net/Doc/Release/Zsh-Line-Editor.html#Character-Highlighting), `none` is not supposed to be the same as `fg=default`. 2. It also seems incorrect to me that `none` effectively does nothing at all. According to the documentation, `none` should mean that > No highlighting is applied to the given context. Instead, `none` just appears to do nothing at all, which seems useless to me; if I don't want to change the highlighting of that part of the line, then I can just not add a spec for it. Do you agree and could someone be so kind as to fix this? :) Cheers, Marlon [-- Attachment #2: Type: text/html, Size: 2698 bytes --]
[-- Attachment #1: Type: text/plain, Size: 807 bytes --] On Mon, Oct 12, 2020 at 11:23 AM Marlon Richert <marlon.richert@gmail.com> wrote: > > 1. It seems incorrect to convert `fg=default` to `none`. From reading the [documentation](http://zsh.sourceforge.net/Doc/Release/Zsh-Line-Editor.html#Character-Highlighting), `none` is not supposed to be the same as `fg=default`. I took a quick look at this yesterday. When you print region_highlight and "fg=default" comes out as "none", that's a bug in the printing logic (function output_highlight in Src/prompt.c). It's fairly easy to fix -- patch below (only for fg; bg should be handled similarly). Naturally, it doesn't solve the other issues you've described. I haven't looked at why the highlight doesn't get applied as it should be. Roman. P.S. zsh-workers is the correct mailing list for this discussion. [-- Attachment #2: output-highlight-patch.txt --] [-- Type: text/plain, Size: 411 bytes --] diff --git a/Src/prompt.c b/Src/prompt.c index 997327e18..36c828321 100644 --- a/Src/prompt.c +++ b/Src/prompt.c @@ -1842,6 +1842,13 @@ output_highlight(zattr atr, char *buf) atrlen += len; if (buf) ptr += len; + } else if (atr & TXTNOFGCOLOUR) { + len = 10; + atrlen += len; + if (buf) { + strcpy(ptr, "fg=default"); + ptr += len; + } } if (atr & TXTBGCOLOUR) { if (atrlen) {
[-- Attachment #1: Type: text/plain, Size: 296 bytes --] On Tue, 13 Oct 2020 at 14:05, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote: > zsh-workers is the correct mailing list for this discussion. > Thanks. I wasn't sure if my first email had gone through, because I couldn't find it in the archives. But I'll send another message about that. [-- Attachment #2: Type: text/html, Size: 601 bytes --]
Roman Perepelitsa wrote on Tue, 13 Oct 2020 13:05 +0200: > On Mon, Oct 12, 2020 at 11:23 AM Marlon Richert > <marlon.richert@gmail.com> wrote: > > > > 1. It seems incorrect to convert `fg=default` to `none`. From reading the [documentation](http://zsh.sourceforge.net/Doc/Release/Zsh-Line-Editor.html#Character-Highlighting), `none` is not supposed to be the same as `fg=default`. > > I took a quick look at this yesterday. When you print region_highlight > and "fg=default" comes out as "none", that's a bug in the printing > logic (function output_highlight in Src/prompt.c). It's fairly easy to > fix -- patch below (only for fg; bg should be handled similarly). > Naturally, it doesn't solve the other issues you've described. I > haven't looked at why the highlight doesn't get applied as it should > be. z-sy-h does «region_highlight_copy=("${region_highlight[@]}"); … region_highlight=("${region_highlight_copy[@]}");» around invoking a highlighter, so the serialization bug could explain the observed ZLE behaviour, couldn't it? > Roman. > > P.S. > > zsh-workers is the correct mailing list for this discussion.
[-- Attachment #1: Type: text/plain, Size: 4591 bytes --] On Wed, Oct 14, 2020 at 10:46 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > > z-sy-h does «region_highlight_copy=("${region_highlight[@]}"); … > region_highlight=("${region_highlight_copy[@]}");» around invoking > a highlighter, so the serialization bug could explain the observed ZLE > behaviour, couldn't it? I haven't tried reproducing the problem with z-sy-h. Instead, I've used the following code from `zsh -f`: zle-line-pre-redraw() { region_highlight=('0 5 fg=1' '1 2 fg=default' '3 4 none') } zle -N zle-line-pre-redraw If you type "12345", odd and only odd numbers are supposed to be red. The actual behavior is that all numbers are red. I now took a look at how highlighting is applied (function zrefresh in Src/Zle/zle_refresh.c), then read the docs again, and I think there is a problem. If the same character is affected by two highlight specifications, how should it be highlighted? For example, if the first spec sets fg=1 and the second sets bg=2, how should the character be highlighted? What about fg=1 plus underline? Or underline plus fg=1? I could imagine two simple merging strategies: 1. All attributes are merged, so fg=1 + bg=2 + underline would result in underlined red text on green background. 2. The second highlight completely overrides the first. fg=1 + bg=2 + underline results in underlined text with the default color and no background. The meaning of "none" naturally follows from the choice of merging strategy. In the first case region highlight with "none" spec has no effect (X + none => X). In the second case such a region is displayed without any highlighting (X + none => no highlighting). The actual code does something else. If a spec has fg or bg with any value other than "default", then the spec completely overrides the previous spec. Otherwise the spec is merged with the previous spec with one exception: fg=default and bg=default have no effect (fg=1 + fg=default,underline => fg=1,underline). Note: "special" highlight is merged with a different algorithm. All other highlights, including "region", "isearch" and "paste", are merged the same way as the elements of region_highlights. A few examples of what the current code does: - fg=1 + bg=2 => bg=2 * the second spec completely overrides the first - fg=1 + underline => fg=1,underline * specs are merged - underline + fg=1 => fg=1 * the second spec completely overrides the first - fg=1 + none => fg=1 * specs are merged - fg=1 + fg=default,underline => fg=1,underline * specs are merged except that fg=default has no effect This doesn't look ideal. So, how can we fix it? "The second highlight completely overrides the first" will change the meaning of "none" from "ignore this spec completely" to "display text with no highlighting". For example, if you set zle_highlight=(special:none), special characters will have no highlighting whatsoever, while currently they would get highlighted by region_highlight. "All attributes are merged" makes it impossible to disable underline, bold, etc. We have fg=default and bg=default to disable colors but there is no equivalent syntax for disabling underline. I think there is one change to the current algorithm that would be an improvement and is relatively safe to do -- make fg=default and bg=default work similarly to fg=1 and bg=1 as far as merging goes. Since fg=1 in a spec causes a complete override (disabling underline, bold, etc.), fg=default should also do that. Patch attached (only for zrefresh; singlerefresh should be updated similarly). With this patch, if a spec has fg or bg, then it completely overrides the previous spec; otherwise the spec is merged with the previous spec. From the examples above, only the following works differently with the provided patch: fg=1 + fg=default,underline used to produce fg=1,underline but now it gives just underline without foreground color. Going forward, we can extend the spec syntax to give users more flexibility. I can see two extensions. 1. In addition to "underline", one can use "underline=on" and "underline=off". 2. If the first character of the spec is "+", it's merged with the current spec for the region; if the character is "=", it overrides; otherwise the current behavior is preserved (specs with fg and bg override, other specs merge). Thoughts? Roman. P.S. singlerefresh() doesn't use default_atr_on and thus zle_highlight=(default:fg=1) has no effect if SINGLE_LINE_ZLE is set. Is this intended? [-- Attachment #2: highlight-patch.txt --] [-- Type: text/plain, Size: 1500 bytes --] diff --git a/Src/Zle/zle_refresh.c b/Src/Zle/zle_refresh.c index d9d9503e2..1b246191f 100644 --- a/Src/Zle/zle_refresh.c +++ b/Src/Zle/zle_refresh.c @@ -1278,27 +1278,22 @@ zrefresh(void) offset = predisplaylen; /* increment over it */ if (rhp->start + offset <= tmppos && tmppos < rhp->end + offset) { - if (rhp->atr & (TXTFGCOLOUR|TXTBGCOLOUR)) { - /* override colour with later entry */ - base_atr_on = (base_atr_on & ~TXT_ATTR_ON_VALUES_MASK) | - rhp->atr; - } else { - /* no colour set yet */ + if (rhp->atr & (TXTFGCOLOUR|TXTNOFGCOLOUR|TXTBGCOLOUR|TXTNOBGCOLOUR)) + base_atr_on = rhp->atr; + else base_atr_on |= rhp->atr; - } if (tmppos == rhp->end + offset - 1 || tmppos == tmpll - 1) base_atr_off |= TXT_ATTR_OFF_FROM_ON(rhp->atr); } } - if (special_atr_on & (TXTFGCOLOUR|TXTBGCOLOUR)) { - /* keep colours from special attributes */ - all_atr_on = special_atr_on | - (base_atr_on & ~TXT_ATTR_COLOUR_ON_MASK); - } else { - /* keep colours from standard attributes */ - all_atr_on = special_atr_on | base_atr_on; - } + + all_atr_on = base_atr_on; + if (special_atr_on & (TXTFGCOLOUR|TXTNOFGCOLOUR)) + all_atr_on &= ~(TXT_ATTR_FG_ON_MASK|TXTNOFGCOLOUR); + if (special_atr_on & (TXTBGCOLOUR|TXTNOBGCOLOUR)) + all_atr_on &= ~(TXT_ATTR_BG_ON_MASK|TXTNOBGCOLOUR); + all_atr_on |= special_atr_on; all_atr_off = TXT_ATTR_OFF_FROM_ON(all_atr_on); if (t == scs) /* if cursor is here, remember it */
> On 15. Oct 2020, at 10.37, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote:
>
> Going forward, we can extend the spec syntax to give users more
> flexibility. I can see two extensions.
>
> 1. In addition to "underline", one can use "underline=on" and "underline=off".
> 2. If the first character of the spec is "+", it's merged with the
> current spec for the region; if the character is "=", it overrides;
> otherwise the current behavior is preserved (specs with fg and bg
> override, other specs merge).
>
> Thoughts?
If you’re going to extend the spec syntax, could you consider adding more ANSI attributes, such as `faint` and `conceal`?
On Thu, Oct 15, 2020 at 6:58 PM Marlon Richert <marlon.richert@gmail.com> wrote:
>
>
> > On 15. Oct 2020, at 10.37, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote:
> >
> > Going forward, we can extend the spec syntax to give users more
> > flexibility. I can see two extensions.
> >
> > 1. In addition to "underline", one can use "underline=on" and "underline=off".
> > 2. If the first character of the spec is "+", it's merged with the
> > current spec for the region; if the character is "=", it overrides;
> > otherwise the current behavior is preserved (specs with fg and bg
> > override, other specs merge).
> >
> > Thoughts?
>
> If you’re going to extend the spec syntax, could you consider adding more ANSI attributes, such as `faint` and `conceal`?
It's premature to say I'm going to extend the spec syntax. Just
thinking out loud, looking for feedback.
That said, extra ANSI attributes would be useful. My first choice
would be italic. The biggest problem with adding it is that zattr
doesn't have free bits left, so adding just one extra one/off
attribute would require changing the type of zattr from uint64_t to
some kind of struct, which would in turn require sweeping code changed
because zattr is used in so many places as an argument to bitwise
operators. I've spent a full day on this a few months back and
eventually stashed the change.
Another obstacle to supporting more ANSI attributes is the shortage of
upper- and lowercase letters that can be used in prompt expansions
(similar to %B/%b and %U/%u for bold and underlined). It would be a
shame if it was possible to use italic/faint/conceal in
region_highlight but not in prompt.
Note that the spec extension I proposed (or rather mused about) above
isn't affected by these challenges.
Roman.
Roman Perepelitsa wrote on Thu, 15 Oct 2020 07:37 +00:00: > I could imagine two simple merging strategies: > > 1. All attributes are merged, so fg=1 + bg=2 + underline would result > in underlined red text on green background. > 2. The second highlight completely overrides the first. fg=1 + bg=2 + > underline results in underlined text with the default color and no > background. > > The meaning of "none" naturally follows from the choice of merging > strategy. In the first case region highlight with "none" spec has no > effect (X + none => X). In the second case such a region is displayed > without any highlighting (X + none => no highlighting). > > The actual code does something else. If a spec has fg or bg with any > value other than "default", then the spec completely overrides the > previous spec. Otherwise the spec is merged with the previous spec > with one exception: fg=default and bg=default have no effect (fg=1 + > fg=default,underline => fg=1,underline). > > Note: "special" highlight is merged with a different algorithm. All > other highlights, including "region", "isearch" and "paste", are > merged the same way as the elements of region_highlights. > > A few examples of what the current code does: > > - fg=1 + bg=2 => bg=2 > * the second spec completely overrides the first > - fg=1 + underline => fg=1,underline > * specs are merged > - underline + fg=1 => fg=1 > * the second spec completely overrides the first > - fg=1 + none => fg=1 > * specs are merged > - fg=1 + fg=default,underline => fg=1,underline > * specs are merged except that fg=default has no effect > > This doesn't look ideal. So, how can we fix it? ⋮ > Thoughts? The inconsistent augmentation semantics have bugged me since I first ran into them. I'd love to see them rationalized. Short on time, but in general terms: 1. Heed PEP 20. (E.g., the points about explicitness and ambiguity) 2. Proposed acceptance test: When two specs are added, A+B, for any desired visual outcome there should be a value of B that, _regardless of the value of A_, achieves that outcome. Not sure how to get from the current semantics to acceptable ones. Maybe we could do, say: - fg=1 + bg=2,fg=inherit ⇒ fg=1,bg=2 - underline + fg=1,underline=inherit ⇒ fg=1,underline - none + fg=1,underline=inherit ⇒ fg=1 [no underline] As to «fg=1 + fg=default ⇒ fg=1», I think we can treat it as a bug and break compatibility?
Roman Perepelitsa wrote on Thu, 15 Oct 2020 17:09 +00:00:
> Another obstacle to supporting more ANSI attributes is the shortage of
> upper- and lowercase letters that can be used in prompt expansions
> (similar to %B/%b and %U/%u for bold and underlined). It would be a
> shame if it was possible to use italic/faint/conceal in
> region_highlight but not in prompt.
They could be added as pseudocolours in the %F{foo} syntax, if nothing else.
On Fri, Oct 16, 2020 at 6:29 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> The inconsistent augmentation semantics have bugged me since I first ran
> into them. I'd love to see them rationalized.
As I recall one of the problems with this is (as Roman mentioned)
there is no way to selectively turn off many of the attributes. That
is, there's no way to tell the terminal "revert to the previous value
of X", instead we have to keep track of that and explicitly assert
"set X to Y". In some cases even that's not enough, and the only
right approach is to clear all attributes and then reassert all
attributes. This shortcoming is directly reflected in the selection
of names/values in Functions/Misc/colors, where I also summarized the
ECMA standard in comments. (There's probably a newer standard at this
point.)
[-- Attachment #1: Type: text/plain, Size: 1251 bytes --] While we're on the topic of `region_highlight`, here's another problem I've noticed with it: `standout` (3) is rendered as `reverse` (7), even though my terminal is perfectly capable of rendering italic. When I use `\e[3m` in `$PS1` or `$ZLS_COLORS`, it renders fine as italic. There's no reason to convert it. On Fri, 16 Oct 2020 at 18:51, Bart Schaefer <schaefer@brasslantern.com> wrote: > On Fri, Oct 16, 2020 at 6:29 AM Daniel Shahaf <d.s@daniel.shahaf.name> > wrote: > > > > The inconsistent augmentation semantics have bugged me since I first ran > > into them. I'd love to see them rationalized. > > As I recall one of the problems with this is (as Roman mentioned) > there is no way to selectively turn off many of the attributes. That > is, there's no way to tell the terminal "revert to the previous value > of X", instead we have to keep track of that and explicitly assert > "set X to Y". In some cases even that's not enough, and the only > right approach is to clear all attributes and then reassert all > attributes. This shortcoming is directly reflected in the selection > of names/values in Functions/Misc/colors, where I also summarized the > ECMA standard in comments. (There's probably a newer standard at this > point.) > [-- Attachment #2: Type: text/html, Size: 1695 bytes --]
Marlon Richert wrote on Thu, 22 Oct 2020 22:58 +0300: > While we're on the topic of `region_highlight`, here's another problem I've > noticed with it: `standout` (3) is rendered as `reverse` (7), even though > my terminal is perfectly capable of rendering italic. When I use `\e[3m` in > `$PS1` or `$ZLS_COLORS`, it renders fine as italic. There's no reason to > convert it. You haven't explained why you think it's zle that does the conversion. For starters, what's the output of `tput smso | xxd` and `tput rev | xxd`? Here they print the same value, even though \e[3m; does italicize. (Plain xterm on Debian.) I'm reminded of https://github.com/tmux/tmux/wiki/FAQ#i-dont-see-italics-or-italics-and-reverse-are-the-wrong-way-round but it may or may not be related.
On Thu, Oct 22, 2020 at 9:59 PM Marlon Richert <marlon.richert@gmail.com> wrote: > While we're on the topic of `region_highlight`, here's another > problem I've noticed with it: `standout` (3) is rendered as > `reverse` (7), even though my terminal is perfectly capable of > rendering italic. When I use `\e[3m` in `$PS1` or `$ZLS_COLORS`, it > renders fine as italic. There's no reason to convert it. Previous discussion about standout, italized and negative image: https://www.zsh.org/mla/workers/2019/msg01189.html. Roman.
[-- Attachment #1: Type: text/plain, Size: 2114 bytes --] On Fri, 23 Oct 2020 at 02:28, Daniel Shahaf <d.s@daniel.shahaf.name> wrote: > You haven't explained why you think it's zle that does the conversion. > For starters, what's the output of `tput smso | xxd` and `tput rev | > xxd`? Here they print the same value, even though \e[3m; does > italicize. (Plain xterm on Debian.) > Ah, my bad. I assumed `region_highlight` handled visual formatting similarly to how `complist` and prompt expansion handle them, and assumed that the exact same names used in the `colors` function meant the same thing. I'm sure many other Zsh end users will assume the same. It would be great if all four of those could be made consistent with each other. On Fri, 23 Oct 2020 at 11:08, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote: > Previous discussion about standout, italized and negative image: > https://www.zsh.org/mla/workers/2019/msg01189.html. Thanks for the link. Did that discussion ever result in a patch? It looks like you never got an answer to [your question]( https://www.zsh.org/mla/workers/2019/msg01191.html): > Shall I send a patch? I'd love to have this feature. I'd love to have `region_highlight` just accept and pass through unmodified all ECMA-48 SGR parameter values. (See http://www.ecma-international.org/publications/files/ECMA-ST/Ecma-048.pdf, page 61.) It is a standard, after all. I feel like routing this through `ncurses` is not only unnecessary (since we have a standard for this); it's also problematic. For many terminals, there is a significant lag between their last release and the last time their `terminfo` entry in `ncurses` got updated. (The entry for macOS's Terminal.app, for example, is already 3 years old and you get better results from `ncurses` by letting Terminal.app declare itself as being `xterm-256color`, rather than relying on the `nsterm` entry.) Rather than using `ncurses` for this, I'd rather see the principle of ["graceful degradation"](https://en.wikipedia.org/wiki/Fault_tolerance) be applied here and let the terminal itself handle (that is, ignore) those SGR parameter values that it does not support. [-- Attachment #2: Type: text/html, Size: 3262 bytes --]
On Fri, Oct 23, 2020 at 11:25 AM Marlon Richert <marlon.richert@gmail.com> wrote: > > Thanks for the link. Did that discussion ever result in a patch? Nope. I've mentioned it earlier on *this* thread. > It looks like you never got an answer to > [your question](https://www.zsh.org/mla/workers/2019/msg01191.html): Here's the follow up: https://www.zsh.org/mla/workers/2020/msg00042.html. Apparently, threading doesn't work across year boundaries. > I feel like routing this through `ncurses` is not only unnecessary > (since we have a standard for this); it's also problematic. The only way to know whether the terminal supports some capability (e.g., moving the cursor to the specified location on the screen, or highlighting text with 256 colors) and to use the said capability is through terminfo. There is no way around it. It also doesn't seem like ncurses causes any problems that are related to this thread. It's possible to support italized text output in zsh without ditching ncurses. Please see the discussion I've linked above. Roman.
[-- Attachment #1: Type: text/plain, Size: 1157 bytes --] On Fri, 23 Oct 2020 at 12:35, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote: > The only way to know whether the terminal supports some capability > (e.g., moving the cursor to the specified location on the screen, or > highlighting text with 256 colors) and to use the said capability is > through terminfo. There is no way around it. > Sure, but that is not necessary for this particular problem. If the user wants to pass something to their own terminal, then just let them. Zsh already lets the user pass in ECMA-48 SGR parameter values directly in both `complist` and prompt expansions. Why not in `region_highlight`? It would be different if there was no standard for this, but there is. I don't see any benefit here to adding two conversation layers (one being Zsh's own `region_highlight` syntax & terminology, the other being `ncurses`) in between. It unnecessarily limits the user's options. Anyway, what I'm trying to say is that it would be really nice to be able to use the exact same values for `ls`, `region_highlight`, `complist` and prompt expansions, instead of having to do ambiguous and/or incomplete conversions. That is all. [-- Attachment #2: Type: text/html, Size: 1515 bytes --]
On Fri, Oct 23, 2020 at 12:40 PM Marlon Richert <marlon.richert@gmail.com> wrote: > > On Fri, 23 Oct 2020 at 12:35, Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote: >> >> The only way to know whether the terminal supports some capability >> (e.g., moving the cursor to the specified location on the screen, or >> highlighting text with 256 colors) and to use the said capability is >> through terminfo. There is no way around it. > > Sure, but that is not necessary for this particular problem. Which problem? As I mentioned, it's definitely possible to add support for italicized text to zsh and to make it consistent with the existing highlighting API. The fact that region_highlight and zle_highlight support "standout" is not a problem but a feature. Standout is a part of the curses standard and zsh interprets it within region_highlight and zle_highlight in a manner consistent with the standard. ("standout" within "colors" is a different matter. That one is incorrect in my opinion.) > If the user wants to pass something to their own terminal, then > just let them. Zsh already lets the user pass in ECMA-48 SGR > parameter values directly in [...] prompt expansions. Do you mean %{...%}? It's indeed useful and allows the user to insert any byte sequence into their prompt. I don't think there is anything specific to ECMA-48 SGR in prompt expansions, or am I missing it? region_highlight doesn't need the equivalent of %{...%} because it's implied. So something like this could work: region_highlight=($'1 2 raw=\e[4m') The value of `raw=` spans until the end of the string, so if you want to combine it with `fg=42`, you'll need to use `fg=42,raw=\e[4m` rather than `raw=\e[4m,fg=42`. That's one way to do it -- I'm not particularly attached to it, just thinking out loud. One tricky part about this is that it doesn't allow you to specify the sequence that undoes the effect of `raw` but this is probably OK because zle inserts SGR0 before every that may have different highlighting. There is something in the code that suggests this isn't always the case but I don't know where this code actually triggers. From the implementation's point of view this will require quite a bit of work because this doesn't fit the existing framework around zattr. Roman.
Roman Perepelitsa wrote on Fri, 23 Oct 2020 11:35 +0200:
> On Fri, Oct 23, 2020 at 11:25 AM Marlon Richert
> <marlon.richert@gmail.com> wrote:
> > It looks like you never got an answer to
> > [your question](https://www.zsh.org/mla/workers/2019/msg01191.html):
>
> Here's the follow up:
> https://www.zsh.org/mla/workers/2020/msg00042.html. Apparently,
> threading doesn't work across year boundaries.
That's correct. Each /mla/workers/{1995..2020}/ directory is
a separate mhonarc repository («'| { cd $(date +%y) &&
mhonarc -add; }'», basically), so inter-year threading won't happen.
I don't actually know a reason we couldn't have a single,
monolithic archive, though.
Cheers,
Daniel
On Fri, Oct 23, 2020 at 4:38 AM Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote: > > The fact that region_highlight and zle_highlight support "standout" is > not a problem but a feature. Standout is a part of the curses standard > and zsh interprets it within region_highlight and zle_highlight in a > manner consistent with the standard. ("standout" within "colors" is a > different matter. That one is incorrect in my opinion.) You're correct that "standout" doesn't match the terminology in ECMA-48. The use of "standout" was chosen to follow the terminology in prompt strings, I believe. This matched the behavior of most terminals at the time (notice ${(k)color[(r)standout]} is actually from Sven all the way back in 2001). Also, there's no way to directly reference an escape sequence for any of the "Attribute Codes" (they aren't in the $fg / $bg hashes), so those values are mostly there for commentary value. I would have no objection to adding "italic"/"no-italic" to the color hash. It also appears standout would more accurately be tied to reverse-video now, but because the keys of the hash are the numeric codes we can only have one or the other. diff --git a/Functions/Misc/colors b/Functions/Misc/colors index 027ca9a14..b221e6688 100644 --- a/Functions/Misc/colors +++ b/Functions/Misc/colors @@ -14,11 +14,12 @@ color=( 00 none # 20 gothic 01 bold # 21 double-underline 02 faint 22 normal - 03 standout 23 no-standout + 03 italic 23 no-italic # no-gothic 04 underline 24 no-underline 05 blink 25 no-blink # 06 fast-blink # 26 proportional 07 reverse 27 no-reverse +# 07 standout 27 no-standout 08 conceal 28 no-conceal # 09 strikethrough # 29 no-strikethrough
On 15 Oct, Roman Perepelitsa wrote: > It's premature to say I'm going to extend the spec syntax. Just > thinking out loud, looking for feedback. > > That said, extra ANSI attributes would be useful. My first choice > would be italic. The biggest problem with adding it is that zattr > doesn't have free bits left, so adding just one extra one/off > attribute would require changing the type of zattr from uint64_t to > some kind of struct, which would in turn require sweeping code changed > because zattr is used in so many places as an argument to bitwise > operators. I've spent a full day on this a few months back and > eventually stashed the change. Such changes can be tricky and it might be better to look for ways to eke out a couple more bits from a 64-bit zattr. We do use a big array of them for storing display contents so making zattr bigger isn't entirely ideal. Three possibilities come to mind :- (1) TXT_ATTR_{B,F}G_TERMCAP which is holding meta information rather than text attributes. (2) Having both OFF and ON bits for attributes. One bit ought to suffice - text is either in bold or it isn't. (3) You could put TXTFGCOLOUR/TXTBGCOLOUR in a high bit of TXT_ATTR_FG_COL_MASK when TXT_ATTR_FG_24BIT is not set. Regarding (1), I just noticed that a new D01prompt test case is failing on FreeBSD because ${(%):-%F{2}} is not producing the same as ${(%):-%F{green}}. This is a new test added just last month in 47352. This is down to TXT_ATTR_FG_TERMCAP being set for 2 but not for green. I'm inclined to think that when we parse a colour is not the right place to decide whether we want to use termcap or not. Leave that for the code which generates the escape sequences. The decision to not trust termcap for named colours comes from 24957. I've no idea what the rationale for that might be. The use of termcap or otherwise has no bearing on whether it'll really be green. I doubt there'd be much harm in dropping those two flags, or does anyone see an issue? Or remember historical reasoning? This would be the easiest way to get two bits for italics. I just did a trial of code changes for (2). The existing code does set, e.g. both TXTFGCOLOUR and TXTNOFGCOLOUR on particular elements. The turning off of attributes seemingly comes after the character. So my trial is turning attributes off one position early but otherwise breaks surprisingly little given the quite significant chunks of code that get ripped out. Not having anywhere to attach final attribute-off actions may mean this is unworkable. Does anyone more familiar with the code have any ideas on what this might break? Being able to leave attributes on at the end of the prompt comes to mind but it may also affect some aspect of manipulating the zle buffer. I expect the original reason for having both OFF and ON flags was that the earliest code using this was the parsing of prompt %-escapes for which the distinction between off, on and unchanged is needed. So an additional mask for changes would have added complexity. But for the zle display code, changing it might make the code simpler. One little xor in settextattributes() is able to derive changes since the last attributes. But it may prove far from trivial to hammer out whatever tricky incompatibilities it does throw up. I may persevere with this somewhat. Partly because I see potental for the refactoring to simplify the code significantly. But I'd be interested in any thoughts or insight anyone might have. (3) would require some tweaks to the bit twiddling. Note that TXTFGCOLOUR and TXTBGCOLOUR are needed, in effect to represent [not] the default colour. I'd only do this after trying the first two but it does free an extra two bits reasonably easily. > Another obstacle to supporting more ANSI attributes is the shortage of > upper- and lowercase letters that can be used in prompt expansions > (similar to %B/%b and %U/%u for bold and underlined). It would be a > shame if it was possible to use italic/faint/conceal in > region_highlight but not in prompt. If it comes to it, %A{faint}. I'm sure this was discussed once before. Oliver
On Sat, Oct 24, 2020 at 3:34 AM Oliver Kiddle <opk@zsh.org> wrote:
> I expect the original reason for having both OFF and ON flags was that
> the earliest code using this was the parsing of prompt %-escapes for
> which the distinction between off, on and unchanged is needed.
Yes, this must be the reason.
On/off/unchanged would be useful within {zle,region}_highlight as
well. This was touched on earlier in this thread. Right now it's
possible to request a region to be underlined while keeping all other
attributes (color, boldness, etc.) unchanged. It's not, however,
possible to *disable* underline while keeping everything else
unchanged. With prompt extensions this is available via '%u'.
Roman.
[-- Attachment #1: Type: text/plain, Size: 1202 bytes --] On Sat, 24 Oct 2020 at 03:50, Bart Schaefer <schaefer@brasslantern.com> wrote: > You're correct that "standout" doesn't match the terminology in > ECMA-48. The use of "standout" was chosen to follow the terminology > in prompt strings, I believe. > > This matched the behavior of most terminals at the time (notice > ${(k)color[(r)standout]} is actually from Sven all the way back in > 2001). Also, there's no way to directly reference an escape sequence > for any of the "Attribute Codes" (they aren't in the $fg / $bg > hashes), so those values are mostly there for commentary value. > > I would have no objection to adding "italic"/"no-italic" to the color > hash. It also appears standout would more accurately be tied to > reverse-video now, but because the keys of the hash are the numeric > codes we can only have one or the other. > Would it be possible to drop `standout` (as it's rather ambiguous) and instead add both `italic` and `reverse`? They both have `terminfo` entries. It would be great if Zsh could eventually support the same highlighting keywords as [Git does]( https://www.git-scm.com/docs/git-config#Documentation/git-config.txt-color), but I guess that's a long way off. [-- Attachment #2: Type: text/html, Size: 1725 bytes --]
[-- Attachment #1: Type: text/plain, Size: 600 bytes --] On Tue, Nov 3, 2020 at 10:54 AM Marlon Richert <marlon.richert@gmail.com> wrote: > > Would it be possible to drop `standout` (as it's rather ambiguous) and > instead add both `italic` and `reverse`? They both have `terminfo` entries. > If you're referring to Functions/Misc/colors, that's what my proposed patch already does. If you're referring to prompts, "standout" appears only in the documentation, but the prompt sequences are %S (on) and %s (off) to be suggestive thereof. %R and %I are already in use for other features; we'd need some kind of extended syntax (%F{name} is not nestable). [-- Attachment #2: Type: text/html, Size: 1005 bytes --]
[-- Attachment #1: Type: text/plain, Size: 1071 bytes --] > On 4. Nov 2020, at 1.45, Bart Schaefer <schaefer@brasslantern.com> wrote: > > >> On Tue, Nov 3, 2020 at 10:54 AM Marlon Richert <marlon.richert@gmail.com> wrote: > >> >> Would it be possible to drop `standout` (as it's rather ambiguous) and instead add both `italic` and `reverse`? They both have `terminfo` entries. > > If you're referring to Functions/Misc/colors, that's what my proposed patch already does. > > If you're referring to prompts, "standout" appears only in the documentation, but the prompt sequences are %S (on) and %s (off) to be suggestive thereof. %R and %I are already in use for other features; we'd need some kind of extended syntax (%F{name} is not nestable). Actually, I was talking about `region_highlight`. It is currently very restrictive in what kind of formatting it supports. Perhaps I replied to the wrong email. Sorry about the confusion. As for prompts, since they already support using raw escape code, it’s not really an issue there. `region_highlight`, however, does not offer any workarounds. [-- Attachment #2: Type: text/html, Size: 1764 bytes --]
[-- Attachment #1: Type: text/plain, Size: 890 bytes --] Can someone please review Roman's patch (attached)? On Tue, Oct 13, 2020 at 2:05 PM Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote: > On Mon, Oct 12, 2020 at 11:23 AM Marlon Richert > <marlon.richert@gmail.com> wrote: > > 1. It seems incorrect to convert `fg=default` to `none`. From reading the [documentation](http://zsh.sourceforge.net/Doc/Release/Zsh-Line-Editor.html#Character-Highlighting), `none` is not supposed to be the same as `fg=default`. > > I took a quick look at this yesterday. When you print region_highlight > and "fg=default" comes out as "none", that's a bug in the printing > logic (function output_highlight in Src/prompt.c). It's fairly easy to > fix -- patch below (only for fg; bg should be handled similarly). > Naturally, it doesn't solve the other issues you've described. I > haven't looked at why the highlight doesn't get applied as it should > be. [-- Attachment #2: output-highlight-patch.txt --] [-- Type: text/plain, Size: 411 bytes --] diff --git a/Src/prompt.c b/Src/prompt.c index 997327e18..36c828321 100644 --- a/Src/prompt.c +++ b/Src/prompt.c @@ -1842,6 +1842,13 @@ output_highlight(zattr atr, char *buf) atrlen += len; if (buf) ptr += len; + } else if (atr & TXTNOFGCOLOUR) { + len = 10; + atrlen += len; + if (buf) { + strcpy(ptr, "fg=default"); + ptr += len; + } } if (atr & TXTBGCOLOUR) { if (atrlen) {
[-- Attachment #1: Type: text/plain, Size: 4045 bytes --] And here's another patch (attached) of Roman's that still needs review. On Thu, Oct 15, 2020 at 10:37 AM Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote: > I haven't tried reproducing the problem with z-sy-h. Instead, I've > used the following code from `zsh -f`: > > zle-line-pre-redraw() { > region_highlight=('0 5 fg=1' '1 2 fg=default' '3 4 none') > } > zle -N zle-line-pre-redraw > > If you type "12345", odd and only odd numbers are supposed to be red. > The actual behavior is that all numbers are red. > > I now took a look at how highlighting is applied (function zrefresh in > Src/Zle/zle_refresh.c), then read the docs again, and I think there is > a problem. If the same character is affected by two highlight > specifications, how should it be highlighted? For example, if the > first spec sets fg=1 and the second sets bg=2, how should the > character be highlighted? What about fg=1 plus underline? Or underline > plus fg=1? > > I could imagine two simple merging strategies: > > 1. All attributes are merged, so fg=1 + bg=2 + underline would result > in underlined red text on green background. > 2. The second highlight completely overrides the first. fg=1 + bg=2 + > underline results in underlined text with the default color and no > background. > > The meaning of "none" naturally follows from the choice of merging > strategy. In the first case region highlight with "none" spec has no > effect (X + none => X). In the second case such a region is displayed > without any highlighting (X + none => no highlighting). > > The actual code does something else. If a spec has fg or bg with any > value other than "default", then the spec completely overrides the > previous spec. Otherwise the spec is merged with the previous spec > with one exception: fg=default and bg=default have no effect (fg=1 + > fg=default,underline => fg=1,underline). > > Note: "special" highlight is merged with a different algorithm. All > other highlights, including "region", "isearch" and "paste", are > merged the same way as the elements of region_highlights. > > A few examples of what the current code does: > > - fg=1 + bg=2 => bg=2 > * the second spec completely overrides the first > - fg=1 + underline => fg=1,underline > * specs are merged > - underline + fg=1 => fg=1 > * the second spec completely overrides the first > - fg=1 + none => fg=1 > * specs are merged > - fg=1 + fg=default,underline => fg=1,underline > * specs are merged except that fg=default has no effect > > This doesn't look ideal. So, how can we fix it? > > "The second highlight completely overrides the first" will change the > meaning of "none" from "ignore this spec completely" to "display text > with no highlighting". For example, if you set > zle_highlight=(special:none), special characters will have no > highlighting whatsoever, while currently they would get highlighted by > region_highlight. > > "All attributes are merged" makes it impossible to disable underline, > bold, etc. We have fg=default and bg=default to disable colors but > there is no equivalent syntax for disabling underline. > > I think there is one change to the current algorithm that would be an > improvement and is relatively safe to do -- make fg=default and > bg=default work similarly to fg=1 and bg=1 as far as merging goes. > Since fg=1 in a spec causes a complete override (disabling underline, > bold, etc.), fg=default should also do that. Patch attached (only for > zrefresh; singlerefresh should be updated similarly). With this patch, > if a spec has fg or bg, then it completely overrides the previous > spec; otherwise the spec is merged with the previous spec. > > From the examples above, only the following works differently with the > provided patch: fg=1 + fg=default,underline used to produce > fg=1,underline but now it gives just underline without foreground > color. > > P.S. > singlerefresh() doesn't use default_atr_on and thus > zle_highlight=(default:fg=1) has no effect if SINGLE_LINE_ZLE is set. > Is this intended? [-- Attachment #2: highlight-patch.txt --] [-- Type: text/plain, Size: 1500 bytes --] diff --git a/Src/Zle/zle_refresh.c b/Src/Zle/zle_refresh.c index d9d9503e2..1b246191f 100644 --- a/Src/Zle/zle_refresh.c +++ b/Src/Zle/zle_refresh.c @@ -1278,27 +1278,22 @@ zrefresh(void) offset = predisplaylen; /* increment over it */ if (rhp->start + offset <= tmppos && tmppos < rhp->end + offset) { - if (rhp->atr & (TXTFGCOLOUR|TXTBGCOLOUR)) { - /* override colour with later entry */ - base_atr_on = (base_atr_on & ~TXT_ATTR_ON_VALUES_MASK) | - rhp->atr; - } else { - /* no colour set yet */ + if (rhp->atr & (TXTFGCOLOUR|TXTNOFGCOLOUR|TXTBGCOLOUR|TXTNOBGCOLOUR)) + base_atr_on = rhp->atr; + else base_atr_on |= rhp->atr; - } if (tmppos == rhp->end + offset - 1 || tmppos == tmpll - 1) base_atr_off |= TXT_ATTR_OFF_FROM_ON(rhp->atr); } } - if (special_atr_on & (TXTFGCOLOUR|TXTBGCOLOUR)) { - /* keep colours from special attributes */ - all_atr_on = special_atr_on | - (base_atr_on & ~TXT_ATTR_COLOUR_ON_MASK); - } else { - /* keep colours from standard attributes */ - all_atr_on = special_atr_on | base_atr_on; - } + + all_atr_on = base_atr_on; + if (special_atr_on & (TXTFGCOLOUR|TXTNOFGCOLOUR)) + all_atr_on &= ~(TXT_ATTR_FG_ON_MASK|TXTNOFGCOLOUR); + if (special_atr_on & (TXTBGCOLOUR|TXTNOBGCOLOUR)) + all_atr_on &= ~(TXT_ATTR_BG_ON_MASK|TXTNOBGCOLOUR); + all_atr_on |= special_atr_on; all_atr_off = TXT_ATTR_OFF_FROM_ON(all_atr_on); if (t == scs) /* if cursor is here, remember it */
On Wed, Mar 31, 2021, at 4:26 AM, Marlon Richert wrote:
> And here's another patch (attached) of Roman's that still needs review.
ping for 48343 and 48345
vq
Lawrence Velázquez wrote on Sat, Apr 10, 2021 at 16:33:29 -0400:
> On Wed, Mar 31, 2021, at 4:26 AM, Marlon Richert wrote:
> > And here's another patch (attached) of Roman's that still needs review.
>
> ping for 48343 and 48345
48343 is a re-post of 47446, by Roman, who has commit access.
Roman, is there any reason not to extend that patch to bg and push it?
The other issues discussed in the thread don't seem to block that patch?
If you haven't got tuits at the moment or are awaiting review, just say
so.
And thanks for all the analyses upthread (e.g., 47458), especially
regarding the merge logic!
Cheers,
Daniel
On Tue, Apr 13, 2021 at 5:20 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> 48343 is a re-post of 47446, by Roman, who has commit access.
>
> Roman, is there any reason not to extend that patch to bg and push it?
I don't remember now, sorry. I'm afraid I'd don't have time right now
to work on this. If someone could pick this up, I would be grateful.
Roman.
Roman Perepelitsa wrote on Tue, Apr 13, 2021 at 22:33:51 +0200:
> On Tue, Apr 13, 2021 at 5:20 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> >
> > 48343 is a re-post of 47446, by Roman, who has commit access.
> >
> > Roman, is there any reason not to extend that patch to bg and push it?
>
> I don't remember now, sorry. I'm afraid I'd don't have time right now
> to work on this. If someone could pick this up, I would be grateful.
Thanks, Roman. Anyone wants to pick up 47446, or at least write an
XFail test for it?
On Fri, Oct 23, 2020 at 5:50 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> I would have no objection to adding "italic"/"no-italic" to the color
> hash. It also appears standout would more accurately be tied to
> reverse-video now, but because the keys of the hash are the numeric
> codes we can only have one or the other.
Bumping myself on this patch from last October. Any objections?
On Wed, Apr 14, 2021, at 7:04 AM, Daniel Shahaf wrote:
> Roman Perepelitsa wrote on Tue, Apr 13, 2021 at 22:33:51 +0200:
> > On Tue, Apr 13, 2021 at 5:20 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > >
> > > 48343 is a re-post of 47446, by Roman, who has commit access.
> > >
> > > Roman, is there any reason not to extend that patch to bg and push it?
> >
> > I don't remember now, sorry. I'm afraid I'd don't have time right now
> > to work on this. If someone could pick this up, I would be grateful.
>
> Thanks, Roman. Anyone wants to pick up 47446, or at least write an
> XFail test for it?
Anyone for workers/47446 and workers/47458?
vq
On Sun, May 9, 2021, at 4:49 PM, Lawrence Velázquez wrote:
> On Wed, Apr 14, 2021, at 7:04 AM, Daniel Shahaf wrote:
> > Roman Perepelitsa wrote on Tue, Apr 13, 2021 at 22:33:51 +0200:
> > > On Tue, Apr 13, 2021 at 5:20 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > > >
> > > > 48343 is a re-post of 47446, by Roman, who has commit access.
> > > >
> > > > Roman, is there any reason not to extend that patch to bg and push it?
> > >
> > > I don't remember now, sorry. I'm afraid I'd don't have time right now
> > > to work on this. If someone could pick this up, I would be grateful.
> >
> > Thanks, Roman. Anyone wants to pick up 47446, or at least write an
> > XFail test for it?
>
> Anyone for workers/47446 and workers/47458?
last call for workers/47446 and workers/47458
--
vq