Community
Participate
Working Groups
NavigatorActionService subclasses ActionGroup but does not use the inherited updateActionBars(). For ActionGroups, fillActionBars should only get called once. Subsequent updates to the action bars should be done by calling setContext() followed by updateActionBars(). Ideally, any subclasses should follow this same lifecycle.
Clients must reconfigure the actionbars every time the CommonActionProvider is invoked. This is required because seach selection could have a different set of retargetable actions associated with it. Therefore, if the NavigatorActionService follows the pattern of fillActionBars() + updateActionBars()*, each client must be aware that they must save a reference to the IActionBars, reconfigure the global actions for each invocation of updateActionBars(), in addition to updating any action selection/enablement state. Given this constraint, do you still think this change makes sense? I think that alot of clients will fail to recognize this and as a result will encounter subtle problems.
I think you are right that it is really a different contract and I agree that it would probably be more difficult for clients to understand if the behavior was changed to match ActionGroup. When I logged the bug, I didn't know anything about the internals of the action provider. The most important thing is to make sure that the behavior is documented in the CommonActionProvider so that clients know what to expect.
Sounds good. I'll update the documentation accordingly.
Updated documentation for CommonActionProvider.
The fix for this introduced a new protected getter getActionSite() on an API class that clients may subclass. Note that there are a lot of existing subclasses of CommonActionProvider, and their implementations do not always call super.init(). Shouldn't subclasses required to call super.init() if they override init()? If yes, this should be specified on init(), not getActionSite(). Who is supposed to call getActionSite? The method is public, suggesting it might be called by anyone, not just subclasses. This suggests that calling super.init() from subclasses is in fact required. Reopening to track this discussion.
The method init() is public, the method getActionSite is protected final. If clients want the super class to cache the actionSite, they should call init(). I can update the doc accordingly to reflect this. For existing clients that do not call super.init() ( in fact, since the method was originally abstract, there are no existing clients that could have called super.init()), they will not be broken; not all clients need to cache the action site, some of them just use inofmration from it to create their actions and then forget the site.
Yes, you are right. I don't know why I thought the getter was public. Sorry for the false alarm! So after all, this was a very minor addition to the API for subclasses.
Ok, given that there is no API involved, you don't need input from me, but I am in favor of this change, in any case.
Fixed and released.
