Overriding commands in plugins

[Michael Gliwinski]

On Monday 22 Mar 2010 03:25:02 Martin Pool wrote:
> >> Perhaps some could be generic, like "after opening the source" but I
> >> think the most interesting ones need to be specific to commands, or at
> >> least to groups of similar commands.
> >
> > That sounds good.  However also slightly complicated when thinking about
> > implementation.  Is there a concept of "groups of commands" in bzr
> > already or would that require a refactor somewhere?
> 
> No, there's no such concept at the moment.  I don't think it needs to
> exist as a particular class.  I would basically be looking to either
> put it on a lower-level object, or as a hook on Command that's
> documented as eg "for all commands that update a working tree, before
> they start that update."

OK, got it.

> >> You're right the lower levels shouldn't be directly dealing with the
> >> UI, but I think it's reasonable for them to say to the UIFactory "I'm
> >> doing something the user might want to know about or confirm", and
> >> then for it to if appropriate ask.  That gives us a central place to
> >> suppress those warnings, to test them, and potentially to hook them.
> >> I think the cross-format fetch warning is fairly tasteful.
> >
> > OK, just finished reading bzrlib.ui and qbzr's uifactory.  All makes
> > sense now. So this should be possible with addition of e.g. 'default'
> > param to ui_factory.get_boolean, I think.  This would allow e.g.:
> >
> > if ui_factory.get_boolean('Encountered foo, abort?', default=True):
> >    raise # something to abort
> >
> > or maybe a separate method to allow for more verbose description of the
> > condition?  I can imagine (for my case) wanting to pass some object
> > representing state of the tree and having ui_factory figure out how to
> > present it.
> 
> It should generally be at a higher level of abstraction than
> get_boolean, because what the command really wants to know is not "ask
> the user a boolean" but "here's an option the user might want to
> confirm" - then we can generically turn on or off all confirmations.
> 
> So basically ui_factory.seek_confirmation('switch_with_dirty_tree',
> default=True)

Yes indeed, missed that point.  Anyway, this is generally what I was thinking 
with separate method approach so I have a good idea what needs done there.

> >> >> I think this probably ought to be in a WorkingTree.pre_switch hook?
> >> >>
> >> >> If other developers feel this would be useful I can help you add it.
> >> >
> >> > OK, so I had a look at how switch is implemented and I'm not quite
> >> > sure how pre_switch hook could be implemented.  IIUC switch isn't as
> >> > really an operation on the WorkingTree, it actually changes branch
> >> > location in the control directory, pulls the branch (if any) and then
> >> > updates the WorkingTree.
> >> >
> >> > Would anyone actually be interested in that?  Cause for my use case I
> >> > can just hook into the CLI command (my users don't use GUIs nor more
> >> > complex things like looms, etc.)
> >>
> >> So in this particular case we have an intermediate layer bzrlib.switch
> >> and it would be reasonable to run the hook from there.  As you say
> >> there are actually two phases, first switching to the other branch and
> >> then updating the wt.  It would be reasonable to have a hook on the wt
> >> that can interrupt before the update, but really you want to catch
> >> this before anything is done, so probably a hook run near the top of
> >> _set_branch_location would be good.
> >>
> >> It's not as obvious as wrapping the command but it would be a step in
> >> a very good direction and we will help you if you have a go at it.
> >
> > OK, so just to make sure I got this right.  So you mean adding a
> > HookPoint 'pre_switch' to MutableTreeHooks and invoking that hook from
> > bzrlib.switch somewhere, yes?  Maybe directly in bzrlib.switch.switch
> > then, e.g. before/after _check_pending_merges?
> 
> Yes.  Then the code run from that hook should call
> ui_factory.seek_confirmation(...) which should be implemented on
> TextUIFactory as get_boolean and perhaps in the DefaultUIFactory as
> just returning the default.

OK.

> > Right, anyways, I think I'm going to start by logging feature requests
> > for those three things in LP and we'll go from there.  I'll find some
> > time to have a look at at least the pre_switch hook and ui_factory
> > change.

OK, so I'll hopefully get around to this soon.  Many thanks for your help.


-- 
Michael Gliwinski
Henderson Group Information Services
9-11 Hightown Avenue, Newtownabby, BT36 4RT
Phone: 028 9034 3319

**********************************************************************************************
The information in this email is confidential and may be legally privileged.  It is intended solely for the addressee and access to the email by anyone else is unauthorised.
If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.
When addressed to our clients, any opinions or advice contained in this e-mail are subject to the terms and conditions expressed  in the governing client engagement leter or contract.
If you have received this email in error please notify support at henderson-group.com

John Henderson (Holdings) Ltd
Registered office: 9 Hightown Avenue, Mallusk, County Antrim, Northern Ireland, BT36 4RT.
Registered in Northern Ireland
Registration Number NI010588
Vat No.: 814 6399 12
*********************************************************************************