On Thu, Apr 11, 2013 at 11:51:22PM +0100, Thomas Adam wrote:
> 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.
Ok let's just leave prepare() as you have it then.
>
> 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.
Yes but now we don't have it anymore :-).
I did think of suggesting just to ditch cmd_context entirely and put the
members inside struct cmd_q but I think it is better with a separate
struct cmd_ctx - there is no need to malloc it though, just put it as a
member in struct cmd_q.
Thanks
>
> > 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
