zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH v2] prompt: support generic non-visible regions
@ 2022-08-11 23:09 Felipe Contreras
  2022-08-14 19:57 ` Bart Schaefer
  2022-08-22  0:11 ` [PATCH v3] " Felipe Contreras
  0 siblings, 2 replies; 10+ messages in thread
From: Felipe Contreras @ 2022-08-11 23:09 UTC (permalink / raw)
  To: zsh-workers; +Cc: Felipe Contreras

readline uses \001 (start of header) and \002 (start of text) as markers
to delimit a non-visible character zone, which are necessary to
calculate the width of a prompt.

In zsh we do this with %{ and %}, but we could support
\001 and \002 as well, so that a function which generates output for
the prompt can use colors in a way that works for both bash and zsh.

This additionally has the benefit of allowing prompts without
PROMPT_PERCENT to use colors correctly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

No changes since v1 other than the commit message.

 Src/prompt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Src/prompt.c b/Src/prompt.c
index 092de63a4..803937029 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -886,6 +886,12 @@ putpromptchar(int doprint, int endchar, zattr *txtchangep)
 		    bv->bp += strlen(bv->bp);
 		}
 	    }
+	} else if(*bv->fm == 0x01) { // start non-visible characters
+	    addbufspc(1);
+	    *bv->bp++ = Inpar;
+	} else if(*bv->fm == 0x02) { // end non-visible characters
+	    addbufspc(1);
+	    *bv->bp++ = Outpar;
 	} else {
 	    char c = *bv->fm == Meta ? *++bv->fm ^ 32 : *bv->fm;
 
-- 
2.37.1



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] prompt: support generic non-visible regions
  2022-08-11 23:09 [PATCH v2] prompt: support generic non-visible regions Felipe Contreras
@ 2022-08-14 19:57 ` Bart Schaefer
  2022-08-14 22:46   ` Felipe Contreras
  2022-08-22  0:11 ` [PATCH v3] " Felipe Contreras
  1 sibling, 1 reply; 10+ messages in thread
From: Bart Schaefer @ 2022-08-14 19:57 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers

On Thu, Aug 11, 2022 at 4:09 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> readline uses \001 (start of header) and \002 (start of text) as markers
> to delimit a non-visible character zone, which are necessary to
> calculate the width of a prompt.

How does one embed a literal ctrl-A or ctrl-B in the prompt?  I
haven't found any readline documentation that explains the use of
"start of header" or "start of text".

In zsh, should an extra digit immediately following \001 be treated as
a "glitch" width?

I'm also leaning to the opinion that \001 and \002 should only be
recognized when PROMPT_PERCENT is unset.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] prompt: support generic non-visible regions
  2022-08-14 19:57 ` Bart Schaefer
@ 2022-08-14 22:46   ` Felipe Contreras
  2022-08-15  1:00     ` Bart Schaefer
  0 siblings, 1 reply; 10+ messages in thread
From: Felipe Contreras @ 2022-08-14 22:46 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

On Sun, Aug 14, 2022 at 2:57 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Thu, Aug 11, 2022 at 4:09 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > readline uses \001 (start of header) and \002 (start of text) as markers
> > to delimit a non-visible character zone, which are necessary to
> > calculate the width of a prompt.
>
> How does one embed a literal ctrl-A or ctrl-B in the prompt?

What would that achieve?

> I haven't found any readline documentation that explains the use of
> "start of header" or "start of text".

It's in the code:

https://git.savannah.gnu.org/cgit/readline.git/tree/display.c#n340

> I'm also leaning to the opinion that \001 and \002 should only be
> recognized when PROMPT_PERCENT is unset.

That would defeat the primary purpose of the patch, which is to be
able to write prompt helper functions which work in multiple shells.

-- 
Felipe Contreras


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] prompt: support generic non-visible regions
  2022-08-14 22:46   ` Felipe Contreras
@ 2022-08-15  1:00     ` Bart Schaefer
  2022-08-15 21:37       ` Felipe Contreras
  0 siblings, 1 reply; 10+ messages in thread
From: Bart Schaefer @ 2022-08-15  1:00 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers

On Sun, Aug 14, 2022 at 3:47 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> > How does one embed a literal ctrl-A or ctrl-B in the prompt?
>
> What would that achieve?

Nothing specific, but one can embed any other literal character, so
why not those?

> > I haven't found any readline documentation that explains the use of
> > "start of header" or "start of text".
>
> It's in the code:

Ah, so "start of header/text" is your phrasing, not theirs.

> > I'm also leaning to the opinion that \001 and \002 should only be
> > recognized when PROMPT_PERCENT is unset.
>
> That would defeat the primary purpose of the patch, which is to be
> able to write prompt helper functions which work in multiple shells.

In that case I think the patch is incomplete, because it's not keeping
track of bv->dontcount and will be confused if both \001 and %} are
used (or %{ and \002).  See lines 602 through 628 of Src/prompt.c
(line numbers as of commit c5a891a2).


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] prompt: support generic non-visible regions
  2022-08-15  1:00     ` Bart Schaefer
@ 2022-08-15 21:37       ` Felipe Contreras
  2022-08-17 22:31         ` Bart Schaefer
  0 siblings, 1 reply; 10+ messages in thread
From: Felipe Contreras @ 2022-08-15 21:37 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

On Sun, Aug 14, 2022 at 8:00 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Sun, Aug 14, 2022 at 3:47 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > > How does one embed a literal ctrl-A or ctrl-B in the prompt?
> >
> > What would that achieve?
>
> Nothing specific, but one can embed any other literal character, so
> why not those?

We could make the code more complex in order to support doing that,
but without knowing the use cases it's unclear how we would test that
it works correctly and it's useful.

Do we want something like "\001foo\002%\001bar%\002"?

> > > I haven't found any readline documentation that explains the use of
> > > "start of header" or "start of text".
> >
> > It's in the code:
>
> Ah, so "start of header/text" is your phrasing, not theirs.

It's not my phrasing, it's what I've found these characters are called [1].

01 SOH Start of Heading
02 STX Start of Text

> > > I'm also leaning to the opinion that \001 and \002 should only be
> > > recognized when PROMPT_PERCENT is unset.
> >
> > That would defeat the primary purpose of the patch, which is to be
> > able to write prompt helper functions which work in multiple shells.
>
> In that case I think the patch is incomplete, because it's not keeping
> track of bv->dontcount and will be confused if both \001 and %} are
> used (or %{ and \002).  See lines 602 through 628 of Src/prompt.c
> (line numbers as of commit c5a891a2).

Yes, I've seen that code, but in my view mixing \001 and %{ doesn't
seem to be a useful use case.

Why would somebody do something like this for example:

\001%F{red}foo%f %{$color%}bar%{$reset%}\002

Or this:

%{\001$color\002foo\001$reset\002%}

I can add code to make %{ and \001 be equivalent, if that's what we
want to do, but how am I supposed to do a reasonable test?

Cheers.

[1] https://www.ascii-code.com/

-- 
Felipe Contreras


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] prompt: support generic non-visible regions
  2022-08-15 21:37       ` Felipe Contreras
@ 2022-08-17 22:31         ` Bart Schaefer
  2022-08-17 22:48           ` Bart Schaefer
  2022-08-22  0:15           ` Felipe Contreras
  0 siblings, 2 replies; 10+ messages in thread
From: Bart Schaefer @ 2022-08-17 22:31 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers

On Mon, Aug 15, 2022 at 2:37 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Do we want something like "\001foo\002%\001bar%\002"?

More on this below.

> On Sun, Aug 14, 2022 at 8:00 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
> >
> > On Sun, Aug 14, 2022 at 3:47 PM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
> > >
> > > That would defeat the primary purpose of the patch, which is to be
> > > able to write prompt helper functions which work in multiple shells.
> >
> > In that case I think the patch is incomplete, because it's not keeping
> > track of bv->dontcount and will be confused if both \001 and %} are
> > used (or %{ and \002).  See lines 602 through 628 of Src/prompt.c
> > (line numbers as of commit c5a891a2).
>
> Yes, I've seen that code, but in my view mixing \001 and %{ doesn't
> seem to be a useful use case.

Your point above about writing helper functions would seem to imply
that someone might inadvertently use such a helper (e.g. via
PROMPT_SUBST) inside a section that was already surrounded by %{ %}
(or surrounded by \001 \002 though that does seem less likely).  The
purpose of bv->dontcount (or at least one purpose thereof) is to
permit nesting zero-width sections.  I agree it wouldn't make sense to
(for example) begin a section with %{ and intentionally end it with
\002.

> I can add code to make %{ and \001 be equivalent, if that's what we
> want to do,

Ideally they'd in fact be distinct (so it would be some kind of an
error to have %{...\002 without an intervening \001 and so on)  but
that might require a lot more fiddling.

> but how am I supposed to do a reasonable test?

There isn't a Test/ case for it yet even now.  How are we handling %}
when there's no %{ before it?  I think the answer is that we don't
have a test for that either ...


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] prompt: support generic non-visible regions
  2022-08-17 22:31         ` Bart Schaefer
@ 2022-08-17 22:48           ` Bart Schaefer
  2022-08-22  0:17             ` Felipe Contreras
  2022-08-22  0:15           ` Felipe Contreras
  1 sibling, 1 reply; 10+ messages in thread
From: Bart Schaefer @ 2022-08-17 22:48 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers

On Wed, Aug 17, 2022 at 3:31 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Mon, Aug 15, 2022 at 2:37 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Do we want something like "\001foo\002%\001bar%\002"?
>
> More on this below.

Oops, forgot to come back to that.

If nesting isn't a concern, then %{\001%} would do it, just change the
tests in the patch to be
(bv->dontcount == 0 && *bv->fm == 0x01)
I don't think there's any reason to treat control characters as non-zero-width?

(Note I'm hoping for input from others than Filipe and I on whether
nesting is important.)

If nesting is a problem, then I suppose yes, we need %\001 like we
have %% and %).  Other suggestions welcome.


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v3] prompt: support generic non-visible regions
  2022-08-11 23:09 [PATCH v2] prompt: support generic non-visible regions Felipe Contreras
  2022-08-14 19:57 ` Bart Schaefer
@ 2022-08-22  0:11 ` Felipe Contreras
  1 sibling, 0 replies; 10+ messages in thread
From: Felipe Contreras @ 2022-08-22  0:11 UTC (permalink / raw)
  To: zsh-workers; +Cc: Felipe Contreras

readline uses \001 (start of header) and \002 (start of text) as markers
to delimit a non-visible character zone, which are necessary to
calculate the width of a prompt.

In zsh we do this with %{ and %}, but we could support
\001 and \002 as well, so that a function which generates output for
the prompt can use colors in a way that works for both bash and zsh.

This additionally has the benefit of allowing prompts without
PROMPT_PERCENT to use colors correctly.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---

Since v2 I added checks to allow nesting of these non-visible regions as
Bart Schaefer suggested.

Interdiff against v2:
  diff --git a/Src/prompt.c b/Src/prompt.c
  index 803937029..ec74cc835 100644
  --- a/Src/prompt.c
  +++ b/Src/prompt.c
  @@ -887,11 +887,15 @@ putpromptchar(int doprint, int endchar, zattr *txtchangep)
   		}
   	    }
   	} else if(*bv->fm == 0x01) { // start non-visible characters
  -	    addbufspc(1);
  -	    *bv->bp++ = Inpar;
  +	    if (!bv->dontcount++) {
  +		addbufspc(1);
  +		*bv->bp++ = Inpar;
  +	    }
   	} else if(*bv->fm == 0x02) { // end non-visible characters
  -	    addbufspc(1);
  -	    *bv->bp++ = Outpar;
  +	    if (bv->dontcount && !--bv->dontcount) {
  +		addbufspc(1);
  +		*bv->bp++ = Outpar;
  +	    }
   	} else {
   	    char c = *bv->fm == Meta ? *++bv->fm ^ 32 : *bv->fm;
   

 Src/prompt.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Src/prompt.c b/Src/prompt.c
index 092de63a4..ec74cc835 100644
--- a/Src/prompt.c
+++ b/Src/prompt.c
@@ -886,6 +886,16 @@ putpromptchar(int doprint, int endchar, zattr *txtchangep)
 		    bv->bp += strlen(bv->bp);
 		}
 	    }
+	} else if(*bv->fm == 0x01) { // start non-visible characters
+	    if (!bv->dontcount++) {
+		addbufspc(1);
+		*bv->bp++ = Inpar;
+	    }
+	} else if(*bv->fm == 0x02) { // end non-visible characters
+	    if (bv->dontcount && !--bv->dontcount) {
+		addbufspc(1);
+		*bv->bp++ = Outpar;
+	    }
 	} else {
 	    char c = *bv->fm == Meta ? *++bv->fm ^ 32 : *bv->fm;
 
-- 
2.37.2.351.g9bf691b78c.dirty



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] prompt: support generic non-visible regions
  2022-08-17 22:31         ` Bart Schaefer
  2022-08-17 22:48           ` Bart Schaefer
@ 2022-08-22  0:15           ` Felipe Contreras
  1 sibling, 0 replies; 10+ messages in thread
From: Felipe Contreras @ 2022-08-22  0:15 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

On Wed, Aug 17, 2022 at 5:31 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Mon, Aug 15, 2022 at 2:37 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Do we want something like "\001foo\002%\001bar%\002"?
>
> More on this below.
>
> > On Sun, Aug 14, 2022 at 8:00 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
> > >
> > > On Sun, Aug 14, 2022 at 3:47 PM Felipe Contreras
> > > <felipe.contreras@gmail.com> wrote:
> > > >
> > > > That would defeat the primary purpose of the patch, which is to be
> > > > able to write prompt helper functions which work in multiple shells.
> > >
> > > In that case I think the patch is incomplete, because it's not keeping
> > > track of bv->dontcount and will be confused if both \001 and %} are
> > > used (or %{ and \002).  See lines 602 through 628 of Src/prompt.c
> > > (line numbers as of commit c5a891a2).
> >
> > Yes, I've seen that code, but in my view mixing \001 and %{ doesn't
> > seem to be a useful use case.
>
> Your point above about writing helper functions would seem to imply
> that someone might inadvertently use such a helper (e.g. via
> PROMPT_SUBST) inside a section that was already surrounded by %{ %}
> (or surrounded by \001 \002 though that does seem less likely).  The
> purpose of bv->dontcount (or at least one purpose thereof) is to
> permit nesting zero-width sections.  I agree it wouldn't make sense to
> (for example) begin a section with %{ and intentionally end it with
> \002.

Yeah, I guess that's possible, although I don't think very likely.

> > but how am I supposed to do a reasonable test?
>
> There isn't a Test/ case for it yet even now.  How are we handling %}
> when there's no %{ before it?  I think the answer is that we don't
> have a test for that either ...

I didn't mean a proper test case, I meant just to check that the code
actually works.

Anyway, this is how I decided to test it:

  $' %{\e[31m\x01\e[31m\x01\e[31m\x02\e[31m\x02\e[31m%}master%{\e[m\x01\e[m\x01\e[m\x02\e[m\x02\e[m%}'

I've sent patch v3 and seems to work fine with all this nesting.

-- 
Felipe Contreras


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2] prompt: support generic non-visible regions
  2022-08-17 22:48           ` Bart Schaefer
@ 2022-08-22  0:17             ` Felipe Contreras
  0 siblings, 0 replies; 10+ messages in thread
From: Felipe Contreras @ 2022-08-22  0:17 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

On Wed, Aug 17, 2022 at 5:48 PM Bart Schaefer <schaefer@brasslantern.com> wrote:

> (Note I'm hoping for input from others than Filipe and I on whether
> nesting is important.)

BTW. It's Felipe.

-- 
Felipe Contreras


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-08-22  0:18 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-11 23:09 [PATCH v2] prompt: support generic non-visible regions Felipe Contreras
2022-08-14 19:57 ` Bart Schaefer
2022-08-14 22:46   ` Felipe Contreras
2022-08-15  1:00     ` Bart Schaefer
2022-08-15 21:37       ` Felipe Contreras
2022-08-17 22:31         ` Bart Schaefer
2022-08-17 22:48           ` Bart Schaefer
2022-08-22  0:17             ` Felipe Contreras
2022-08-22  0:15           ` Felipe Contreras
2022-08-22  0:11 ` [PATCH v3] " Felipe Contreras

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