* region_highlight converts `fg=default` to `none`, which is not the same @ 2020-10-12 9:22 Marlon Richert 2020-10-13 11:05 ` Roman Perepelitsa 0 siblings, 1 reply; 33+ messages in thread From: Marlon Richert @ 2020-10-12 9:22 UTC (permalink / raw) To: Zsh hackers list [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-12 9:22 region_highlight converts `fg=default` to `none`, which is not the same Marlon Richert @ 2020-10-13 11:05 ` Roman Perepelitsa 2020-10-14 5:12 ` Marlon Richert ` (2 more replies) 0 siblings, 3 replies; 33+ messages in thread From: Roman Perepelitsa @ 2020-10-13 11:05 UTC (permalink / raw) To: Marlon Richert; +Cc: Zsh hackers list [-- 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) { ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-13 11:05 ` Roman Perepelitsa @ 2020-10-14 5:12 ` Marlon Richert 2020-10-14 20:46 ` Daniel Shahaf 2021-03-31 8:24 ` Marlon Richert 2 siblings, 0 replies; 33+ messages in thread From: Marlon Richert @ 2020-10-14 5:12 UTC (permalink / raw) To: Roman Perepelitsa; +Cc: Zsh hackers list [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-13 11:05 ` Roman Perepelitsa 2020-10-14 5:12 ` Marlon Richert @ 2020-10-14 20:46 ` Daniel Shahaf 2020-10-15 7:37 ` Roman Perepelitsa 2021-03-31 8:24 ` Marlon Richert 2 siblings, 1 reply; 33+ messages in thread From: Daniel Shahaf @ 2020-10-14 20:46 UTC (permalink / raw) To: Roman Perepelitsa; +Cc: Marlon Richert, Zsh hackers list 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-14 20:46 ` Daniel Shahaf @ 2020-10-15 7:37 ` Roman Perepelitsa 2020-10-15 16:58 ` Marlon Richert ` (2 more replies) 0 siblings, 3 replies; 33+ messages in thread From: Roman Perepelitsa @ 2020-10-15 7:37 UTC (permalink / raw) To: Daniel Shahaf; +Cc: Marlon Richert, Zsh hackers list [-- 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 */ ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-15 7:37 ` Roman Perepelitsa @ 2020-10-15 16:58 ` Marlon Richert 2020-10-15 17:09 ` Roman Perepelitsa 2020-10-16 13:28 ` Daniel Shahaf 2021-03-31 8:26 ` region_highlight converts `fg=default` to `none`, which is not the same Marlon Richert 2 siblings, 1 reply; 33+ messages in thread From: Marlon Richert @ 2020-10-15 16:58 UTC (permalink / raw) To: Roman Perepelitsa; +Cc: Daniel Shahaf, Zsh hackers list > 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`? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-15 16:58 ` Marlon Richert @ 2020-10-15 17:09 ` Roman Perepelitsa 2020-10-16 13:36 ` Daniel Shahaf 2020-10-24 1:34 ` Oliver Kiddle 0 siblings, 2 replies; 33+ messages in thread From: Roman Perepelitsa @ 2020-10-15 17:09 UTC (permalink / raw) To: Marlon Richert; +Cc: Daniel Shahaf, Zsh hackers list 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-15 17:09 ` Roman Perepelitsa @ 2020-10-16 13:36 ` Daniel Shahaf 2020-10-24 1:34 ` Oliver Kiddle 1 sibling, 0 replies; 33+ messages in thread From: Daniel Shahaf @ 2020-10-16 13:36 UTC (permalink / raw) To: zsh-workers 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-15 17:09 ` Roman Perepelitsa 2020-10-16 13:36 ` Daniel Shahaf @ 2020-10-24 1:34 ` Oliver Kiddle 2020-10-24 6:42 ` Roman Perepelitsa 1 sibling, 1 reply; 33+ messages in thread From: Oliver Kiddle @ 2020-10-24 1:34 UTC (permalink / raw) To: Roman Perepelitsa; +Cc: Marlon Richert, Zsh hackers list 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-24 1:34 ` Oliver Kiddle @ 2020-10-24 6:42 ` Roman Perepelitsa 0 siblings, 0 replies; 33+ messages in thread From: Roman Perepelitsa @ 2020-10-24 6:42 UTC (permalink / raw) To: Oliver Kiddle; +Cc: Marlon Richert, Zsh hackers list 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-15 7:37 ` Roman Perepelitsa 2020-10-15 16:58 ` Marlon Richert @ 2020-10-16 13:28 ` Daniel Shahaf 2020-10-16 15:50 ` Bart Schaefer 2021-03-31 8:26 ` region_highlight converts `fg=default` to `none`, which is not the same Marlon Richert 2 siblings, 1 reply; 33+ messages in thread From: Daniel Shahaf @ 2020-10-16 13:28 UTC (permalink / raw) To: Roman Perepelitsa; +Cc: Marlon Richert, Zsh hackers list 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? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-16 13:28 ` Daniel Shahaf @ 2020-10-16 15:50 ` Bart Schaefer 2020-10-22 19:58 ` Marlon Richert 0 siblings, 1 reply; 33+ messages in thread From: Bart Schaefer @ 2020-10-16 15:50 UTC (permalink / raw) To: Daniel Shahaf; +Cc: Roman Perepelitsa, Marlon Richert, Zsh hackers list 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.) ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-16 15:50 ` Bart Schaefer @ 2020-10-22 19:58 ` Marlon Richert 2020-10-22 23:28 ` Daniel Shahaf 2020-10-23 8:08 ` Roman Perepelitsa 0 siblings, 2 replies; 33+ messages in thread From: Marlon Richert @ 2020-10-22 19:58 UTC (permalink / raw) To: Bart Schaefer; +Cc: Daniel Shahaf, Roman Perepelitsa, Zsh hackers list [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-22 19:58 ` Marlon Richert @ 2020-10-22 23:28 ` Daniel Shahaf 2020-10-23 8:08 ` Roman Perepelitsa 1 sibling, 0 replies; 33+ messages in thread From: Daniel Shahaf @ 2020-10-22 23:28 UTC (permalink / raw) To: Marlon Richert; +Cc: Bart Schaefer, Roman Perepelitsa, Zsh hackers list 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-22 19:58 ` Marlon Richert 2020-10-22 23:28 ` Daniel Shahaf @ 2020-10-23 8:08 ` Roman Perepelitsa 2020-10-23 9:24 ` Marlon Richert 1 sibling, 1 reply; 33+ messages in thread From: Roman Perepelitsa @ 2020-10-23 8:08 UTC (permalink / raw) To: Marlon Richert; +Cc: Bart Schaefer, Daniel Shahaf, Zsh hackers list 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-23 8:08 ` Roman Perepelitsa @ 2020-10-23 9:24 ` Marlon Richert 2020-10-23 9:35 ` Roman Perepelitsa 0 siblings, 1 reply; 33+ messages in thread From: Marlon Richert @ 2020-10-23 9:24 UTC (permalink / raw) To: Roman Perepelitsa; +Cc: Bart Schaefer, Daniel Shahaf, Zsh hackers list [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-23 9:24 ` Marlon Richert @ 2020-10-23 9:35 ` Roman Perepelitsa 2020-10-23 10:40 ` Marlon Richert 2020-10-23 23:57 ` Threading across year boundaries (was: Re: region_highlight converts `fg=default` to `none`, which is not the same) Daniel Shahaf 0 siblings, 2 replies; 33+ messages in thread From: Roman Perepelitsa @ 2020-10-23 9:35 UTC (permalink / raw) To: Marlon Richert; +Cc: Bart Schaefer, Daniel Shahaf, Zsh hackers list 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-23 9:35 ` Roman Perepelitsa @ 2020-10-23 10:40 ` Marlon Richert 2020-10-23 11:38 ` Roman Perepelitsa 2020-10-23 23:57 ` Threading across year boundaries (was: Re: region_highlight converts `fg=default` to `none`, which is not the same) Daniel Shahaf 1 sibling, 1 reply; 33+ messages in thread From: Marlon Richert @ 2020-10-23 10:40 UTC (permalink / raw) To: Roman Perepelitsa; +Cc: Bart Schaefer, Daniel Shahaf, Zsh hackers list [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-23 10:40 ` Marlon Richert @ 2020-10-23 11:38 ` Roman Perepelitsa 2020-10-24 0:50 ` Functions/Misc/colors vs. region_highlight Bart Schaefer 0 siblings, 1 reply; 33+ messages in thread From: Roman Perepelitsa @ 2020-10-23 11:38 UTC (permalink / raw) To: Marlon Richert; +Cc: Bart Schaefer, Daniel Shahaf, Zsh hackers list 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Functions/Misc/colors vs. region_highlight 2020-10-23 11:38 ` Roman Perepelitsa @ 2020-10-24 0:50 ` Bart Schaefer 2020-11-03 18:54 ` Marlon Richert 2021-04-18 21:40 ` Bart Schaefer 0 siblings, 2 replies; 33+ messages in thread From: Bart Schaefer @ 2020-10-24 0:50 UTC (permalink / raw) To: Roman Perepelitsa; +Cc: Marlon Richert, Daniel Shahaf, Zsh hackers list 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Functions/Misc/colors vs. region_highlight 2020-10-24 0:50 ` Functions/Misc/colors vs. region_highlight Bart Schaefer @ 2020-11-03 18:54 ` Marlon Richert 2020-11-03 23:45 ` Bart Schaefer 2021-04-18 21:40 ` Bart Schaefer 1 sibling, 1 reply; 33+ messages in thread From: Marlon Richert @ 2020-11-03 18:54 UTC (permalink / raw) To: Bart Schaefer; +Cc: Roman Perepelitsa, Daniel Shahaf, Zsh hackers list [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Functions/Misc/colors vs. region_highlight 2020-11-03 18:54 ` Marlon Richert @ 2020-11-03 23:45 ` Bart Schaefer 2020-11-04 15:47 ` Marlon Richert 0 siblings, 1 reply; 33+ messages in thread From: Bart Schaefer @ 2020-11-03 23:45 UTC (permalink / raw) To: Marlon Richert; +Cc: Roman Perepelitsa, Daniel Shahaf, Zsh hackers list [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Functions/Misc/colors vs. region_highlight 2020-11-03 23:45 ` Bart Schaefer @ 2020-11-04 15:47 ` Marlon Richert 0 siblings, 0 replies; 33+ messages in thread From: Marlon Richert @ 2020-11-04 15:47 UTC (permalink / raw) To: Bart Schaefer; +Cc: Roman Perepelitsa, Daniel Shahaf, Zsh hackers list [-- 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 --] ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: Functions/Misc/colors vs. region_highlight 2020-10-24 0:50 ` Functions/Misc/colors vs. region_highlight Bart Schaefer 2020-11-03 18:54 ` Marlon Richert @ 2021-04-18 21:40 ` Bart Schaefer 1 sibling, 0 replies; 33+ messages in thread From: Bart Schaefer @ 2021-04-18 21:40 UTC (permalink / raw) To: Zsh hackers list 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? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Threading across year boundaries (was: Re: region_highlight converts `fg=default` to `none`, which is not the same) 2020-10-23 9:35 ` Roman Perepelitsa 2020-10-23 10:40 ` Marlon Richert @ 2020-10-23 23:57 ` Daniel Shahaf 1 sibling, 0 replies; 33+ messages in thread From: Daniel Shahaf @ 2020-10-23 23:57 UTC (permalink / raw) Cc: Marlon Richert, Zsh hackers list 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-15 7:37 ` Roman Perepelitsa 2020-10-15 16:58 ` Marlon Richert 2020-10-16 13:28 ` Daniel Shahaf @ 2021-03-31 8:26 ` Marlon Richert 2021-04-10 20:33 ` Lawrence Velázquez 2 siblings, 1 reply; 33+ messages in thread From: Marlon Richert @ 2021-03-31 8:26 UTC (permalink / raw) To: Zsh hackers list [-- 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 */ ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2021-03-31 8:26 ` region_highlight converts `fg=default` to `none`, which is not the same Marlon Richert @ 2021-04-10 20:33 ` Lawrence Velázquez 2021-04-13 15:20 ` Daniel Shahaf 0 siblings, 1 reply; 33+ messages in thread From: Lawrence Velázquez @ 2021-04-10 20:33 UTC (permalink / raw) To: zsh-workers; +Cc: Marlon Richert 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2021-04-10 20:33 ` Lawrence Velázquez @ 2021-04-13 15:20 ` Daniel Shahaf 2021-04-13 20:33 ` Roman Perepelitsa 0 siblings, 1 reply; 33+ messages in thread From: Daniel Shahaf @ 2021-04-13 15:20 UTC (permalink / raw) To: Lawrence Velázquez; +Cc: zsh-workers, Marlon Richert 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2021-04-13 15:20 ` Daniel Shahaf @ 2021-04-13 20:33 ` Roman Perepelitsa 2021-04-14 11:04 ` Daniel Shahaf 0 siblings, 1 reply; 33+ messages in thread From: Roman Perepelitsa @ 2021-04-13 20:33 UTC (permalink / raw) To: Daniel Shahaf; +Cc: Lawrence Velázquez, Zsh hackers list, Marlon Richert 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. ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2021-04-13 20:33 ` Roman Perepelitsa @ 2021-04-14 11:04 ` Daniel Shahaf 2021-05-09 20:49 ` Lawrence Velázquez 0 siblings, 1 reply; 33+ messages in thread From: Daniel Shahaf @ 2021-04-14 11:04 UTC (permalink / raw) To: Roman Perepelitsa, Marlon Richert Cc: Lawrence Velázquez, Zsh hackers list 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? ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2021-04-14 11:04 ` Daniel Shahaf @ 2021-05-09 20:49 ` Lawrence Velázquez 2021-05-31 1:16 ` Lawrence Velázquez 0 siblings, 1 reply; 33+ messages in thread From: Lawrence Velázquez @ 2021-05-09 20:49 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2021-05-09 20:49 ` Lawrence Velázquez @ 2021-05-31 1:16 ` Lawrence Velázquez 0 siblings, 0 replies; 33+ messages in thread From: Lawrence Velázquez @ 2021-05-31 1:16 UTC (permalink / raw) To: zsh-workers 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 ^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: region_highlight converts `fg=default` to `none`, which is not the same 2020-10-13 11:05 ` Roman Perepelitsa 2020-10-14 5:12 ` Marlon Richert 2020-10-14 20:46 ` Daniel Shahaf @ 2021-03-31 8:24 ` Marlon Richert 2 siblings, 0 replies; 33+ messages in thread From: Marlon Richert @ 2021-03-31 8:24 UTC (permalink / raw) To: Zsh hackers list [-- 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) { ^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2021-05-31 1:17 UTC | newest] Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-12 9:22 region_highlight converts `fg=default` to `none`, which is not the same Marlon Richert 2020-10-13 11:05 ` Roman Perepelitsa 2020-10-14 5:12 ` Marlon Richert 2020-10-14 20:46 ` Daniel Shahaf 2020-10-15 7:37 ` Roman Perepelitsa 2020-10-15 16:58 ` Marlon Richert 2020-10-15 17:09 ` Roman Perepelitsa 2020-10-16 13:36 ` Daniel Shahaf 2020-10-24 1:34 ` Oliver Kiddle 2020-10-24 6:42 ` Roman Perepelitsa 2020-10-16 13:28 ` Daniel Shahaf 2020-10-16 15:50 ` Bart Schaefer 2020-10-22 19:58 ` Marlon Richert 2020-10-22 23:28 ` Daniel Shahaf 2020-10-23 8:08 ` Roman Perepelitsa 2020-10-23 9:24 ` Marlon Richert 2020-10-23 9:35 ` Roman Perepelitsa 2020-10-23 10:40 ` Marlon Richert 2020-10-23 11:38 ` Roman Perepelitsa 2020-10-24 0:50 ` Functions/Misc/colors vs. region_highlight Bart Schaefer 2020-11-03 18:54 ` Marlon Richert 2020-11-03 23:45 ` Bart Schaefer 2020-11-04 15:47 ` Marlon Richert 2021-04-18 21:40 ` Bart Schaefer 2020-10-23 23:57 ` Threading across year boundaries (was: Re: region_highlight converts `fg=default` to `none`, which is not the same) Daniel Shahaf 2021-03-31 8:26 ` region_highlight converts `fg=default` to `none`, which is not the same Marlon Richert 2021-04-10 20:33 ` Lawrence Velázquez 2021-04-13 15:20 ` Daniel Shahaf 2021-04-13 20:33 ` Roman Perepelitsa 2021-04-14 11:04 ` Daniel Shahaf 2021-05-09 20:49 ` Lawrence Velázquez 2021-05-31 1:16 ` Lawrence Velázquez 2021-03-31 8:24 ` Marlon Richert
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/zsh/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).