Hi,
On Thu, Apr 11, 2013 at 11:32:21PM +0100, Nicholas Marriott wrote:
> Should prepare actually do validation be able to return
> CMD_RETURN_ERROR? Or would we want to run hooks even when we know the
> command is going to fail?
Thanks for this feedback, and the other point about hooks acting more like
options do -- I'll get round to those in a bit.
As for whether prepare() should do validation... I'm not sure. I don't
think it can without restructuring some commands quite heavily to cater for
that. It is a slightly odd mixture of state between prepare() setting the
running environment for the command, and exec() still using bits of that
context and parsing the args struct to do further validation and to then
either invalidate running that command, or not. But I can't see how else it
should work, since exec() on some commands really can get interesting,
especially where it is trying to emulate more than one command in the same
exec() function (such as select-window, choose-tree, etc.)
So I think it's safe to say that prepare() has no knowledge of whether a
command is going to run or not, and all it should guarantee is the
environment it can set up for the command. That can fail of course in
exec() at the point that tries to use the context it's being asked to run in
and that's OK. So we should probably just allow this to try and run and
fail in the same way as an ordinary command would do.
I hope that makes sense.
> I think struct cmd_ctx would be better than cmd_context, it matches all
> the others.
Heh; I did that deliberately since we used to have cmd_ctx before cmd_q came
along, and it was getting confusing; but sure, I can rename this.
> Could struct cmd_q just contain a cmd_ctx rather than a pointer?
> Particularly since it appears to be leaked :-).
Yeah, I spotted that and free()d it. I suppose we could just include it
without malloc()ing it.
> Also I would be inclined to declare it as:
>
> struct cmd_ctx {
> struct client *c;
> struct window *w;
> struct window *w2;
> struct window_pane *wp;
> struct window_pane *wp2;
> ...
> }
OK.
> Some other stuff, mostly minor apart from cmd-queue.c. Trimmed out a lot
> of context hope it is clear enough:
>
> > +void
> > +cmd_bind_key_prepare(unused struct cmd *self, unused struct cmd_q *cmdq)
> > +{
> > + return;
> > +}
>
> This return is unnecessary.
Yeah, I'll make this NULL in cmd_entry for those commands which don't need a
prepare() and then not define one. It's like this only because I
auto-generated these stubs from the outset to avoid RSI.
[... minor nites duly noted ...]
>
> I think I would add a helper something like:
>
> void
> cmdq_run_hook(struct hooks *hooks, const char *prefix, struct cmd *cmd,
> struct cmdq *cmdq)
> {
> struct hook *h;
> char *s;
>
> xasprintf(&s, "%s-%s", prefix, cmd->entry->name);
> if ((h = hooks_find(hooks, s)) != NULL)
> hooks_run(h, cmdq);
> free(s);
> }
>
> Then the changes to cmdq_continue could be much smaller.
Sure. Can do.
> > +enum cmd_retval
> > +cmd_show_hooks_exec(struct cmd *self, struct cmd_q *cmdq)
> > +{
> > + struct args *args = self->args;
> > + struct session *s;
> > + struct hook *hook;
> > + struct hooks *hooks_ent;
>
> Why not just hooks?
Dunno. Will change it.
Thanks for the review, I'll reroll a patch with these fixups in.
-- Thomas Adam
--
"It was the cruelest game I've ever played and it's played inside my head."
-- "Hush The Warmth", Gorky's Zygotic Mynci.
------------------------------------------------------------------------------
Precog is a next-generation analytics platform capable of advanced
analytics on semi-structured data. The platform includes APIs for building
apps and a phenomenal toolset for data science. Developers can use
our toolset for easy data analysis & visualization. Get a free account!
http://www2.precog.com/precogplatform/slashdotnewsletter
_______________________________________________
tmux-users mailing list
tmux-users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/tmux-users
