zsh-workers
 help / color / mirror / code / Atom feed
* [PATCH] prompt: support generic non-visible regions
@ 2022-08-10 11:51 Felipe Contreras
  2022-08-10 18:59 ` Mikael Magnusson
  2022-08-11 14:37 ` Roman Perepelitsa
  0 siblings, 2 replies; 21+ messages in thread
From: Felipe Contreras @ 2022-08-10 11:51 UTC (permalink / raw)
  To: zsh-workers; +Cc: Oliver Kiddle, dana, Felipe Contreras

readline assumes anything between \001 (start of header) and \002 (start
of text) is non-visible characters.

In zsh we do this with `%F{color}`, but we could support
`\001\e[31m\002` as well.

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

I don't know if this is the right way to do it, but in my limited
testing it seems to work fine.

 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] 21+ messages in thread

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-10 11:51 [PATCH] prompt: support generic non-visible regions Felipe Contreras
@ 2022-08-10 18:59 ` Mikael Magnusson
  2022-08-10 19:56   ` Felipe Contreras
  2022-08-11 14:37 ` Roman Perepelitsa
  1 sibling, 1 reply; 21+ messages in thread
From: Mikael Magnusson @ 2022-08-10 18:59 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers, Oliver Kiddle, dana

On 8/10/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> readline assumes anything between \001 (start of header) and \002 (start
> of text) is non-visible characters.
>
> In zsh we do this with `%F{color}`, but we could support
> `\001\e[31m\002` as well.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>
> I don't know if this is the right way to do it, but in my limited
> testing it seems to work fine.

The commit message seems a bit confused, %F has nothing to do with
marking characters as 0-width, rather everything between %{ and %}
will be treated as such. You can use %{%} (or \001 and \002) to set
colors via the specific \e [ Ps m code, but also to send any other
codes handled by the terminal.

That aside, I don't really see a reason to add support for
bash-specific prompt sequences that are more cumbersome to use than
the already existing zsh ones. Bash prompts are already completely
incompatible anyway.

-- 
Mikael Magnusson


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-10 18:59 ` Mikael Magnusson
@ 2022-08-10 19:56   ` Felipe Contreras
  2022-08-11 14:02     ` Mikael Magnusson
  0 siblings, 1 reply; 21+ messages in thread
From: Felipe Contreras @ 2022-08-10 19:56 UTC (permalink / raw)
  To: Mikael Magnusson; +Cc: zsh-workers, Oliver Kiddle, dana

On Wed, Aug 10, 2022 at 1:59 PM Mikael Magnusson <mikachu@gmail.com> wrote:
>
> On 8/10/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> > readline assumes anything between \001 (start of header) and \002 (start
> > of text) is non-visible characters.
> >
> > In zsh we do this with `%F{color}`, but we could support
> > `\001\e[31m\002` as well.
> >
> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> > ---
> >
> > I don't know if this is the right way to do it, but in my limited
> > testing it seems to work fine.
>
> The commit message seems a bit confused, %F has nothing to do with
> marking characters as 0-width, rather everything between %{ and %}
> will be treated as such. You can use %{%} (or \001 and \002) to set
> colors via the specific \e [ Ps m code, but also to send any other
> codes handled by the terminal.

If I do PS1='%F{red}foo' putpromptchar() will call
set_colour_attribute(), which eventually calls this:

  if (!bv->dontcount) {
      addbufspc(1);
      *bv->bp++ = Inpar;
  }
  tputs(tgoto(tcstr[tc], colour, colour), 1, putstr);
  if (!bv->dontcount) {
      addbufspc(1);
      *bv->bp++ = Outpar;
  }

I can do the same thing fputs() is doing with PS1=$'\e[31mfoo', but
now zsh will think my prompt is bigger than it actually is and the
shell will be screwed. So I have to put that inside %{%}.

Therefore "%F{red}" = $'%{\e[31m%}'

> That aside, I don't really see a reason to add support for
> bash-specific prompt sequences that are more cumbersome to use than
> the already existing zsh ones. Bash prompts are already completely
> incompatible anyway.

The prompts are not compatible, but the functions used in those prompts can be.

  __git_ps1_test () {
    local branch='master'
    local red=$'\001\e[31m\002'
    local clear=$'\001\e[m\002'
    echo "${red}${branch}${clear}"
  }

The function above works perfectly fine in bash and zsh with my patch,
and I can add $(__git_ps1_test) to both of my prompts.

-- 
Felipe Contreras


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-10 19:56   ` Felipe Contreras
@ 2022-08-11 14:02     ` Mikael Magnusson
  2022-08-11 14:26       ` Felipe Contreras
  0 siblings, 1 reply; 21+ messages in thread
From: Mikael Magnusson @ 2022-08-11 14:02 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers, Oliver Kiddle, dana

On 8/10/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> On Wed, Aug 10, 2022 at 1:59 PM Mikael Magnusson <mikachu@gmail.com> wrote:
>>
>> On 8/10/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> > readline assumes anything between \001 (start of header) and \002
>> > (start
>> > of text) is non-visible characters.
>> >
>> > In zsh we do this with `%F{color}`, but we could support
>> > `\001\e[31m\002` as well.
>> >
>> > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> > ---
>> >
>> > I don't know if this is the right way to do it, but in my limited
>> > testing it seems to work fine.
>>
>> The commit message seems a bit confused, %F has nothing to do with
>> marking characters as 0-width, rather everything between %{ and %}
>> will be treated as such. You can use %{%} (or \001 and \002) to set
>> colors via the specific \e [ Ps m code, but also to send any other
>> codes handled by the terminal.
>
> If I do PS1='%F{red}foo' putpromptchar() will call
> set_colour_attribute(), which eventually calls this:
>
>   if (!bv->dontcount) {
>       addbufspc(1);
>       *bv->bp++ = Inpar;
>   }
>   tputs(tgoto(tcstr[tc], colour, colour), 1, putstr);
>   if (!bv->dontcount) {
>       addbufspc(1);
>       *bv->bp++ = Outpar;
>   }
>
> I can do the same thing fputs() is doing with PS1=$'\e[31mfoo', but
> now zsh will think my prompt is bigger than it actually is and the
> shell will be screwed. So I have to put that inside %{%}.
>
> Therefore "%F{red}" = $'%{\e[31m%}'

Sure, this agrees 100% with what I was saying, but the statement in
the commit still doesn't make sense. You've proven that you can use %{
or \001 to do what %F{} does, but the commit message states the
opposite which is not true.

>> That aside, I don't really see a reason to add support for
>> bash-specific prompt sequences that are more cumbersome to use than
>> the already existing zsh ones. Bash prompts are already completely
>> incompatible anyway.
>
> The prompts are not compatible, but the functions used in those prompts can
> be.
>
>   __git_ps1_test () {
>     local branch='master'
>     local red=$'\001\e[31m\002'
>     local clear=$'\001\e[m\002'
>     echo "${red}${branch}${clear}"
>   }
>
> The function above works perfectly fine in bash and zsh with my patch,
> and I can add $(__git_ps1_test) to both of my prompts.

You could also have a $START_INVIS and $END_INVIS parameter depending
on what shell you're in. Well, I don't have a strong opinion either
way, but it doesn't seem worth it to me. If we support this, people
might ask "but you support \001, why not this other xyz prompt feature
from bash?"

-- 
Mikael Magnusson


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 14:02     ` Mikael Magnusson
@ 2022-08-11 14:26       ` Felipe Contreras
  2022-08-11 18:32         ` Mikael Magnusson
  0 siblings, 1 reply; 21+ messages in thread
From: Felipe Contreras @ 2022-08-11 14:26 UTC (permalink / raw)
  To: Mikael Magnusson; +Cc: zsh-workers, Oliver Kiddle, dana

On Thu, Aug 11, 2022 at 9:02 AM Mikael Magnusson <mikachu@gmail.com> wrote:
> On 8/10/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> > On Wed, Aug 10, 2022 at 1:59 PM Mikael Magnusson <mikachu@gmail.com> wrote:

> >> The commit message seems a bit confused, %F has nothing to do with
> >> marking characters as 0-width, rather everything between %{ and %}
> >> will be treated as such. You can use %{%} (or \001 https://www.logicallyfallacious.com/logicalfallacies/Slippery-Slopeand \002) to set
> >> colors via the specific \e [ Ps m code, but also to send any other
> >> codes handled by the terminal.
> >
> > If I do PS1='%F{red}foo' putpromptchar() will call
> > set_colour_attribute(), which eventually calls this:
> >
> >   if (!bv->dontcount) {
> >       addbufspc(1);
> >       *bv->bp++ = Inpar;
> >   }
> >   tputs(tgoto(tcstr[tc], colour, colour), 1, putstr);
> >   if (!bv->dontcount) {
> >       addbufspc(1);
> >       *bv->bp++ = Outpar;
> >   }
> >
> > I can do the same thing fputs() is doing with PS1=$'\e[31mfoo', but
> > now zsh will think my prompt is bigger than it actually is and the
> > shell will be screwed. So I have to put that inside %{%}.
> >
> > Therefore "%F{red}" = $'%{\e[31m%}'
>
> Sure, this agrees 100% with what I was saying, but the statement in
> the commit still doesn't make sense. You've proven that you can use %{
> or \001 to do what %F{} does, but the commit message states the
> opposite which is not true.

The commit message says:

1. We do readline $'\001\e[31m\002' as zsh '%F{red}'.

This is true.

2. We could support $'\001\e[31m\002'.

This is also true.

> >> That aside, I don't really see a reason to add support for
> >> bash-specific prompt sequences that are more cumbersome to use than
> >> the already existing zsh ones. Bash prompts are already completely
> >> incompatible anyway.
> >
> > The prompts are not compatible, but the functions used in those prompts can
> > be.
> >
> >   __git_ps1_test () {
> >     local branch='master'
> >     local red=$'\001\e[31m\002'
> >     local clear=$'\001\e[m\002'
> >     echo "${red}${branch}${clear}"
> >   }
> >
> > The function above works perfectly fine in bash and zsh with my patch,
> > and I can add $(__git_ps1_test) to both of my prompts.
>
> You could also have a $START_INVIS and $END_INVIS parameter depending
> on what shell you're in.

Yes, and I can do red=$'\001\e[31m\002' or red='%F{red}' depending
what shell I'm in, which is what I was doing.

But *why* would I do that when zsh is perfectly capable of
interpreting \001 and \002? (with my patch)

> Well, I don't have a strong opinion either
> way, but it doesn't seem worth it to me. If we support this, people
> might ask "but you support \001, why not this other xyz prompt feature
> from bash?"

Slippery slope fallacy [1]. Yes, they might ask that, and you might
answer "no, unline \001, xyz requires too much effort".

This doesn't require too much effort.

[1] https://www.logicallyfallacious.com/logicalfallacies/Slippery-Slope

-- 
Felipe Contreras


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-10 11:51 [PATCH] prompt: support generic non-visible regions Felipe Contreras
  2022-08-10 18:59 ` Mikael Magnusson
@ 2022-08-11 14:37 ` Roman Perepelitsa
  2022-08-11 15:21   ` Bart Schaefer
  2022-08-11 17:24   ` Roman Perepelitsa
  1 sibling, 2 replies; 21+ messages in thread
From: Roman Perepelitsa @ 2022-08-11 14:37 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers, Oliver Kiddle, dana

On Wed, Aug 10, 2022 at 1:56 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> readline assumes anything between \001 (start of header) and \002 (start
> of text) is non-visible characters.
>
> In zsh we do this with `%F{color}`, but we could support
> `\001\e[31m\002` as well.

I like this patch but for a different reason. It closes the capability
gap in zsh prompts with and without prompt_percent.

When one disables prompt_percent, they can do all the same things as
before _except_ mark some sequences as zero-width. The loss of this
capability in turn means that one cannot use colors (and many other
things) in prompt without prompt_percent.

With this patch we'll have a nice guarantee that the following two
pieces of code are equivalent:

A:

    emulate -R zsh
    PS1=$whatever

B:

    emulate -R zsh -o no_prompt_percent
    PS1=${(%)whatever}

(Assuming no other prompt parameters are set.)

No real-world use cases come to mind but it just looks simple and
elegant from the user's point of view.

Roman.


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 14:37 ` Roman Perepelitsa
@ 2022-08-11 15:21   ` Bart Schaefer
  2022-08-11 16:20     ` Felipe Contreras
  2022-08-11 17:24   ` Roman Perepelitsa
  1 sibling, 1 reply; 21+ messages in thread
From: Bart Schaefer @ 2022-08-11 15:21 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Felipe Contreras, Zsh hackers list, Oliver Kiddle, dana

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

On Thu, Aug 11, 2022, 9:38 AM Roman Perepelitsa <roman.perepelitsa@gmail.com>
wrote:

>
> I like this patch but for a different reason. It closes the capability
> gap in zsh prompts with and without prompt_percent.


I've been traveling this week without access to the source code so haven't
otherwise commented on this, but:

1. Roman's point above is valid.

2. As previously mentioned by others, this is related to %{ %} rather than
to %F, and should be implemented and documented as such.  One cannot write
$'\001red\002' (I presume) and it would not be generally useful in that
context.

3. Escapes for ctrl-a and ctrl-b also work?  Or this is explicitly looking
for \001 \002 ?  Any possible conflicts?

[-- Attachment #2: Type: text/html, Size: 1271 bytes --]

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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 15:21   ` Bart Schaefer
@ 2022-08-11 16:20     ` Felipe Contreras
  2022-08-11 16:49       ` Bart Schaefer
  0 siblings, 1 reply; 21+ messages in thread
From: Felipe Contreras @ 2022-08-11 16:20 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Roman Perepelitsa, Zsh hackers list, Oliver Kiddle, dana

On Thu, Aug 11, 2022 at 10:21 AM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> On Thu, Aug 11, 2022, 9:38 AM Roman Perepelitsa <roman.perepelitsa@gmail.com> wrote:
>>
>> I like this patch but for a different reason. It closes the capability
>> gap in zsh prompts with and without prompt_percent.
>
> I've been traveling this week without access to the source code so haven't otherwise commented on this, but:
>
> 1. Roman's point above is valid.
>
> 2. As previously mentioned by others, this is related to %{ %} rather than to %F, and should be implemented and documented as such.  One cannot write $'\001red\002' (I presume) and it would not be generally useful in that context.

More related, yes, but %F does do the equivalent of %{%} internally (I
didn't know what %{%} did when I wrote the commit message).

For reference bash does have the equivalent of that with \[ \], but
that's specific to bash and it's converted to \001 \002 internally
which is what readline understands. A prompt function (like
__git_ps1()) cannot return \[ \].

> 3. Escapes for ctrl-a and ctrl-b also work?  Or this is explicitly looking for \001 \002 ?  Any possible conflicts?

^A ^B are another way to say \001 \002 is it not? Or \x01 \x02. At the
end of the day are the same values.

I don't know how to echo ^A in order to test though.

-- 
Felipe Contreras


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 16:20     ` Felipe Contreras
@ 2022-08-11 16:49       ` Bart Schaefer
  2022-08-11 18:48         ` Felipe Contreras
  0 siblings, 1 reply; 21+ messages in thread
From: Bart Schaefer @ 2022-08-11 16:49 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: Roman Perepelitsa, Zsh hackers list, Oliver Kiddle, dana

[-- Attachment #1: Type: text/plain, Size: 494 bytes --]

On Thu, Aug 11, 2022, 11:20 AM Felipe Contreras <felipe.contreras@gmail.com>
wrote:

>
> For reference bash does have the equivalent of that with \[ \], but
> that's specific to bash and it's converted to \001 \002 internally
> which is what readline understands. A prompt function (like
> __git_ps1()) cannot return \[ \].



In that case we should document this as being for readline compatibility
rather than bash prompt compatibility.  Forestalls more of the slippery
slope arguments, too.

[-- Attachment #2: Type: text/html, Size: 880 bytes --]

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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 14:37 ` Roman Perepelitsa
  2022-08-11 15:21   ` Bart Schaefer
@ 2022-08-11 17:24   ` Roman Perepelitsa
  2022-08-11 18:55     ` Roman Perepelitsa
  2022-08-11 19:23     ` Felipe Contreras
  1 sibling, 2 replies; 21+ messages in thread
From: Roman Perepelitsa @ 2022-08-11 17:24 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers, Oliver Kiddle, dana

On Thu, Aug 11, 2022 at 4:37 PM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> I like this patch [...]

I should've said that I like the idea behind it. The patch implements
one part of it. To complete the implementation percent expansion needs
to output \001 and \002.

> No real-world use cases come to mind [...]

Here's one. If this is implemented, it'll be possible to compute the
width of prompt in columns with ${(m)#${(%%)PS1}} (perform prompt
expansion and then ask for width). This is necessary in prompts that
look like this:

    left------------------------right
    bottom%

The first line spans the whole width of the terminal. One example of
such prompt is `prompt bart` from `promptinit`. This particular prompt
can compute the width of `left` and `righ` by utilizing the knowledge
of their content. If these pieces could be specified by the end-user,
the problem would be more difficult. I've described one (rather
cumbersome) solution here:
https://www.reddit.com/r/zsh/comments/cgbm24/multiline_prompt_the_missing_ingredient/.

Roman.


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 14:26       ` Felipe Contreras
@ 2022-08-11 18:32         ` Mikael Magnusson
  2022-08-11 18:47           ` Felipe Contreras
  0 siblings, 1 reply; 21+ messages in thread
From: Mikael Magnusson @ 2022-08-11 18:32 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers

On 8/11/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> On Thu, Aug 11, 2022 at 9:02 AM Mikael Magnusson <mikachu@gmail.com> wrote:
>> On 8/10/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> > On Wed, Aug 10, 2022 at 1:59 PM Mikael Magnusson <mikachu@gmail.com>
>> > wrote:
>
>> >> The commit message seems a bit confused, %F has nothing to do with
>> >> marking characters as 0-width, rather everything between %{ and %}
>> >> will be treated as such. You can use %{%} (or \001
>> >> \002) to set
>> >> colors via the specific \e [ Ps m code, but also to send any other
>> >> codes handled by the terminal.
>> >
>> > If I do PS1='%F{red}foo' putpromptchar() will call
>> > set_colour_attribute(), which eventually calls this:
>> >
>> >   if (!bv->dontcount) {
>> >       addbufspc(1);
>> >       *bv->bp++ = Inpar;
>> >   }
>> >   tputs(tgoto(tcstr[tc], colour, colour), 1, putstr);
>> >   if (!bv->dontcount) {
>> >       addbufspc(1);
>> >       *bv->bp++ = Outpar;
>> >   }
>> >
>> > I can do the same thing fputs() is doing with PS1=$'\e[31mfoo', but
>> > now zsh will think my prompt is bigger than it actually is and the
>> > shell will be screwed. So I have to put that inside %{%}.
>> >
>> > Therefore "%F{red}" = $'%{\e[31m%}'
>>
>> Sure, this agrees 100% with what I was saying, but the statement in
>> the commit still doesn't make sense. You've proven that you can use %{
>> or \001 to do what %F{} does, but the commit message states the
>> opposite which is not true.
>
> The commit message says:
>
> 1. We do readline $'\001\e[31m\002' as zsh '%F{red}'.
>
> This is true.

The statement is true, but it's not what the commit message says, it says:
"readline assumes anything between \001 (start of header) and \002 (start
of text) is non-visible characters.

In zsh we do this with `%F{color}`"

I don't know why you are arguing about this, there's nothing bad about
fixing an error in a commit message after someone points it out.

> 2. We could support $'\001\e[31m\002'.
>
> This is also true.

That's true.

-- 
Mikael Magnusson


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 18:32         ` Mikael Magnusson
@ 2022-08-11 18:47           ` Felipe Contreras
  2022-08-11 18:54             ` Mikael Magnusson
  0 siblings, 1 reply; 21+ messages in thread
From: Felipe Contreras @ 2022-08-11 18:47 UTC (permalink / raw)
  To: Mikael Magnusson; +Cc: zsh-workers

On Thu, Aug 11, 2022 at 1:32 PM Mikael Magnusson <mikachu@gmail.com> wrote:
> On 8/11/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:

> > The commit message says:
> >
> > 1. We do readline $'\001\e[31m\002' as zsh '%F{red}'.
> >
> > This is true.
>
> The statement is true, but it's not what the commit message says, it says:
> "readline assumes anything between \001 (start of header) and \002 (start
> of text) is non-visible characters.
>
> In zsh we do this with `%F{color}`"

It's the same: what we do in zsh with `%F{color}`, we do in readline
with $'\001\e[31m\002'.

> I don't know why you are arguing about this, there's nothing bad about
> fixing an error in a commit message after someone points it out.

I don't believe it's an error. You are the one that is arguing it's an error.

Regardless of what you choose to call it, it will be changed. I don't
need you to believe it's correct, and you don't need me to believe
it's an error.

The end result is the same: it will change.

-- 
Felipe Contreras


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 16:49       ` Bart Schaefer
@ 2022-08-11 18:48         ` Felipe Contreras
  0 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2022-08-11 18:48 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Roman Perepelitsa, Zsh hackers list, Oliver Kiddle, dana

On Thu, Aug 11, 2022 at 11:49 AM Bart Schaefer
<schaefer@brasslantern.com> wrote:
>
> On Thu, Aug 11, 2022, 11:20 AM Felipe Contreras <felipe.contreras@gmail.com> wrote:
>>
>>
>> For reference bash does have the equivalent of that with \[ \], but
>> that's specific to bash and it's converted to \001 \002 internally
>> which is what readline understands. A prompt function (like
>> __git_ps1()) cannot return \[ \].
>
> In that case we should document this as being for readline compatibility rather than bash prompt compatibility.  Forestalls more of the slippery slope arguments, too.

The commit message mentions readline, not bash.

-- 
Felipe Contreras


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 18:47           ` Felipe Contreras
@ 2022-08-11 18:54             ` Mikael Magnusson
  2022-08-11 19:46               ` Felipe Contreras
  0 siblings, 1 reply; 21+ messages in thread
From: Mikael Magnusson @ 2022-08-11 18:54 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers

On 8/11/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> On Thu, Aug 11, 2022 at 1:32 PM Mikael Magnusson <mikachu@gmail.com> wrote:
>> On 8/11/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>
>> > The commit message says:
>> >
>> > 1. We do readline $'\001\e[31m\002' as zsh '%F{red}'.
>> >
>> > This is true.
>>
>> The statement is true, but it's not what the commit message says, it
>> says:
>> "readline assumes anything between \001 (start of header) and \002 (start
>> of text) is non-visible characters.
>>
>> In zsh we do this with `%F{color}`"
>
> It's the same: what we do in zsh with `%F{color}`, we do in readline
> with $'\001\e[31m\002'.

It's clearly not the same and incorrect. You're basically just lying
at this point, please stop.

-- 
Mikael Magnusson


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 17:24   ` Roman Perepelitsa
@ 2022-08-11 18:55     ` Roman Perepelitsa
  2022-08-11 19:23     ` Felipe Contreras
  1 sibling, 0 replies; 21+ messages in thread
From: Roman Perepelitsa @ 2022-08-11 18:55 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers, Oliver Kiddle, dana

On Thu, Aug 11, 2022 at 7:24 PM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> On Thu, Aug 11, 2022 at 4:37 PM Roman Perepelitsa
> <roman.perepelitsa@gmail.com> wrote:
> >
> > I like this patch [...]
>
> I should've said that I like the idea behind it. The patch implements
> one part of it. To complete the implementation percent expansion needs
> to output \001 and \002.

I apologize for replying to my own message multiple times.

I just realized that what I suggested may not work. Consider this code:

    print -r -- ${(%):-'%{hello%}'}

Currently it is equivalent to this:

    print -r -- 'hello'

However, if percent expansion starts inserting \001 and \002, the same
code will be equivalent to this:

    print -r -- $'\001hello\002'

This may be undesirable.

The patch by Felipe, as it stands, doesn't have this problem.

Roman.


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 17:24   ` Roman Perepelitsa
  2022-08-11 18:55     ` Roman Perepelitsa
@ 2022-08-11 19:23     ` Felipe Contreras
  2022-08-11 19:35       ` Roman Perepelitsa
  1 sibling, 1 reply; 21+ messages in thread
From: Felipe Contreras @ 2022-08-11 19:23 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: zsh-workers, Oliver Kiddle, dana

On Thu, Aug 11, 2022 at 12:24 PM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> On Thu, Aug 11, 2022 at 4:37 PM Roman Perepelitsa
> <roman.perepelitsa@gmail.com> wrote:
> >
> > I like this patch [...]
>
> I should've said that I like the idea behind it. The patch implements
> one part of it. To complete the implementation percent expansion needs
> to output \001 and \002.

That's easy. The current code in prompt.c uses Inpar and Outpar, which
are 0x88 and 0x8A respectively. If instead we do:

#define invis_start ((char) 0x01)
#define invis_end   ((char) 0x02)

And replace Inpar/Outpar with invis_start/invis_end the code should
behave identically (since those characters are removed).

Then in promptexpand() we simply not remove them.

I'm not sure if leaking these characters is what we want though.

-- 
Felipe Contreras


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 19:23     ` Felipe Contreras
@ 2022-08-11 19:35       ` Roman Perepelitsa
  2022-08-11 19:44         ` Felipe Contreras
  0 siblings, 1 reply; 21+ messages in thread
From: Roman Perepelitsa @ 2022-08-11 19:35 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: zsh-workers, Oliver Kiddle, dana

On Thu, Aug 11, 2022 at 9:24 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Thu, Aug 11, 2022 at 12:24 PM Roman Perepelitsa
> <roman.perepelitsa@gmail.com> wrote:
> >
> > On Thu, Aug 11, 2022 at 4:37 PM Roman Perepelitsa
> > <roman.perepelitsa@gmail.com> wrote:
> > >
> > > I like this patch [...]
> >
> > I should've said that I like the idea behind it. The patch implements
> > one part of it. To complete the implementation percent expansion needs
> > to output \001 and \002.
>
> That's easy. The current code in prompt.c uses Inpar and Outpar, which
> are 0x88 and 0x8A respectively. If instead we do:
>
> #define invis_start ((char) 0x01)
> #define invis_end   ((char) 0x02)
>
> And replace Inpar/Outpar with invis_start/invis_end the code should
> behave identically (since those characters are removed).
>
> Then in promptexpand() we simply not remove them.

Let me clarify. Ideally we want (I think) for these two tests to pass:

1.

    [[ $(print -r -- ${(%):-'%{hello%}'}) == hello ]]

2.

    [[ $(print -r -- ${(m)#${(%):-'%F{1}❎%f'}}) == 2 ]]

The first test currently passes, the second fails. In my previous
comment I tried to say that making the second test pass will cause the
first test to fail.

Do you see how to make both of these tests pass?

Roman.


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 19:35       ` Roman Perepelitsa
@ 2022-08-11 19:44         ` Felipe Contreras
  0 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2022-08-11 19:44 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: zsh-workers, Oliver Kiddle, dana

On Thu, Aug 11, 2022 at 2:35 PM Roman Perepelitsa
<roman.perepelitsa@gmail.com> wrote:
>
> On Thu, Aug 11, 2022 at 9:24 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > On Thu, Aug 11, 2022 at 12:24 PM Roman Perepelitsa
> > <roman.perepelitsa@gmail.com> wrote:
> > >
> > > On Thu, Aug 11, 2022 at 4:37 PM Roman Perepelitsa
> > > <roman.perepelitsa@gmail.com> wrote:
> > > >
> > > > I like this patch [...]
> > >
> > > I should've said that I like the idea behind it. The patch implements
> > > one part of it. To complete the implementation percent expansion needs
> > > to output \001 and \002.
> >
> > That's easy. The current code in prompt.c uses Inpar and Outpar, which
> > are 0x88 and 0x8A respectively. If instead we do:
> >
> > #define invis_start ((char) 0x01)
> > #define invis_end   ((char) 0x02)
> >
> > And replace Inpar/Outpar with invis_start/invis_end the code should
> > behave identically (since those characters are removed).
> >
> > Then in promptexpand() we simply not remove them.
>
> Let me clarify. Ideally we want (I think) for these two tests to pass:
>
> 1.
>
>     [[ $(print -r -- ${(%):-'%{hello%}'}) == hello ]]
>
> 2.
>
>     [[ $(print -r -- ${(m)#${(%):-'%F{1}❎%f'}}) == 2 ]]
>
> The first test currently passes, the second fails. In my previous
> comment I tried to say that making the second test pass will cause the
> first test to fail.
>
> Do you see how to make both of these tests pass?

We could add a mode to the % flag which is passed to promptexpand() in
order to tell it to not remove the invisible start/end markers.

However, in my tests this simply adds two characters to $((m)#...}. I
don't see how this would return the correct amount of visible
characters.

-- 
Felipe Contreras


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 18:54             ` Mikael Magnusson
@ 2022-08-11 19:46               ` Felipe Contreras
  2022-08-11 20:38                 ` Bart Schaefer
  0 siblings, 1 reply; 21+ messages in thread
From: Felipe Contreras @ 2022-08-11 19:46 UTC (permalink / raw)
  To: Mikael Magnusson; +Cc: zsh-workers

On Thu, Aug 11, 2022 at 1:54 PM Mikael Magnusson <mikachu@gmail.com> wrote:
> On 8/11/22, Felipe Contreras <felipe.contreras@gmail.com> wrote:
> > On Thu, Aug 11, 2022 at 1:32 PM Mikael Magnusson <mikachu@gmail.com> wrote:

> >> In zsh we do this with `%F{color}`"
> >
> > It's the same: what we do in zsh with `%F{color}`, we do in readline
> > with $'\001\e[31m\002'.
>
> It's clearly not the same and incorrect. You're basically just lying
> at this point, please stop.

This is my sincere belief. You are free to not believe it is.

-- 
Felipe Contreras


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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 19:46               ` Felipe Contreras
@ 2022-08-11 20:38                 ` Bart Schaefer
  2022-08-11 23:22                   ` Felipe Contreras
  0 siblings, 1 reply; 21+ messages in thread
From: Bart Schaefer @ 2022-08-11 20:38 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: Mikael Magnusson, Zsh hackers list

[-- Attachment #1: Type: text/plain, Size: 531 bytes --]

On Thu, Aug 11, 2022, 2:47 PM Felipe Contreras <felipe.contreras@gmail.com>
wrote:

>
> > > It's the same: what we do in zsh with `%F{color}`, we do in readline
> > > with $'\001\e[31m\002'.
>
> This is my sincere belief. You are free to not believe it is.
>

This is getting a bit silly, but:

While the above is generically true, given "color" is a placeholder for a
set of possible escapes of which "31m" is a specific example, it is not
true that $'\001anyrandomstring\002' in readline is accomplished via
%F{anything} in zsh.

[-- Attachment #2: Type: text/html, Size: 965 bytes --]

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

* Re: [PATCH] prompt: support generic non-visible regions
  2022-08-11 20:38                 ` Bart Schaefer
@ 2022-08-11 23:22                   ` Felipe Contreras
  0 siblings, 0 replies; 21+ messages in thread
From: Felipe Contreras @ 2022-08-11 23:22 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: Mikael Magnusson, Zsh hackers list

On Thu, Aug 11, 2022 at 3:38 PM Bart Schaefer <schaefer@brasslantern.com> wrote:
>
> On Thu, Aug 11, 2022, 2:47 PM Felipe Contreras <felipe.contreras@gmail.com> wrote:
>>
>> > > It's the same: what we do in zsh with `%F{color}`, we do in readline
>> > > with $'\001\e[31m\002'.
>>
>> This is my sincere belief. You are free to not believe it is.
>
>
> This is getting a bit silly, but:
>
> While the above is generically true, given "color" is a placeholder for a set of possible escapes of which "31m" is a specific example, it is not true that $'\001anyrandomstring\002' in readline is accomplished via %F{anything} in zsh.

Fine, if you really want to be punctilious: "%F{red}" =>
"\001\e[31m\002", and "%F{$color_name}" =>
"\001\e[${color_code}m\002".

I don't see what this has to do with the patch.

Anyway, since I haven't seen any comments regarding the actual code of
the patch I've resent it with the updated commit message.

-- 
Felipe Contreras


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

end of thread, other threads:[~2022-08-11 23:28 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-10 11:51 [PATCH] prompt: support generic non-visible regions Felipe Contreras
2022-08-10 18:59 ` Mikael Magnusson
2022-08-10 19:56   ` Felipe Contreras
2022-08-11 14:02     ` Mikael Magnusson
2022-08-11 14:26       ` Felipe Contreras
2022-08-11 18:32         ` Mikael Magnusson
2022-08-11 18:47           ` Felipe Contreras
2022-08-11 18:54             ` Mikael Magnusson
2022-08-11 19:46               ` Felipe Contreras
2022-08-11 20:38                 ` Bart Schaefer
2022-08-11 23:22                   ` Felipe Contreras
2022-08-11 14:37 ` Roman Perepelitsa
2022-08-11 15:21   ` Bart Schaefer
2022-08-11 16:20     ` Felipe Contreras
2022-08-11 16:49       ` Bart Schaefer
2022-08-11 18:48         ` Felipe Contreras
2022-08-11 17:24   ` Roman Perepelitsa
2022-08-11 18:55     ` Roman Perepelitsa
2022-08-11 19:23     ` Felipe Contreras
2022-08-11 19:35       ` Roman Perepelitsa
2022-08-11 19:44         ` 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).