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

[Marlon Richert <marlon.richert@gmail.com>]

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