Re: [RFC][PATCH] Reset ZLE hooks when changing prompt themes (was Re: [RFC][PATCH] `newuser` prompt theme)

[Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx>]

Marlon wrote on Thu, Apr 15, 2021 at 21:54:48 +0300:
> Patch attached, but not yet including Daniel’s suggested change below (since I still have questions about it). It includes an update to the docs, but note that I am not able to build those myself. I would much appreciate it f someone else could do that to test whether I did it correctly.
> 
> On 14 Apr 2021, at 15:05, Daniel Shahaf <d.s@xxxxxxxxxxxxxxxxxx> wrote:
> >> +++ b/Functions/Prompts/promptinit
> >> @@ -178,8 +177,13 @@ Use prompt -h <theme> for help on specific themes.'
> >> 
> >>        # Reset some commonly altered bits to the default
> >>        local hook
> >> -       for hook in chpwd precmd preexec periodic zshaddhistory zshexit; do
> >> -         add-zsh-hook -D "${hook}" "prompt_*_${hook}"
> >> +       for hook in chpwd precmd preexec periodic zshaddhistory zshexit \
> >> +           zsh_directory_name; do
> >> +         add-zsh-hook -D "$hook" "prompt_*_$hook"
> >> +       done
> >> +       for hook in isearch-exit isearch-update line-pre-redraw line-init \
> >> +           line-finish history-line-set keymap-select; do
> >> +         add-zle-hook-widget -D "$hook" "prompt_*_$hook"
> > 
> > All these should be documented, like prompt_${foo}_preview is.
> > 
> > Recommend to name these prompt_${foo}_bar-{isearch-exit,isearch-update,…,keymap-select}
> > for some fixed value of «bar» to avoid namespace issues (i.e., name
> > collisions between existing prompts and future hooks).
> 
> Wouldn’t that be a breaking change in the API, though?

No, it wouldn't, because those are add-zle-hook-widget hooks and the
current API only deals with add-zsh-hook hooks.

We could even recommend that add-zsh-hook hooks be named
prompt_${foo}_baz_{chpwd,…} for some fixed value of baz.  We don't even
have to change the code to support this (because the asterisk will match
«${foo}_baz» just fine); we just need to document the recommendation and
deprecate the previous recommended naming pattern.

> I mean, there are already 3rd party prompt themes that use the current prompt_${foo}_{isearch-exit,isearch-update,…,keymap-select} naming scheme.

And you're arguing that avoiding breaking code of third parties that
relied on a patch that (presumably) hadn't even been posted to
-workers@, never mind merged to master and released in a stable release,
trumps preventing namespace collisions for future users?

> Also, isn’t the prefix prompt_${foo}_ already a namespace of sorts? Why change this?

What if the foo theme declares a function called prompt_foo_lorem (or
even prompt_foo_ipsum_lorem) that _isn't_ an «add-*-hook lorem» hook
function, and later we start to to auto-register functions matching the
name pattern?

Or if we were to make «add-zsh-hook bar lorem» and «add-zle-hook-widget
bar ipsum» both valid, for a single value of bar, and a prompt theme
wanted to install both hooks _and_ name the functions «lorem» and «ipsum»
conventionally?

Or if someone wanted to quickly grep for add-zle-hook-widget callbacks?

> From b60bfd135ae2631a3b28aaf81074594d0ec1f967 Mon Sep 17 00:00:00 2001
> From: Marlon Richert <marlon.richert@xxxxxxxxx>
> Date: Thu, 15 Apr 2021 21:50:36 +0300
> Subject: [PATCH] Reset ZLE hooks when changing prompt themes
> 

Your mail didn't trigger workers/48587 (that's good).  I wonder why.
Probably because your MUA quoted-printable-escaped the first byte of the
From_ line.

> +++ b/Doc/Zsh/contrib.yo
> @@ -2032,10 +2032,10 @@ setopts (tt(promptbang), etc.) are turned on, all other prompt-related
>  options are turned off.  The tt(prompt_opts) array preserves setopts even
>  beyond the scope of tt(localoptions), should your function need that.
>  )
> -item(Modify precmd and preexec)(
> -Use of tt(add-zsh-hook) is recommended.  The tt(precmd) and tt(preexec)
> -hooks are automatically adjusted if the prompt theme changes or is
> -disabled.
> +item(Modify hooks)(
> +Use of tt(add-zsh-hook) and tt(add-zle-hook-widget) is recommended.  All hooks
> +that follow the naming pattern tt(prompt_<theme>_<hook>) are automatically

This should be tt(prompt_)var(theme)tt(_)var(hook).

> +removed when the prompt theme changes or is disabled.

Add a pointer to the relevant section?  Something like
.
    ifzman(… Manipulating Hook Functions … above)ifnzman(noderef(Manipulating Hook Functions))
.
plus appropriate markup in the first branch.

> +++ Functions/Prompts/promptinit
> @@ -14,6 +14,8 @@ prompt_themes=()
>  promptinit () {
>    emulate -L zsh
>    setopt extendedglob
> +  autoload -Uz add-zsh-hook add-zle-hook-widget
> +
>    local ppath='' name theme
>    local -a match mbegin mend
>  
> @@ -32,9 +34,6 @@ promptinit () {
>      fi
>    done
>  
> -  # To manipulate precmd and preexec hooks...
> -  autoload -Uz add-zsh-hook
> -
>    # Variables common to all prompt styles
>    prompt_newline=$'\n%{\r%}'
>  }
> @@ -178,8 +177,13 @@ Use prompt -h <theme> for help on specific themes.'
>  
>         # Reset some commonly altered bits to the default
>         local hook
> -       for hook in chpwd precmd preexec periodic zshaddhistory zshexit; do
> -         add-zsh-hook -D "${hook}" "prompt_*_${hook}"
> +       for hook in chpwd precmd preexec periodic zshaddhistory zshexit \
> +           zsh_directory_name; do
> +         add-zsh-hook -D "$hook" "prompt_*_$hook"
> +       done
> +       for hook in isearch-exit isearch-update line-pre-redraw line-init \
> +           line-finish history-line-set keymap-select; do
> +         add-zle-hook-widget -D "$hook" "prompt_*_$hook"
>         done
>         typeset -ga zle_highlight=( ${zle_highlight:#default:*} )
>         (( ${#zle_highlight} )) || unset zle_highlight

Hmm.  I'm not at all sure that this is sufficient.  There's a bunch of
reference to precmd and preexec in promptinit; it's possible that some
of these should also handle chpwd,periodic,zshaddhistory,zshexit
(a preëxisting issue) and the add-zle-hook-widget hook classes (an issue
this patch might be introducing).

Cheers,

Daniel