zsh-workers
 help / color / mirror / code / Atom feed
* 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  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-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-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

* 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

* 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: 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: 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: 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

* 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: 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

* 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

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).