zsh-workers
 help / color / mirror / Atom feed
* [PATCH 1/2] promptinit: typo: RPOMPT -> RPROMPT
@ 2021-01-25  8:16 Arseny Maslennikov
  2021-01-25  8:16 ` [PATCH 2/2] promptinit: Fix prompt cleanups Arseny Maslennikov
  0 siblings, 1 reply; 11+ messages in thread
From: Arseny Maslennikov @ 2021-01-25  8:16 UTC (permalink / raw)
  To: zsh-workers; +Cc: Arseny Maslennikov

---
 Functions/Prompts/promptinit | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Functions/Prompts/promptinit b/Functions/Prompts/promptinit
index e27b8779a..5e42ebdd3 100644
--- a/Functions/Prompts/promptinit
+++ b/Functions/Prompts/promptinit
@@ -49,7 +49,7 @@ prompt_preview_safely() {
 
   # This handles all the stuff from the default :prompt-theme cleanup
   local +h PS1=$PS1 PS2=$PS2 PS3=$PS3 PS4=$PS4 RPS1=$RPS1 RPS2=$RPS2
-  local +h PROMPT=$PROMPT RPROMPT=$RPOMPT RPROMPT2=$RPROMPT2 PSVAR=$PSVAR
+  local +h PROMPT=$PROMPT RPROMPT=$RPROMPT RPROMPT2=$RPROMPT2 PSVAR=$PSVAR
   local -a precmd_functions preexec_functions prompt_preview_cleanup
   local -aLl +h zle_highlight
 
@@ -101,7 +101,7 @@ Use prompt -h <theme> for help on specific themes.'
       if [[ -z "$prompt_theme[1]" ]]; then
         # Not using a prompt theme; save settings
         local +h PS1=$PS1 PS2=$PS2 PS3=$PS3 PS4=$PS4 RPS1=$RPS1 RPS2=$RPS2
-        local +h PROMPT=$PROMPT RPROMPT=$RPOMPT RPROMPT2=$RPROMPT2 PSVAR=$PSVAR
+        local +h PROMPT=$PROMPT RPROMPT=$RPROMPT RPROMPT2=$RPROMPT2 PSVAR=$PSVAR
         local -a precmd_functions preexec_functions
       else
         trap 'prompt_${prompt_theme[1]}_setup "${(@)prompt_theme[2,-1]}"' 0
-- 
2.30.0



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

* [PATCH 2/2] promptinit: Fix prompt cleanups
  2021-01-25  8:16 [PATCH 1/2] promptinit: typo: RPOMPT -> RPROMPT Arseny Maslennikov
@ 2021-01-25  8:16 ` Arseny Maslennikov
  2021-01-25  8:23   ` Roman Perepelitsa
                     ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Arseny Maslennikov @ 2021-01-25  8:16 UTC (permalink / raw)
  To: zsh-workers; +Cc: Arseny Maslennikov

The Zsh documentation at Doc/Zsh/contrib, paragraph 26.6.4 "Writing
Themes", says:

Declare cleanup

    If your function makes any other changes that should be undone when
    the theme is disabled, your setup function may call

    prompt_cleanup command

    where command should be suitably quoted. If your theme is ever
    disabled or replaced by another, command is executed with eval. You
    may declare more than one such cleanup hook.

The promptinit framework fails to apply cleanup commands of the current
theme on any theme change other than "prompt restore", as well as when
invoking setup functions of a certain $theme as an implementation detail
of `prompt -[hp] $theme'.

This problem was previously brought up in zsh-users/23314, to no avail.

We fix it in the following way, hopefully without breaking compatibility:
* Rename zstyle `cleanup' on the context `:prompt-theme' to `restore'
  everywhere but in prompt_cleanup(). It is only used as a restore
  mechanism now.
* Ensure prompt_cleanup() continues to store its command list in the
  `cleanup' style.
* Clean up before theme switch at the end of set_prompt().
* Prepend every use of prompt_*_setup (which might modify the shell
  state in ways that require cleanup) with a cleanup run.
* Adjust `prompt restore' to do both parts of the newly split restore
  mechanism, cleanup first.
---
 Functions/Prompts/prompt_restore_setup |  1 +
 Functions/Prompts/promptinit           | 43 ++++++++++++++++++--------
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/Functions/Prompts/prompt_restore_setup b/Functions/Prompts/prompt_restore_setup
index 54c4adbf9..b77dbe815 100644
--- a/Functions/Prompts/prompt_restore_setup
+++ b/Functions/Prompts/prompt_restore_setup
@@ -1,2 +1,3 @@
 # Damn that was easy
 zstyle -t :prompt-theme cleanup
+zstyle -t :prompt-theme restore
diff --git a/Functions/Prompts/promptinit b/Functions/Prompts/promptinit
index 5e42ebdd3..1c6d27ad7 100644
--- a/Functions/Prompts/promptinit
+++ b/Functions/Prompts/promptinit
@@ -47,17 +47,19 @@ prompt_preview_safely() {
     return
   fi
 
-  # This handles all the stuff from the default :prompt-theme cleanup
+  # This handles all the stuff from the default :prompt-theme restore
   local +h PS1=$PS1 PS2=$PS2 PS3=$PS3 PS4=$PS4 RPS1=$RPS1 RPS2=$RPS2
   local +h PROMPT=$PROMPT RPROMPT=$RPROMPT RPROMPT2=$RPROMPT2 PSVAR=$PSVAR
-  local -a precmd_functions preexec_functions prompt_preview_cleanup
+  local -a precmd_functions preexec_functions prompt_preview_restore
   local -aLl +h zle_highlight
 
   {
     # Save and clear current restore-point if any
-    zstyle -g prompt_preview_cleanup :prompt-theme cleanup
+    zstyle -g prompt_preview_restore :prompt-theme restore
     {
-      zstyle -d :prompt-theme cleanup
+      zstyle -d :prompt-theme restore
+      # Execute current cleanup sequence, if any.
+      zstyle -t :prompt-theme cleanup
 
       # The next line is a bit ugly.  It (perhaps unnecessarily)
       # runs the prompt theme setup function to ensure that if
@@ -74,8 +76,8 @@ prompt_preview_safely() {
       zstyle -t :prompt-theme cleanup
     }
   } always {
-    (( $#prompt_preview_cleanup )) &&
-      zstyle -e :prompt-theme cleanup "${prompt_preview_cleanup[@]}"
+    (( $#prompt_preview_restore )) &&
+      zstyle -e :prompt-theme restore "${prompt_preview_restore[@]}"
   }
 }
 
@@ -104,7 +106,7 @@ Use prompt -h <theme> for help on specific themes.'
         local +h PROMPT=$PROMPT RPROMPT=$RPROMPT RPROMPT2=$RPROMPT2 PSVAR=$PSVAR
         local -a precmd_functions preexec_functions
       else
-        trap 'prompt_${prompt_theme[1]}_setup "${(@)prompt_theme[2,-1]}"' 0
+        trap 'zstyle -t :prompt-theme cleanup; prompt_${prompt_theme[1]}_setup "${(@)prompt_theme[2,-1]}"' 0
       fi
       ;;
   esac
@@ -123,6 +125,7 @@ Use prompt -h <theme> for help on specific themes.'
            # The next line is a bit ugly.  It (perhaps unnecessarily)
            # runs the prompt theme setup function to ensure that if
            # the theme has a _help function that it's been autoloaded.
+           zstyle -t :prompt-theme cleanup
            prompt_$2_setup
          fi
          if functions prompt_$2_help >/dev/null; then
@@ -179,28 +182,41 @@ Use prompt -h <theme> for help on specific themes.'
        typeset -ga zle_highlight=( ${zle_highlight:#default:*} )
        (( ${#zle_highlight} )) || unset zle_highlight
 
+       zstyle -t :prompt-theme cleanup
        prompt_$1_setup "$@[2,-1]" && prompt_theme=( "$@" )
        ;;
   esac
 }
 
 prompt_cleanup () {
-  local -a cleanup_hooks
+  local -a cleanup_hooks theme_active
   if zstyle -g cleanup_hooks :prompt-theme cleanup
   then
     cleanup_hooks+=(';' "$@")
     zstyle -e :prompt-theme cleanup "${cleanup_hooks[@]}"
-  elif (( $+prompt_preview_cleanup == 0 ))
+    return 0
+  fi
+
+  (( $+prompt_preview_restore == 0 )) &&
+  if ! zstyle -g theme_active :prompt-theme restore
   then
     print -u2 "prompt_cleanup: no prompt theme active"
     return 1
   fi
+
+  # Set the cleanup sequence up.
+  zstyle -e :prompt-theme cleanup \
+      'zstyle -d :prompt-theme cleanup;' \
+      'reply=(yes)'
+
+  # Will add the hook this time.
+  prompt_cleanup "$@"
 }
 
 prompt () {
   local -a prompt_opts theme_active
 
-  zstyle -g theme_active :prompt-theme cleanup || {
+  zstyle -g theme_active :prompt-theme restore || {
     # This is done here rather than in set_prompt so that it
     # is safe and sane for set_prompt to setopt localoptions,
     # which will be cleared before we arrive back here again.
@@ -210,8 +226,8 @@ prompt () {
     [[ -o promptpercent ]] && prompt_opts+=(percent)
     [[ -o promptsp ]] && prompt_opts+=(sp)
     [[ -o promptsubst ]] && prompt_opts+=(subst)
-    zstyle -e :prompt-theme cleanup \
-        'zstyle -d :prompt-theme cleanup;' \
+    zstyle -e :prompt-theme restore \
+        'zstyle -d :prompt-theme restore;' \
 	'prompt_default_setup;' \
         ${PS1+PS1="${(q)PS1}"} \
         ${PS2+PS2="${(q)PS2}"} \
@@ -239,7 +255,7 @@ prompt_preview_theme () {
   emulate -L zsh
 
   # Check for proper state handling
-  (( $+prompt_preview_cleanup )) || {
+  (( $+prompt_preview_restore )) || {
     prompt_preview_safely "$@"
     return
   }
@@ -249,6 +265,7 @@ prompt_preview_theme () {
   print -n "$1 theme"
   (( $#* > 1 )) && print -n " with parameters \`$*[2,-1]'"
   print ":"
+  zstyle -t :prompt-theme cleanup
   prompt_${1}_setup "$@[2,-1]"
   (( ${#prompt_opts} )) &&
       setopt noprompt{bang,cr,percent,sp,subst} "prompt${^prompt_opts[@]}"
-- 
2.30.0



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

* Re: [PATCH 2/2] promptinit: Fix prompt cleanups
  2021-01-25  8:16 ` [PATCH 2/2] promptinit: Fix prompt cleanups Arseny Maslennikov
@ 2021-01-25  8:23   ` Roman Perepelitsa
  2021-01-25 12:50     ` Arseny Maslennikov
  2021-02-18 13:03   ` (ping) " Arseny Maslennikov
  2021-02-22 18:29   ` Bart Schaefer
  2 siblings, 1 reply; 11+ messages in thread
From: Roman Perepelitsa @ 2021-01-25  8:23 UTC (permalink / raw)
  To: Arseny Maslennikov; +Cc: Zsh hackers list

On Mon, Jan 25, 2021 at 9:17 AM Arseny Maslennikov <ar@cs.msu.ru> wrote:
>
> This problem was previously brought up in zsh-users/23314, to no avail.

Also related: zsh-workers/46202.

Roman.


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

* Re: [PATCH 2/2] promptinit: Fix prompt cleanups
  2021-01-25  8:23   ` Roman Perepelitsa
@ 2021-01-25 12:50     ` Arseny Maslennikov
  0 siblings, 0 replies; 11+ messages in thread
From: Arseny Maslennikov @ 2021-01-25 12:50 UTC (permalink / raw)
  To: Roman Perepelitsa; +Cc: Zsh hackers list

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

On Mon, Jan 25, 2021 at 09:23:32AM +0100, Roman Perepelitsa wrote:
> On Mon, Jan 25, 2021 at 9:17 AM Arseny Maslennikov <ar@cs.msu.ru> wrote:
> >
> > This problem was previously brought up in zsh-users/23314, to no avail.
> 
> Also related: zsh-workers/46202.

Thank you for bringing this up!

However, during working on the patch I found out this is not the only
problem with prompt theme cleanups. prompt_$theme_setup works both as an
autoloaded function to pull the theme components in and as the setup
function, so `prompt -h $theme' and `prompt -p $theme' both _activate_ the
$theme and fail to run the cleanup actions.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* (ping) Re: [PATCH 2/2] promptinit: Fix prompt cleanups
  2021-01-25  8:16 ` [PATCH 2/2] promptinit: Fix prompt cleanups Arseny Maslennikov
  2021-01-25  8:23   ` Roman Perepelitsa
@ 2021-02-18 13:03   ` Arseny Maslennikov
  2021-02-20  0:50     ` dana
  2021-02-22 18:29   ` Bart Schaefer
  2 siblings, 1 reply; 11+ messages in thread
From: Arseny Maslennikov @ 2021-02-18 13:03 UTC (permalink / raw)
  To: zsh-workers

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

On Mon, Jan 25, 2021 at 11:16:25AM +0300, Arseny Maslennikov wrote:
> The Zsh documentation at Doc/Zsh/contrib, paragraph 26.6.4 "Writing
> Themes", says:
> 
> Declare cleanup
> 
>     If your function makes any other changes that should be undone when
>     the theme is disabled, your setup function may call
> 
>     prompt_cleanup command
> 
>     where command should be suitably quoted. If your theme is ever
>     disabled or replaced by another, command is executed with eval. You
>     may declare more than one such cleanup hook.
> 
> The promptinit framework fails to apply cleanup commands of the current
> theme on any theme change other than "prompt restore", as well as when
> invoking setup functions of a certain $theme as an implementation detail
> of `prompt -[hp] $theme'.
> 
> This problem was previously brought up in zsh-users/23314, to no avail.
> 
> We fix it in the following way, hopefully without breaking compatibility:
> * Rename zstyle `cleanup' on the context `:prompt-theme' to `restore'
>   everywhere but in prompt_cleanup(). It is only used as a restore
>   mechanism now.
> * Ensure prompt_cleanup() continues to store its command list in the
>   `cleanup' style.
> * Clean up before theme switch at the end of set_prompt().
> * Prepend every use of prompt_*_setup (which might modify the shell
>   state in ways that require cleanup) with a cleanup run.
> * Adjust `prompt restore' to do both parts of the newly split restore
>   mechanism, cleanup first.
> ---
>  Functions/Prompts/prompt_restore_setup |  1 +
>  Functions/Prompts/promptinit           | 43 ++++++++++++++++++--------
>  2 files changed, 31 insertions(+), 13 deletions(-)

Could someone please look at this series?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: (ping) Re: [PATCH 2/2] promptinit: Fix prompt cleanups
  2021-02-18 13:03   ` (ping) " Arseny Maslennikov
@ 2021-02-20  0:50     ` dana
  2021-02-20 19:01       ` Bart Schaefer
  0 siblings, 1 reply; 11+ messages in thread
From: dana @ 2021-02-20  0:50 UTC (permalink / raw)
  To: Arseny Maslennikov; +Cc: Zsh hackers list

On 18 Feb 2021, at 07:03, Arseny Maslennikov <ar@cs.msu.ru> wrote:
> Could someone please look at this series?

I've committed the typo fix since it was so obvious, but i'm not very familiar
with promptinit and don't have much time to study it at the moment, so i'll
have to leave the other patch to someone else for now. Sorry about that

dana



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

* Re: (ping) Re: [PATCH 2/2] promptinit: Fix prompt cleanups
  2021-02-20  0:50     ` dana
@ 2021-02-20 19:01       ` Bart Schaefer
  0 siblings, 0 replies; 11+ messages in thread
From: Bart Schaefer @ 2021-02-20 19:01 UTC (permalink / raw)
  To: dana; +Cc: Arseny Maslennikov, Zsh hackers list

On Fri, Feb 19, 2021 at 4:50 PM dana <dana@dana.is> wrote:
>
> On 18 Feb 2021, at 07:03, Arseny Maslennikov <ar@cs.msu.ru> wrote:
> > Could someone please look at this series?
>
> I've committed the typo fix since it was so obvious, but i'm not very familiar
> with promptinit and don't have much time to study it at the moment

I'll try to get a better look at it this weekend.


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

* Re: [PATCH 2/2] promptinit: Fix prompt cleanups
  2021-01-25  8:16 ` [PATCH 2/2] promptinit: Fix prompt cleanups Arseny Maslennikov
  2021-01-25  8:23   ` Roman Perepelitsa
  2021-02-18 13:03   ` (ping) " Arseny Maslennikov
@ 2021-02-22 18:29   ` Bart Schaefer
  2021-02-22 22:24     ` Arseny Maslennikov
  2021-02-22 22:28     ` [PATCH v2] " Arseny Maslennikov
  2 siblings, 2 replies; 11+ messages in thread
From: Bart Schaefer @ 2021-02-22 18:29 UTC (permalink / raw)
  To: Arseny Maslennikov; +Cc: zsh-workers

On Mon, Jan 25, 2021 at 12:17 AM Arseny Maslennikov <ar@cs.msu.ru> wrote:
>
> The promptinit framework fails to apply cleanup commands of the current
> theme on any theme change other than "prompt restore", as well as when
> invoking setup functions of a certain $theme as an implementation detail
> of `prompt -[hp] $theme'.

Thanks, I did have a chance to look at this.

> We fix it in the following way, hopefully without breaking compatibility:
> * Rename zstyle `cleanup' on the context `:prompt-theme' to `restore'
>   everywhere but in prompt_cleanup(). It is only used as a restore
>   mechanism now.
> * Ensure prompt_cleanup() continues to store its command list in the
>   `cleanup' style.
> * Clean up before theme switch at the end of set_prompt().

These all seem fine, conceptually.

> * Prepend every use of prompt_*_setup (which might modify the shell
>   state in ways that require cleanup) with a cleanup run.

Have you confirmed that this is a no-op when there has not been a
previous installation of a theme?

> * Adjust `prompt restore' to do both parts of the newly split restore
>   mechanism, cleanup first.

Also fine.  Just a couple of nit-picky things about the diff itself:

> diff --git a/Functions/Prompts/promptinit b/Functions/Prompts/promptinit
> index 5e42ebdd3..1c6d27ad7 100644
> --- a/Functions/Prompts/promptinit
> +++ b/Functions/Prompts/promptinit
> @@ -123,6 +125,7 @@ Use prompt -h <theme> for help on specific themes.'
>             # The next line is a bit ugly.  It (perhaps unnecessarily)
>             # runs the prompt theme setup function to ensure that if
>             # the theme has a _help function that it's been autoloaded.
> +           zstyle -t :prompt-theme cleanup
>             prompt_$2_setup
>           fi
>           if functions prompt_$2_help >/dev/null; then

Placement of the added zstyle call makes the comment incorrect, move
it above the comment?

> @@ -179,28 +182,41 @@ Use prompt -h <theme> for help on specific themes.'
> +  # Will add the hook this time.
> +  prompt_cleanup "$@"

This is logically correct, but I don't think it adds much in terms of
maintainability to make this a recursive call; there are only 2
commands executed in that event.


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

* Re: [PATCH 2/2] promptinit: Fix prompt cleanups
  2021-02-22 18:29   ` Bart Schaefer
@ 2021-02-22 22:24     ` Arseny Maslennikov
  2021-02-22 22:28     ` [PATCH v2] " Arseny Maslennikov
  1 sibling, 0 replies; 11+ messages in thread
From: Arseny Maslennikov @ 2021-02-22 22:24 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers

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

On Mon, Feb 22, 2021 at 10:29:52AM -0800, Bart Schaefer wrote:
> On Mon, Jan 25, 2021 at 12:17 AM Arseny Maslennikov <ar@cs.msu.ru> wrote:
> >
> > The promptinit framework fails to apply cleanup commands of the current
> > theme on any theme change other than "prompt restore", as well as when
> > invoking setup functions of a certain $theme as an implementation detail
> > of `prompt -[hp] $theme'.
> 
> Thanks, I did have a chance to look at this.
> 
> > We fix it in the following way, hopefully without breaking compatibility:
> > * Rename zstyle `cleanup' on the context `:prompt-theme' to `restore'
> >   everywhere but in prompt_cleanup(). It is only used as a restore
> >   mechanism now.
> > * Ensure prompt_cleanup() continues to store its command list in the
> >   `cleanup' style.
> > * Clean up before theme switch at the end of set_prompt().
> 
> These all seem fine, conceptually.
> 
> > * Prepend every use of prompt_*_setup (which might modify the shell
> >   state in ways that require cleanup) with a cleanup run.
> 
> Have you confirmed that this is a no-op when there has not been a
> previous installation of a theme?

I double-checked right now; in that case, with the patch applied as
posted, it is a no-op when the command 'prompt -p' is invoked, but not
when 'prompt -h' is — the trap does not run. Thanks for the good catch!

The following diff on top of my patch fixes it:

--->8---

diff --git a/Functions/Prompts/promptinit b/Functions/Prompts/promptinit
index ab2b50857..5c7dcc5a1 100644
--- a/Functions/Prompts/promptinit
+++ b/Functions/Prompts/promptinit
@@ -105,9 +105,11 @@ Use prompt -h <theme> for help on specific themes.'
         local +h PS1=$PS1 PS2=$PS2 PS3=$PS3 PS4=$PS4 RPS1=$RPS1 RPS2=$RPS2
         local +h PROMPT=$PROMPT RPROMPT=$RPROMPT RPROMPT2=$RPROMPT2 PSVAR=$PSVAR
         local -a precmd_functions preexec_functions
+        local theme_reset=''
       else
-        trap 'zstyle -t :prompt-theme cleanup; prompt_${prompt_theme[1]}_setup "${(@)prompt_theme[2,-1]}"' 0
+        local theme_reset='prompt_${prompt_theme[1]}_setup "${(@)prompt_theme[2,-1]}"'
       fi
+      trap 'zstyle -t :prompt-theme cleanup;'"${theme_reset:+ $theme_reset}" 0
       ;;
   esac
   case "$opt" in

--------

> 
> > * Adjust `prompt restore' to do both parts of the newly split restore
> >   mechanism, cleanup first.
> 
> Also fine.  Just a couple of nit-picky things about the diff itself:
> 
> > diff --git a/Functions/Prompts/promptinit b/Functions/Prompts/promptinit
> > index 5e42ebdd3..1c6d27ad7 100644
> > --- a/Functions/Prompts/promptinit
> > +++ b/Functions/Prompts/promptinit
> > @@ -123,6 +125,7 @@ Use prompt -h <theme> for help on specific themes.'
> >             # The next line is a bit ugly.  It (perhaps unnecessarily)
> >             # runs the prompt theme setup function to ensure that if
> >             # the theme has a _help function that it's been autoloaded.
> > +           zstyle -t :prompt-theme cleanup
> >             prompt_$2_setup
> >           fi
> >           if functions prompt_$2_help >/dev/null; then
> 
> Placement of the added zstyle call makes the comment incorrect, move
> it above the comment?

Will do.

> 
> > @@ -179,28 +182,41 @@ Use prompt -h <theme> for help on specific themes.'
> > +  # Will add the hook this time.
> > +  prompt_cleanup "$@"
> 
> This is logically correct, but I don't think it adds much in terms of
> maintainability to make this a recursive call; there are only 2
> commands executed in that event.

Ok; I'll have to refactor this again. :)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* [PATCH v2] promptinit: Fix prompt cleanups
  2021-02-22 18:29   ` Bart Schaefer
  2021-02-22 22:24     ` Arseny Maslennikov
@ 2021-02-22 22:28     ` Arseny Maslennikov
  2021-02-22 23:50       ` Bart Schaefer
  1 sibling, 1 reply; 11+ messages in thread
From: Arseny Maslennikov @ 2021-02-22 22:28 UTC (permalink / raw)
  To: zsh-workers; +Cc: Arseny Maslennikov

The Zsh documentation at Doc/Zsh/contrib, paragraph 26.6.4 "Writing
Themes", says:

Declare cleanup

    If your function makes any other changes that should be undone when
    the theme is disabled, your setup function may call

    prompt_cleanup command

    where command should be suitably quoted. If your theme is ever
    disabled or replaced by another, command is executed with eval. You
    may declare more than one such cleanup hook.

The promptinit framework fails to apply cleanup commands of the current
theme on any theme change other than "prompt restore", as well as when
invoking setup functions of a certain $theme as an implementation detail
of `prompt -[hp] $theme'.

This problem was previously brought up in zsh-users/23314, to no avail.

We fix it in the following way, hopefully without breaking compatibility:
* Rename zstyle `cleanup' on the context `:prompt-theme' to `restore'
  everywhere but in prompt_cleanup(). It is only used as a restore
  mechanism now.
* Ensure prompt_cleanup() continues to store its command list in the
  `cleanup' style.
* Clean up before theme switch at the end of set_prompt().
* Prepend every use of prompt_*_setup (which might modify the shell
  state in ways that require cleanup) with a cleanup run.
* Adjust `prompt restore' to do both parts of the newly split restore
  mechanism, cleanup first.
---

Changes since v1:
* fix the case when no theme was previously activated
* small refactorings
---

 Functions/Prompts/prompt_restore_setup |  1 +
 Functions/Prompts/promptinit           | 54 +++++++++++++++++---------
 2 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/Functions/Prompts/prompt_restore_setup b/Functions/Prompts/prompt_restore_setup
index 54c4adbf9..b77dbe815 100644
--- a/Functions/Prompts/prompt_restore_setup
+++ b/Functions/Prompts/prompt_restore_setup
@@ -1,2 +1,3 @@
 # Damn that was easy
 zstyle -t :prompt-theme cleanup
+zstyle -t :prompt-theme restore
diff --git a/Functions/Prompts/promptinit b/Functions/Prompts/promptinit
index 5e42ebdd3..37d69f100 100644
--- a/Functions/Prompts/promptinit
+++ b/Functions/Prompts/promptinit
@@ -47,17 +47,19 @@ prompt_preview_safely() {
     return
   fi
 
-  # This handles all the stuff from the default :prompt-theme cleanup
+  # This handles all the stuff from the default :prompt-theme restore
   local +h PS1=$PS1 PS2=$PS2 PS3=$PS3 PS4=$PS4 RPS1=$RPS1 RPS2=$RPS2
   local +h PROMPT=$PROMPT RPROMPT=$RPROMPT RPROMPT2=$RPROMPT2 PSVAR=$PSVAR
-  local -a precmd_functions preexec_functions prompt_preview_cleanup
+  local -a precmd_functions preexec_functions prompt_preview_restore
   local -aLl +h zle_highlight
 
   {
     # Save and clear current restore-point if any
-    zstyle -g prompt_preview_cleanup :prompt-theme cleanup
+    zstyle -g prompt_preview_restore :prompt-theme restore
     {
-      zstyle -d :prompt-theme cleanup
+      zstyle -d :prompt-theme restore
+      # Execute current cleanup sequence, if any.
+      zstyle -t :prompt-theme cleanup
 
       # The next line is a bit ugly.  It (perhaps unnecessarily)
       # runs the prompt theme setup function to ensure that if
@@ -74,8 +76,8 @@ prompt_preview_safely() {
       zstyle -t :prompt-theme cleanup
     }
   } always {
-    (( $#prompt_preview_cleanup )) &&
-      zstyle -e :prompt-theme cleanup "${prompt_preview_cleanup[@]}"
+    (( $#prompt_preview_restore )) &&
+      zstyle -e :prompt-theme restore "${prompt_preview_restore[@]}"
   }
 }
 
@@ -103,9 +105,11 @@ Use prompt -h <theme> for help on specific themes.'
         local +h PS1=$PS1 PS2=$PS2 PS3=$PS3 PS4=$PS4 RPS1=$RPS1 RPS2=$RPS2
         local +h PROMPT=$PROMPT RPROMPT=$RPROMPT RPROMPT2=$RPROMPT2 PSVAR=$PSVAR
         local -a precmd_functions preexec_functions
+        local theme_reset=''
       else
-        trap 'prompt_${prompt_theme[1]}_setup "${(@)prompt_theme[2,-1]}"' 0
+        local theme_reset='prompt_${prompt_theme[1]}_setup "${(@)prompt_theme[2,-1]}"'
       fi
+      trap 'zstyle -t :prompt-theme cleanup;'"${theme_reset:+ $theme_reset}" 0
       ;;
   esac
   case "$opt" in
@@ -120,6 +124,7 @@ Use prompt -h <theme> for help on specific themes.'
        ;;
     h) if [[ -n "$2" && -n $prompt_themes[(r)$2] ]]; then
          if functions prompt_$2_setup >/dev/null; then
+           zstyle -t :prompt-theme cleanup
            # The next line is a bit ugly.  It (perhaps unnecessarily)
            # runs the prompt theme setup function to ensure that if
            # the theme has a _help function that it's been autoloaded.
@@ -179,28 +184,38 @@ Use prompt -h <theme> for help on specific themes.'
        typeset -ga zle_highlight=( ${zle_highlight:#default:*} )
        (( ${#zle_highlight} )) || unset zle_highlight
 
+       zstyle -t :prompt-theme cleanup
        prompt_$1_setup "$@[2,-1]" && prompt_theme=( "$@" )
        ;;
   esac
 }
 
 prompt_cleanup () {
-  local -a cleanup_hooks
-  if zstyle -g cleanup_hooks :prompt-theme cleanup
-  then
-    cleanup_hooks+=(';' "$@")
-    zstyle -e :prompt-theme cleanup "${cleanup_hooks[@]}"
-  elif (( $+prompt_preview_cleanup == 0 ))
+  local -a cleanup_hooks theme_active
+  if ! zstyle -g cleanup_hooks :prompt-theme cleanup
   then
-    print -u2 "prompt_cleanup: no prompt theme active"
-    return 1
+    (( $+prompt_preview_restore == 0 )) &&
+    if ! zstyle -g theme_active :prompt-theme restore
+    then
+      print -u2 "prompt_cleanup: no prompt theme active"
+      return 1
+    fi
+
+    # Set the cleanup sequence up.
+    zstyle -e :prompt-theme cleanup \
+        'zstyle -d :prompt-theme cleanup;' \
+        'reply=(yes)'
+    zstyle -g cleanup_hooks :prompt-theme cleanup
   fi
+
+  cleanup_hooks+=(';' "$@")
+  zstyle -e :prompt-theme cleanup "${cleanup_hooks[@]}"
 }
 
 prompt () {
   local -a prompt_opts theme_active
 
-  zstyle -g theme_active :prompt-theme cleanup || {
+  zstyle -g theme_active :prompt-theme restore || {
     # This is done here rather than in set_prompt so that it
     # is safe and sane for set_prompt to setopt localoptions,
     # which will be cleared before we arrive back here again.
@@ -210,8 +225,8 @@ prompt () {
     [[ -o promptpercent ]] && prompt_opts+=(percent)
     [[ -o promptsp ]] && prompt_opts+=(sp)
     [[ -o promptsubst ]] && prompt_opts+=(subst)
-    zstyle -e :prompt-theme cleanup \
-        'zstyle -d :prompt-theme cleanup;' \
+    zstyle -e :prompt-theme restore \
+        'zstyle -d :prompt-theme restore;' \
 	'prompt_default_setup;' \
         ${PS1+PS1="${(q)PS1}"} \
         ${PS2+PS2="${(q)PS2}"} \
@@ -239,7 +254,7 @@ prompt_preview_theme () {
   emulate -L zsh
 
   # Check for proper state handling
-  (( $+prompt_preview_cleanup )) || {
+  (( $+prompt_preview_restore )) || {
     prompt_preview_safely "$@"
     return
   }
@@ -249,6 +264,7 @@ prompt_preview_theme () {
   print -n "$1 theme"
   (( $#* > 1 )) && print -n " with parameters \`$*[2,-1]'"
   print ":"
+  zstyle -t :prompt-theme cleanup
   prompt_${1}_setup "$@[2,-1]"
   (( ${#prompt_opts} )) &&
       setopt noprompt{bang,cr,percent,sp,subst} "prompt${^prompt_opts[@]}"
-- 
2.30.1



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

* Re: [PATCH v2] promptinit: Fix prompt cleanups
  2021-02-22 22:28     ` [PATCH v2] " Arseny Maslennikov
@ 2021-02-22 23:50       ` Bart Schaefer
  0 siblings, 0 replies; 11+ messages in thread
From: Bart Schaefer @ 2021-02-22 23:50 UTC (permalink / raw)
  To: Arseny Maslennikov; +Cc: zsh-workers

On Mon, Feb 22, 2021 at 2:28 PM Arseny Maslennikov <ar@cs.msu.ru> wrote:
>
>  prompt_cleanup () {
[...]
> +    (( $+prompt_preview_restore == 0 )) &&
> +    if ! zstyle -g theme_active :prompt-theme restore
> +    then

A little idiosyncratic :-) but OK.


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

end of thread, other threads:[~2021-02-22 23:51 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-25  8:16 [PATCH 1/2] promptinit: typo: RPOMPT -> RPROMPT Arseny Maslennikov
2021-01-25  8:16 ` [PATCH 2/2] promptinit: Fix prompt cleanups Arseny Maslennikov
2021-01-25  8:23   ` Roman Perepelitsa
2021-01-25 12:50     ` Arseny Maslennikov
2021-02-18 13:03   ` (ping) " Arseny Maslennikov
2021-02-20  0:50     ` dana
2021-02-20 19:01       ` Bart Schaefer
2021-02-22 18:29   ` Bart Schaefer
2021-02-22 22:24     ` Arseny Maslennikov
2021-02-22 22:28     ` [PATCH v2] " Arseny Maslennikov
2021-02-22 23:50       ` Bart Schaefer

zsh-workers

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.vuxu.org/zsh-workers

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 zsh-workers zsh-workers/ http://inbox.vuxu.org/zsh-workers \
		zsh-workers@zsh.org
	public-inbox-index zsh-workers

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.vuxu.org/vuxu.archive.zsh.workers


code repositories for the project(s) associated with this inbox:

	https://git.vuxu.org/mirror/zsh/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git