Re: PATCH: remove OFF flags for display attributes in zattr

[Oliver Kiddle <opk@xxxxxxx>]

Following is an additional patch which clears up some things like macro
names and resolves a couple of issues found with further testing.

The first issue was with colours from an earlier ${(%)...} substitution
being restored following %b in PROMPT_EOL_MARK. This now marks all
attributes as unknown before evaluating it which accurately reflects
the fact that attributes can be anything following the user command.
zrefresh() explicitly unsets all attributes followed by (superfluously?)
standout and underline but that doesn't occur until later.

Another fix was to limit the restored attributes (after %b) to
TXT_ATTR_ALL because we only care about, e.g. TXTFGCOLOUR and not
anything in TXT_ATTR_FG_MASK

The wider use of a mask of unknown attributes required more complete
handling of its value. I'm not certain this is ever needed in the case
of the treplaceattrs() function but it is at least complete and
consistent that to include it there.

Oliver

diff --git a/Src/Zle/zle.h b/Src/Zle/zle.h
index 97cc7d797..1a3e4c241 100644
--- a/Src/Zle/zle.h
+++ b/Src/Zle/zle.h
@@ -490,11 +490,7 @@ typedef struct {
      */
     REFRESH_CHAR chr;
     /*
-     * Its attributes.  'On' attributes (TXT_ATTR_ON_MASK) are
-     * applied before the character, 'off' attributes (TXT_ATTR_OFF_MASK)
-     * after it.  'On' attributes are present for all characters that
-     * need the effect; 'off' attributes are only present for the
-     * last character in the sequence.
+     * Its attributes.
      */
     zattr atr;
 } REFRESH_ELEMENT;
diff --git a/Src/Zle/zle_refresh.c b/Src/Zle/zle_refresh.c
index 05e0e4dce..d56478147 100644
--- a/Src/Zle/zle_refresh.c
+++ b/Src/Zle/zle_refresh.c
@@ -599,7 +599,7 @@ zwcputc(const REFRESH_ELEMENT *c)
     VARARR(char, mbtmp, MB_CUR_MAX + 1);
 #endif
 
-    txtpendingattrs = c->atr;
+    treplaceattrs(c->atr);
     applytextattributes(0);
 
 #ifdef MULTIBYTE_SUPPORT
@@ -1107,7 +1107,7 @@ zrefresh(void)
 	tsetcap(TCALLATTRSOFF, 0);
 	tsetcap(TCSTANDOUTEND, 0);
 	tsetcap(TCUNDERLINEEND, 0);
-	txtcurrentattrs = txtunknownattrs = 0;
+	txtcurrentattrs = txtpendingattrs = txtunknownattrs = 0;
 
 	if (trashedzle && !clearflag)
 	    reexpandprompt(); 
@@ -1132,7 +1132,7 @@ zrefresh(void)
 	    if (lpromptwof == winw)
 		zputs("\n", shout);	/* works with both hasam and !hasam */
 	} else {
-	    txtpendingattrs = pmpt_attr;
+	    treplaceattrs(pmpt_attr);
 	    applytextattributes(0);
 	}
 	if (clearflag) {
@@ -1205,7 +1205,7 @@ zrefresh(void)
 	if (special_attr & (TXTFGCOLOUR|TXTBGCOLOUR)) {
 	    /* keep colours from special attributes */
 	    all_attr = special_attr |
-		(base_attr & ~TXT_ATTR_COLOUR_ON_MASK);
+		(base_attr & ~TXT_ATTR_COLOUR_MASK);
 	} else {
 	    /* keep colours from standard attributes */
 	    all_attr = special_attr | base_attr;
@@ -1629,7 +1629,7 @@ zrefresh(void)
 		vcs = 0;
 	    }
 	    /* reset character attributes to that set by the main prompt */
-	    txtpendingattrs = pmpt_attr;
+	    treplaceattrs(pmpt_attr);
 	    applytextattributes(0);
 	}
     }
@@ -1645,7 +1645,7 @@ individually */
 
 /* reset character attributes */
     if (clearf && postedit) {
-	txtpendingattrs = pmpt_attr ? pmpt_attr : rpmpt_attr;
+	treplaceattrs(pmpt_attr ? pmpt_attr : rpmpt_attr);
 	applytextattributes(0);
     }
     clearf = 0;
@@ -2037,7 +2037,7 @@ refreshline(int ln)
 	    break;
 	do {
 #endif
-	    txtpendingattrs = nl->atr;
+	    treplaceattrs(nl->atr);
 	    applytextattributes(0);
 
 	    /*
@@ -2434,7 +2434,7 @@ singlerefresh(ZLE_STRING_T tmpline, int tmpll, int tmpcs)
 		t0 < rhp->end + offset) {
 		if (base_attr & (TXTFGCOLOUR|TXTBGCOLOUR)) {
 		    /* keep colour already set */
-		    base_attr |= rhp->atr & ~TXT_ATTR_COLOUR_ON_MASK;
+		    base_attr |= rhp->atr & ~TXT_ATTR_COLOUR_MASK;
 		} else {
 		    /* no colour set yet */
 		    base_attr |= rhp->atr;
@@ -2444,7 +2444,7 @@ singlerefresh(ZLE_STRING_T tmpline, int tmpll, int tmpcs)
 	if (special_attr & (TXTFGCOLOUR|TXTBGCOLOUR)) {
 	    /* keep colours from special attributes */
 	    all_attr = special_attr |
-		(base_attr & ~TXT_ATTR_COLOUR_ON_MASK);
+		(base_attr & ~TXT_ATTR_COLOUR_MASK);
 	} else {
 	    /* keep colours from standard attributes */
 	    all_attr = special_attr | base_attr;
diff --git a/Src/builtin.c b/Src/builtin.c
index 49f940e1c..4c295d11f 100644
--- a/Src/builtin.c
+++ b/Src/builtin.c
@@ -4603,7 +4603,8 @@ bin_print(char *name, char **args, Options ops, int func)
     /* compute lengths, and interpret according to -P, -D, -e, etc. */
     argc = arrlen(args);
     len = (int *) hcalloc(argc * sizeof(int));
-    txtunknownattrs = TXT_ATTR_ALL;
+    if (OPT_ISSET(ops, 'P'))
+	txtunknownattrs = TXT_ATTR_ALL;
     for (n = 0; n < argc; n++) {
 	/* first \ sequences */
 	if (fmt ||
@@ -4656,7 +4657,6 @@ bin_print(char *name, char **args, Options ops, int func)
 	    unqueue_signals();
 	}
     }
-    txtunknownattrs = 0;
 
     /* -o and -O -- sort the arguments */
     if (OPT_ISSET(ops,'o') || OPT_ISSET(ops,'O')) {
diff --git a/Src/prompt.c b/Src/prompt.c
index 8f0a5073d..880194f87 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -1593,18 +1593,19 @@ applytextattributes(int flags)
     if (keepcount < turncount || (change & ~txtpendingattrs & TXTBOLDFACE)) {
 	tsetcap(TCALLATTRSOFF, flags);
 	/* this cleared all attributes, may need to restore some */
-	change = txtpendingattrs;
+	change = txtpendingattrs & TXT_ATTR_ALL & ~txtunknownattrs;
 	txtunknownattrs = 0;
     } else {
 	if (change & ~txtpendingattrs & TXTSTANDOUT) {
 	    tsetcap(TCSTANDOUTEND, flags);
 	    /* in some cases, that clears all attributes */
-	    change = txtpendingattrs | (TXTUNDERLINE & change);
+	    change = (txtpendingattrs & TXT_ATTR_ALL & ~txtunknownattrs) |
+		    (TXTUNDERLINE & change);
 	}
 	if (change & ~txtpendingattrs & TXTUNDERLINE) {
 	    tsetcap(TCUNDERLINEEND, flags);
 	    /* in some cases, that clears all attributes */
-	    change = txtpendingattrs;
+	    change = txtpendingattrs & TXT_ATTR_ALL & ~txtunknownattrs;
 	}
     }
     if (change & txtpendingattrs & TXTBOLDFACE)
@@ -1614,9 +1615,9 @@ applytextattributes(int flags)
     if (change & txtpendingattrs & TXTUNDERLINE)
 	tsetcap(TCUNDERLINEBEG, flags);
 
-    if (change & TXT_ATTR_FG_ON_MASK)
+    if (change & TXT_ATTR_FG_MASK)
 	set_colour_attribute(txtpendingattrs, COL_SEQ_FG, flags);
-    if (change & TXT_ATTR_BG_ON_MASK)
+    if (change & TXT_ATTR_BG_MASK)
 	set_colour_attribute(txtpendingattrs, COL_SEQ_BG, flags);
 
     txtcurrentattrs = txtpendingattrs;
@@ -1626,12 +1627,27 @@ applytextattributes(int flags)
 mod_export void
 cleartextattributes(int flags)
 {
-    txtpendingattrs = 0;
+    treplaceattrs(0);
     applytextattributes(flags);
 }
 
 /**/
-void
+mod_export void
+treplaceattrs(zattr newattrs)
+{
+    if (txtunknownattrs) {
+	/* Set current attributes to the opposite of the new ones
+	 * for any that are unknown so that applytextattributes()
+	 * detects them as changed. */
+	txtcurrentattrs &= ~txtunknownattrs;
+	txtcurrentattrs |= txtunknownattrs & ~newattrs;
+    }
+
+    txtpendingattrs = newattrs;
+}
+
+/**/
+mod_export void
 tsetattrs(zattr newattrs)
 {
     /* assume any unknown attributes that we're now setting were unset */
@@ -1639,18 +1655,17 @@ tsetattrs(zattr newattrs)
 
     txtpendingattrs |= newattrs & TXT_ATTR_ALL;
     if (newattrs & TXTFGCOLOUR) {
-	txtpendingattrs = (txtpendingattrs &
-	    ~(TXT_ATTR_FG_COL_MASK|TXT_ATTR_FG_24BIT)) |
-	    (newattrs & (TXT_ATTR_FG_ON_MASK|TXT_ATTR_FG_24BIT));
+	txtpendingattrs &= ~TXT_ATTR_FG_MASK;
+	txtpendingattrs |= newattrs & TXT_ATTR_FG_MASK;
     }
     if (newattrs & TXTBGCOLOUR) {
-	txtpendingattrs &= ~(TXT_ATTR_BG_COL_MASK|TXT_ATTR_BG_24BIT);
-	txtpendingattrs |= newattrs & TXT_ATTR_BG_ON_MASK;
+	txtpendingattrs &= ~TXT_ATTR_BG_MASK;
+	txtpendingattrs |= newattrs & TXT_ATTR_BG_MASK;
     }
 }
 
 /**/
-void
+mod_export void
 tunsetattrs(zattr newattrs)
 {
     /* assume any unknown attributes that we're now unsetting were set */
@@ -1658,9 +1673,9 @@ tunsetattrs(zattr newattrs)
 
     txtpendingattrs &= ~(newattrs & TXT_ATTR_ALL);
     if (newattrs & TXTFGCOLOUR)
-	txtpendingattrs &= ~(TXTFGCOLOUR|TXT_ATTR_FG_COL_MASK|TXT_ATTR_FG_24BIT);
+	txtpendingattrs &= ~TXT_ATTR_FG_MASK;
     if (newattrs & TXTBGCOLOUR)
-	txtpendingattrs &= ~(TXTBGCOLOUR|TXT_ATTR_BG_COL_MASK|TXT_ATTR_BG_24BIT);
+	txtpendingattrs &= ~TXT_ATTR_BG_MASK;
 }
 
 /*****************************************************************************
diff --git a/Src/subst.c b/Src/subst.c
index 1e8350ffe..897188862 100644
--- a/Src/subst.c
+++ b/Src/subst.c
@@ -3716,6 +3716,8 @@ colonsubscript:
     if (presc) {
 	int ops = opts[PROMPTSUBST], opb = opts[PROMPTBANG];
 	int opp = opts[PROMPTPERCENT];
+	zattr savecurrent = txtcurrentattrs;
+	zattr saveunknown = txtunknownattrs;
 
 	if (presc < 2) {
 	    opts[PROMPTPERCENT] = 1;
@@ -3753,7 +3755,9 @@ colonsubscript:
 	    val = dupstring(tmps);
 	    free(tmps);
 	}
-	txtunknownattrs = 0;
+
+	txtpendingattrs = txtcurrentattrs = savecurrent;
+	txtunknownattrs = saveunknown;
 	opts[PROMPTSUBST] = ops;
 	opts[PROMPTBANG] = opb;
 	opts[PROMPTPERCENT] = opp;
diff --git a/Src/utils.c b/Src/utils.c
index 8b60d652c..55f2d1ab0 100644
--- a/Src/utils.c
+++ b/Src/utils.c
@@ -1543,6 +1543,7 @@ preprompt(void)
 	if (!eolmark)
 	    eolmark = "%B%S%#%s%b";
 	opts[PROMPTPERCENT] = 1;
+	txtunknownattrs = TXT_ATTR_ALL;
 	str = promptexpand(eolmark, 1, NULL, NULL);
 	countprompt(str, &w, 0, -1);
 	opts[PROMPTPERCENT] = percents;
diff --git a/Src/zsh.h b/Src/zsh.h
index b9fa253d7..35ae033e3 100644
--- a/Src/zsh.h
+++ b/Src/zsh.h
@@ -2707,16 +2707,16 @@ struct ttyinfo {
 #define TXT_ATTR_BG_24BIT        0x8000
 
 /* Mask out everything to do with setting a foreground colour */
-#define TXT_ATTR_FG_ON_MASK \
+#define TXT_ATTR_FG_MASK \
     (TXTFGCOLOUR|TXT_ATTR_FG_COL_MASK|TXT_ATTR_FG_24BIT)
 
 /* Mask out everything to do with setting a background colour */
-#define TXT_ATTR_BG_ON_MASK \
+#define TXT_ATTR_BG_MASK \
     (TXTBGCOLOUR|TXT_ATTR_BG_COL_MASK|TXT_ATTR_BG_24BIT)
 
 /* Mask out everything to do with activating colours */
-#define TXT_ATTR_COLOUR_ON_MASK			\
-    (TXT_ATTR_FG_ON_MASK|TXT_ATTR_BG_ON_MASK)
+#define TXT_ATTR_COLOUR_MASK \
+    (TXT_ATTR_FG_MASK|TXT_ATTR_BG_MASK)
 
 #define txtchangeget(T,A)	(((T) & A ## _MASK) >> A ## _SHIFT)
 
diff --git a/Test/D01prompt.ztst b/Test/D01prompt.ztst
index ae8c78ef6..a0abb7e1d 100644
--- a/Test/D01prompt.ztst
+++ b/Test/D01prompt.ztst
@@ -264,6 +264,13 @@
   [[ $A3 = $A1$A2 && -n $A1 && -n $A2 ]]
 0:Attribute optimisation - preserve initial disabling of attribute but drop useless later one
 
+  : ${(%):-%K{blue}}
+  A1="${(%):-%b}x"
+  : ${(%):-%k}
+  A2="${(%):-%b}x"
+  [[ $A1 = $A2 && -n $A1 && -n $A2 ]]
+0:Don't restore attributes from earlier substitution after disabling bold
+
  (RPS1=foo; echo $RPS1 $RPROMPT)
  (RPS2=bar; echo $RPS2 $RPROMPT2)
 -fD:RPS1 and RPROMPT are aliases (regression from 5.0.6) (workers/49600)