Bugzilla – Full Text Bug Listing
| Summary: | [EclipseContext] revisit how variables are looked-up | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Paul Webster <pwebster> | ||||||||||||||||
| Component: | UI | Assignee: | Oleg Besedin <ob1.eclipse> | ||||||||||||||||
| Status: | VERIFIED FIXED | QA Contact: | |||||||||||||||||
| Severity: | normal | ||||||||||||||||||
| Priority: | P3 | CC: | bokowski, bsd, emoffatt, john.arthorne, ob1.eclipse, remy.suen | ||||||||||||||||
| Version: | unspecified | ||||||||||||||||||
| Target Milestone: | 4.1 M3 | ||||||||||||||||||
| Hardware: | PC | ||||||||||||||||||
| OS: | Linux | ||||||||||||||||||
| Whiteboard: | |||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Paul Webster
You can add a "service" 5 ways, each with different implications for when it is available - when the MPart is created you can just set X.class.getName() => xInstance - you can set X.class.getName() => context function, that will instantiate and return the xInstance when asked - you can use the event broker to listen for a context being set, and then perform one of the above 2 steps - you can provide a context function through DS which will then be available at the root context - you can simply provide an OSGi service which will be available at the root context We are currently applying a number of patterns to make this information available. Sometimes we set it in response to events, sometimes we have to set local information and then post a ContextFunction like org.eclipse.e4.ui.internal.workbench.ActivePartLookupFunction. Usually we do it statically, like in org.eclipse.e4.ui.internal.workbench.E4Workbench.createWorkbenchContext(IEclipseContext, IExtensionRegistry, IExceptionHandler, IContributionFactory) (not extensible). Here are some thoughts on how we can simplify the system. 1) Simplify the requirements on providing information by removing the restriction that applicationContext.get(variable) has to also have access to that information (the more general statement would be that you don't have to make your data available higher up the IEclipseContext parent chain). 2) make finding the correct context the responsibility of the caller (like the keybinding dispatcher or the menu item selection listener). PW New Guidelines:
1) you can ask "something" at the MApplication level for the application's "active context" ... implementation would probably be the common "look up the active child chain" code:
IEclipseContext next
=(IEclipseContext)current.getLocal(IContextConstants.ACTIVE_CHILD);
while (next != null) {
current = next;
next = (IEclipseContext)
current.getLocal(IContextConstants.ACTIVE_CHILD);
}
2) each MWindow should also maintain a notion of its active child context. This handles the case where you have 2 MWindows and can see both toolbars. You want the toolitems in the second window looking up their handler in the second window, and dealing with the second window selection, etc.
This might be something that has to be generalized to the MParts as well, but I don't have a usecase for that.
3) this will allow us to simplify how we provide a lot of our current data. Parts can simply set the data in their context. The selection can also be set in a context, and and then another mechanism, like a RunAndTrack or EventHandler can post the value to the Window context. It can either be the same variable at both levels, or the notion of set an inputVariable and it's available as an outputVariable.
PW
(In reply to comment #1) > You can add a "service" 5 ways, each with different implications for when it is > available This should be dealt with in a different bug. PW Thanks for writing this up Paul.
I like the proposal to being able to resolve variables through the active context. Many of my handlers affect the active part. But I try to write them as top-level handlers (e.g., set in the top-level application), or at least capable of being installed as such. This requires that I write them as:
@Execute
public void execute(@Named(IServiceContants.ACTIVE_PART) part) {
IEclipseContext context = part.getContext();
Object var = context.get(...);
Object var2 = context.get(...);
....
}
So I lose the benefits of injection.
To make sure I get it: What you proposing is to have a method that returns an active child? Something like: void IEclipseContext#makeActive() IEclipseContext IEclipseContext#getActive() AFAIK we manage the ACTIVE_CHILD property OK, although if you want to encapsulate that we'll have to look at the 2 usecases I know of:
1) parentContext makes childContextN the ACTIVE_CHILD
2) parentContext clears the ACTIVE_CHILD variable (now it's a leaf node)
The part that we definitely need is something (either another constant backed by a ContextFunction or encoded in the IEclipseContext methods):
IEclipseContext getActiveChildLeaf() {
IEclipseContext current = this;
IEclipseContext child
= (IEclipseContext)current.getLocal(IContextConstants.ACTIVE_CHILD);
while (child != null) {
current = child;
child = (IEclipseContext) current.getLocal(IContextConstants.ACTIVE_CHILD);
}
return current;
}
PW
Created attachment 180080 [details]
Patch I
The patch adds new methods to IEclipseContext:
void activate();
IEclipseContext deactivate();
IEclipseContext getActive();
and a new qualifier "@Active".
Adding "@Active" to injected values will instruct Di to use active context.
("Active" hereinafter is the leaf-most descendent context that can be traversed to by looking up active children of the specified context.)
The patch also contains JUnits to check new functionality.
Next, I'll start changing UI code with the goal of eliminating external access to IContextConstants.ACTIVE_CHILD. It is quite possible that new methods on IEclipseContext will be adjusted in the process.
(In reply to comment #9) > Adding "@Active" to injected values will instruct Di to use active context. Sounds great! This seems to push activation history tracking into the context. Is this really something that can be handled generically at this level, or is it possible that a UI might want to make different activation history decisions? For example in Firefox when I close a tab, it activates the tab that is closest to the one that was closed, regardless of my activation history. This code seems to enforce MRU activation ordering at a low level, where that might always be appropriate. Also, is there some other part of the UI (model, widgets), that are tracking activation history today? If so, is the aim to consolidate them, or somehow keep the activation history data in sync? (In reply to comment #11) > This seems to push activation history tracking into the context. ... > This code seems to enforce MRU activation ordering ... That's a good point; I was (and still am :-)) somewhat unsure about this. Activating last active child seems to be convenient, but it is easy to imagine cases where it is not necessary. I guess I can limit #deactivate() processing to only return the last MRU entry, leaving decision to activate it or not to the consumer. > Also, is there some other part of the UI (model, widgets), that are tracking > activation history today? Nope, there is none. That was a bit of a problem in the 4.0 release. Created attachment 180165 [details]
Patch II
Changes code so that #deactivate() does not activates the MRU context, but merely returns it - making it up to consumer to decide if they want to activate MRU context.
(Patch applied to CVS Head.)
(In reply to comment #13) > Changes code so that #deactivate() does not activates the MRU context, but > merely returns it - making it up to consumer to decide if they want to activate > MRU context. The client cannot access the MRU list though so it's not clear to me how the client is supposed to achieve this. Unless the expectation is that the client also keep its own MRU list. (In reply to comment #14) > The client cannot access the MRU list though so it's not clear to me how the > client is supposed to achieve this. Unless the expectation is that the client > also keep its own MRU list. If the deactivate method returns the MRU child, then the client can choose to make that active... If on the other hand the client wants different activation ordering (like Firefox nearest neighbour for example), they can decide that for themselves. (In reply to comment #15) > If the deactivate method returns the MRU child, then the client can choose to > make that active... Right, never mind, I was reading the code wrong. I think the deactivate() method's implementation is off though as it is arbitrarily making the parent context not have an active child. If I have a window and the active part is partA and I tell partB to deactivate, I would expect it to be a no-op since partB is already "not active". However, the current implementation has no regard for this and just sets the window's context to not have any active child...? Created attachment 180175 [details] Patch III - #deactivate() should do nothing on inactive context (In reply to comment #16) > I think the deactivate() method's implementation is off though as it is > arbitrarily making the parent context not have an active child. > If I have a window and the active part is partA and I tell partB to deactivate, > I would expect it to be a no-op since partB is already "not active". Good point, I'll change the code to check if context being deactivated is, in fact, an active context before doing anything. (Patch applied to CVS Head.) Are there plans to introduce another method that will set the active child but not do it recursively? There is a lot of code right now that just sets a context's active child without forcing the entire active chain to be altered. (In reply to comment #18) > Are there plans to introduce another method that will set the active child but > not do it recursively? There is a lot of code right now that just sets a > context's active child without forcing the entire active chain to be altered. That depends on whether that is a desirable behavior. The new #activate() method currently walk up the parenting chain making its branch active(* See the note below). Is there a use case where we actually want to activate one child context, whithout activating its branch? -------- * Note: the "walk up" is currently done until it enounters a node that is already active. This limits the churn, but might not work in all cases. (Say, we encounter a node that is already active but not marked as active child in its parent.) I might need to adjust the "walk up" to follow to the root node. Technically speaking, just getting the previously active child is not useful for the purposes of acting as a part activation history in the compatibility layer as we do not actually always want the last active child there. For example, in 3.x, if we activate a view and then an editor and then close the editor, the focus stays in the editor area (if there's another editor to activate) instead of jumping to the view that we just clicked on. Of course, this isn't really a problem with the context, it just means I need to spin my own version. (In reply to comment #20) > Technically speaking, just getting the previously active child is not useful > for the purposes of acting as a part activation history in the compatibility > layer as we do not actually always want the last active child there. Actually, it's a problem even for regular e4 applications because say I have an active part in one stack and I activate another part in another stack. Then I try to close the part I just activated, I want to be able to know which was the last active part in that particular stack (instead of the entire application) so that I can set that particular part as the selected element of that stack. Created attachment 180199 [details] Patch IV - no magic (In reply to comment #21) > Actually, it's a problem even for regular e4 applications ... Yes, I see what you mean. I'll change it to a "no magic" version: - there is no activation history in the contexts; - the #activate() method gets a flag that either activates only only context, or activates the whole branch up to the root node. (Patch applied to CVS Head.) So activate can activate either that context in its parent context, or the entire parent chain? And deactivate will set the parent active child to null? PW (In reply to comment #23) > So activate can activate either that context in its parent context, or the > entire parent chain? Yes. > And deactivate will set the parent active child to null? Yes, but only if you are the active child. Otherwise it is a no-op. Created attachment 180276 [details]
Patch V - updated APIs
This should be closer to the final version: IEclipseContext:
public void activate(); <= altivates this context only
public void deactivate(); <= deactivates this context (if it was active)
public void activateBranch(); <= activates parent chain to the root
public IEclipseContext getActiveChild(); <= may return null
public IEclipseContext getActiveLeaf(); <= may return originating context
Created attachment 180285 [details]
Patch VI - removes uses of IContextConstants.ACTIVE_CHILD
The patch changes UI code to take advantage of the new APIs.
(I only updated immediate users of IContextConstants.ACTIVE_CHILD; most likely code can be further simplified using injection of "@Active" variables.)
Created attachment 180341 [details]
Patch VIb - removes uses of IContextConstants.ACTIVE_CHILD
Updated Patch IV; moved IContextConstants.ACTIVE_CHILD into internal EclipseContext.
Patch VIb applied to CVS Head. While I have no doubt that we'll have more adjustments to IEclipseContext, this seems to address the goal of this enhancement request. ======== Note on compatibility with Eclipse 4.0: IContextConstants.ACTIVE_CHILD was removed. Instead, the following convenience methods were added to IEclipseContext: public void activate(); public void deactivate(); public void activateBranch(); public IEclipseContext getActiveChild(); public IEclipseContext getActiveLeaf(); along with injection qualifier "@Active". Verified that ActivationInjectionTest's passed in I20101026-0100 (testContextActivation, testActivationInjection, testInjection). |