Off-by-one with select-*-shell-word text object?

Off-by-one with select-*-shell-word text object?

[Bart Schaefer]

Noticed during recent zsh-users thread:

torch% quote-this-word() { zle select-in-shell-word; zle quote-region }
torch% zle -N quote-this-word
torch% bindkey "''" quote-this-word
torch% echo foo''
torch% echo' foo'

Note the left-quote has now included the space preceding the word.  I at
first suspected another vi-mode vs. emacs-mode difference, but:

torch% bindkey -v "''" quote-this-word
torch% bindkey -v
torch% echo foo''
torch% echo' foo'

Same effect for select-a-shell-word.  The right thing happens if I avoid
the *-shell-word variant:

torch% quote-this-word() { zle select-in-word; zle quote-region }
torch% echo foo''
torch% echo 'foo'

If including the preceding space is intentional, it should be mentioned
in the documentation.

Re: Off-by-one with select-*-shell-word text object?

[Oliver Kiddle]

Bart wrote:
> Noticed during recent zsh-users thread:
>
> torch% quote-this-word() { zle select-in-shell-word; zle quote-region }
> torch% zle -N quote-this-word
> torch% bindkey "''" quote-this-word
> torch% echo foo''
> torch% echo' foo'
>
> Note the left-quote has now included the space preceding the word.  I at
> first suspected another vi-mode vs. emacs-mode difference, but:
>
> torch% bindkey -v "''" quote-this-word

You'd need to do vicmd bindings to identify a vi-mode vs. emacs-mode
difference: bindkey -a. However, it is neither that nor an off-by-one
error.

The problem is that both select-in-shell-word and select-a-shell-word
are handled by a single function that tests the value of bindk to
see which widget it is running. bindk apparently points to the
quote-this-word shell widget so that isn't doing what was intended.
In this case you're getting select-a-shell-word behaviour as a result.
For select-a-shell-word including the preceding space is intentional.

Looking at execzlefunc, I can't see an alternative way to get the
actual widget being run. Have I missed anything? I could add another
global variable for that. Or a ZLE_ flag - actually two to cover
select-in-word/select-in-blank-word. Or it needs six small wrapper
functions for the actual widgets. Have you got any preference in
the choice between those or alternative ideas?

Oliver

Re: Off-by-one with select-*-shell-word text object?

[Bart Schaefer]

On Sep 16, 10:47pm, Oliver Kiddle wrote:
} Subject: Re: Off-by-one with select-*-shell-word text object?
}
} Bart wrote:
} > Noticed during recent zsh-users thread:
} >
} > torch% quote-this-word() { zle select-in-shell-word; zle quote-region }
} > torch% zle -N quote-this-word
} > torch% bindkey "''" quote-this-word
} > torch% echo foo''
} > torch% echo' foo'
} 
} The problem is that both select-in-shell-word and select-a-shell-word
} are handled by a single function that tests the value of bindk to
} see which widget it is running.

Aha!  Indeed, this works:

  quote-this-word() { zle select-in-shell-word -w ; zle quote-region }

} For select-a-shell-word including the preceding space is intentional.

That should probably be documented, then.  Currently the doc says the
adjacent blanks are selected for -a-blank-word and -a-word, but does
not mention that for -a-shell-word.  I suppose it could be inferred
from name similarity, but that shouldn't be necessary.

} Looking at execzlefunc, I can't see an alternative way to get the
} actual widget being run. Have I missed anything?

Although it's not ideal to have a built-in widget that requires the
use of the -w flag when calling from a user-defined widget, it's also
probably OK if properly documented.

} Or it needs six small wrapper functions for the actual widgets.

This would be in line with the way similar widgets with a common
core have been implemented in the past.  However:

} Or a ZLE_ flag - actually two to cover
} select-in-word/select-in-blank-word.

I think one flag to force the equivalent of set_bindk = 1 would do?
(zle_main.c:1358)

I don't have a preference here, except:

It occurs to me that using this same sort of flag on the menu-complete
and reverse-menu-complete Thingys might have fixed the problem that
you addressed a different way in workers/38043.

It also occurs to me to wonder whether set_bindk = 0 *ever* makes sense
for builtin widgets.  Can somebody give me an example of a builtin that
would care about the widget name of its user-defined caller?

Re: Off-by-one with select-*-shell-word text object?

[Oliver Kiddle]

Bart wrote:
> } Or a ZLE_ flag - actually two to cover
>
> I think one flag to force the equivalent of set_bindk = 1 would do?
> (zle_main.c:1358)

That seems like a nice simple solution. Especially if there aren't
any that need set_bindk = 0 as you suggest below.

> It also occurs to me to wonder whether set_bindk = 0 *ever* makes sense
> for builtin widgets.  Can somebody give me an example of a builtin that
> would care about the widget name of its user-defined caller?

I think it matters for the k2 == bindk test in getvirange(). That's
the code that ensures that pressing the same key twice indicates
that the vi command should operate on a line. Builtin widgets that
call getvirange() all have the ZLE_VIOPER flag set so could be
selected for on the basis of that.

zlecallhook sets set_bindk. That doesn't seem entirely useful
($WIDGET is zle-keymap-select etc) but if it did the opposite, that
could matter here because you'd get something like vi-cmd-mode
instead of the calling widget.

run-help, which-command and zap-to-char suffer from the same
issue as the text object widgets: they use bindk to select their
behaviour. 

So any thoughts on this solution?

Oliver

diff --git a/Src/Zle/zle_thingy.c b/Src/Zle/zle_thingy.c
index 9107154..345534e 100644
--- a/Src/Zle/zle_thingy.c
+++ b/Src/Zle/zle_thingy.c
@@ -756,7 +756,10 @@ bin_zle_call(char *name, char **args, UNUSED(Options ops), UNUSED(char func))
     }
 
     t = rthingy(wname);
-    ret = execzlefunc(t, args, setbindk);
+    /* for internal widgets we don't want bindk set except for when getting
+     * a vi range to detect a repeated key */
+    ret = execzlefunc(t, args, setbindk ||
+	    (t->widget && (t->widget->flags & (WIDGET_INT | ZLE_VIOPER)) == WIDGET_INT));
     unrefthingy(t);
     if (saveflag)
 	zmod = modsave;

Re: Off-by-one with select-*-shell-word text object?

[Bart Schaefer]

On Sep 17,  1:30am, Oliver Kiddle wrote:
} Subject: Re: Off-by-one with select-*-shell-word text object?
}
} Bart wrote:
} > It also occurs to me to wonder whether set_bindk = 0 *ever* makes sense
} > for builtin widgets.  Can somebody give me an example of a builtin that
} > would care about the widget name of its user-defined caller?
} 
} I think it matters for the k2 == bindk test in getvirange().

That gets me wondering about the execzlefunc() call in getvirange().
E.g. in vicmd mode, cS changes nothing but leaves me in insert mode.
Probably should be an error to invoke any ZLE_VIOPER widget *other*
than the same one again?  But I digress ...

} zlecallhook sets set_bindk. That doesn't seem entirely useful

Actually it's quite useful, and the recently-added add-zle-hook-widget
function depends on it (to the point that zle-line-pre-redraw was
changed to match).

} run-help, which-command and zap-to-char suffer from the same
} issue as the text object widgets: they use bindk to select their
} behaviour. 
} 
} So any thoughts on this solution?

I'd probably write it as an assignment of setbindk rather than put the
whole expression in the execzlefunc argument, but otherwise this seems
sensible on the face of it.

Re: Off-by-one with select-*-shell-word text object?

[Daniel Shahaf]

Bart Schaefer wrote on Fri, Sep 16, 2016 at 20:02:05 -0700:
> On Sep 17,  1:30am, Oliver Kiddle wrote:
> } run-help, which-command and zap-to-char suffer from the same
> } issue as the text object widgets: they use bindk to select their
> } behaviour. 
> } 
> } So any thoughts on this solution?
> 
> I'd probably write it as an assignment of setbindk rather than put the
> whole expression in the execzlefunc argument, but otherwise this seems
> sensible on the face of it.

I think it would be cleaner to invent a new bitmask flag and set it on
the affected widgets (select-*-word, run-help, etc) than to overload
WIDGET_INT for this purpose.

Whether a widget is implemented in C or in shell has no bearing on
whether it assumes that $WIDGET points to itself.

Cheers,

Daniel

Re: Off-by-one with select-*-shell-word text object?

[Bart Schaefer]

On Sep 17,  7:42am, Daniel Shahaf wrote:
}
} I think it would be cleaner to invent a new bitmask flag and set it on
} the affected widgets (select-*-word, run-help, etc) than to overload
} WIDGET_INT for this purpose.

It's not overloading WIDGET_INT, it's overloading ZLE_VIOPER - because
those are the only cases of builtins that want the opposite case (point
to the caller).  Testing WIDGET_INT is only necessary because the sense
of setbindk is reversed from the sense of ZLE_VIOPER.

} Whether a widget is implemented in C or in shell has no bearing on
} whether it assumes that $WIDGET points to itself.

At the moment that's not true; a widget implemented in shell can never
safely assume that $WIDGET points to itself.

Re: Off-by-one with select-*-shell-word text object?

[Daniel Shahaf]

Bart Schaefer wrote on Sat, Sep 17, 2016 at 07:29:18 -0700:
> On Sep 17,  7:42am, Daniel Shahaf wrote:
> }
> } I think it would be cleaner to invent a new bitmask flag and set it on
> } the affected widgets (select-*-word, run-help, etc) than to overload
> } WIDGET_INT for this purpose.
> 
> It's not overloading WIDGET_INT, it's overloading ZLE_VIOPER - because
> those are the only cases of builtins that want the opposite case (point
> to the caller).  Testing WIDGET_INT is only necessary because the sense
> of setbindk is reversed from the sense of ZLE_VIOPER.
> 

The code tests that ZLE_VIOPER is unset and that WIDGET_INT is set;
therefore, any widget implemented in a module will have $WIDGET pointing
to itself, unless that widget has ZLE_VIOPER set.

As Oliver said, aside from the vi operator widgets, only 4 builtin
widgets care about whether $WIDGET points to themselves or not.  There
is no reason, AIUI, to force set_bindk to 1 for the other 390 builtin
widgets, as well as for all future widgets implemented in modules.

That's why I suggested to invent a new bitmask flag that implies
set_bindk on a per-widget basis: the property 'requires $WIDGET to be
self-referential' does not follow from 'is implemented in C'.  I can
also imagine future user-defined widgets that need $WIDGET to always
point to themselves.

> } Whether a widget is implemented in C or in shell has no bearing on
> } whether it assumes that $WIDGET points to itself.
> 
> At the moment that's not true; a widget implemented in shell can never
> safely assume that $WIDGET points to itself.

I am not arguing that widgets implemented in shell should be able to
assume that $WIDGET is self-referential.  I am arguing that widgets
implemented in C should not be able to to assume that.  (unless they
have that new bitmask flag I'm proposing)

Cheers,

Daniel

Re: Off-by-one with select-*-shell-word text object?

[Oliver Kiddle]

Daniel Shahaf wrote:
> Bart Schaefer wrote on Sat, Sep 17, 2016 at 07:29:18 -0700:
> > On Sep 17,  7:42am, Daniel Shahaf wrote:
> > }
> > } I think it would be cleaner to invent a new bitmask flag and set it on
> > } the affected widgets (select-*-word, run-help, etc) than to overload
> > } WIDGET_INT for this purpose.
> > 
> > It's not overloading WIDGET_INT, it's overloading ZLE_VIOPER - because

Or arguably it is bindk that is overloaded from when $WIDGET was
added as a shell interface. I'd consider redundancy in the flags
to be worse than overloading.

> As Oliver said, aside from the vi operator widgets, only 4 builtin
> widgets care about whether $WIDGET points to themselves or not.  There
> is no reason, AIUI, to force set_bindk to 1 for the other 390 builtin
> widgets, as well as for all future widgets implemented in modules.

You've got to force set_bindk to either 1 or 0 and my guess is that
a future widget is more likely to want 1 than to want 0. Even with
a new bitmask flag, I'd sooner opt for it having the opposite sense
to what you suggest.

> That's why I suggested to invent a new bitmask flag that implies
> set_bindk on a per-widget basis: the property 'requires $WIDGET to be
> self-referential' does not follow from 'is implemented in C'.  I can
> also imagine future user-defined widgets that need $WIDGET to always
> point to themselves.

We've already got a wealth of ZLE_ flags and setting them for a new
widget can be error prone. Nobody ever noticed that run-help,
which-command and zap-to-char wouldn't work from a shell widget so
picking up on errors is not necessarily easy.
[aside: if anyone relied on it for run-help/which-command to invoke
their own command, they should use zle -A].

getvirange() is more concerned with the keys that were used than
with the identify of the widget. That's why I'd sooner regard it
as bindk that is overloaded. But I don't think adding another
variable like $WIDGET is a good idea. Would just complicate the
interface and it isn't really needed.

Oliver

Re: Off-by-one with select-*-shell-word text object?

[Bart Schaefer]

On Sep 19, 12:01pm, Oliver Kiddle wrote:
}
} You've got to force set_bindk to either 1 or 0 and my guess is that
} a future widget is more likely to want 1 than to want 0. Even with
} a new bitmask flag, I'd sooner opt for it having the opposite sense
} to what you suggest.
}  
} Daniel Shahaf wrote:
} > That's why I suggested to invent a new bitmask flag that implies
} > set_bindk on a per-widget basis: the property 'requires $WIDGET to
} > be self-referential' does not follow from 'is implemented in C'.

My somewhat related point is that UNLESS we're going to add an option
to "zle -N"/"zle -C" to control that new bitmask flag for user-defined
widgets, the unusual and therefore flag-worthy case applies only with
respect to builtins, and therefore is "requires $WIDGET to be caller-
referential."

} [aside: if anyone relied on it for run-help/which-command to invoke
} their own command, they should use zle -A].

This might be worth documenting.  It might have to be paired with
"zle .run-help" or similar?

} getvirange() is more concerned with the keys that were used than
} with the identify of the widget. That's why I'd sooner regard it
} as bindk that is overloaded.

Indeed, "bindkey -a z vi-change" makes "cc" "cz" and "zc" synonymous.

} I don't think adding another variable like $WIDGET is a good idea.

It'd need to be a $LASTKEYS to go with $LASTWIDGET, if anything.