Re: region_highlight converts `fg=default` to `none`, which is not the same

[Roman Perepelitsa <roman.perepelitsa@gmail.com>]

[-- 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 */