Overriding commands in plugins

[Martin Pool]

On 20 March 2010 02:15, Michael Gliwinski
<Michael.Gliwinski at henderson-group.com> 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."


>
>> > Also, I agree in this case (and similar ones) it would make more sense
>> > for it to be done at lower level so other UIs and as Robert mentioned
>> > other conceptually similar things are handled as well.  The problem is
>> > that the lower level shouldn't really be doing UI kinds of things, e.g.
>> > interactively asking for confirmation.  Is there a way to express such
>> > things in UI-agnostic way anyways?
>>
>> 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)

>> >> 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.

> 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.


-- 
Martin <http://launchpad.net/~mbp/>