Bugzilla – Full Text Bug Listing
| Summary: | [UI] Design and implement a EPartService API | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] e4 | Reporter: | Remy Suen <remy.suen> | ||||||||||||||||||||||
| Component: | UI | Assignee: | Paul Webster <pwebster> | ||||||||||||||||||||||
| Status: | RESOLVED FIXED | QA Contact: | |||||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||||
| Priority: | P3 | CC: | bokowski, emoffatt, ob1.eclipse, pwebster, susan | ||||||||||||||||||||||
| Version: | 1.0 | ||||||||||||||||||||||||
| Target Milestone: | 1.0 RC0 | ||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||
| Whiteboard: | |||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Remy Suen
Eric and I talked briefly about this and he suggested that a "part service" be created for these calls. The PartService implementation also wrap the event publisher for any part related events. Boris agrees. I will begin working on this tomorrow. It will be similar to the API for 3.x's IWorkbenchPage. We will call it EPartService for now though we will want to rename it before 4.0. :) Before I forget (since I already forgot once and had to debug again), the NPEs in the setInput method is because the client object has been uninjected (and 'uiItem' has been reset to null). The reason this is _not_ observed when you actually run the photo demo and close it is because PRE's stop() method will simply dispose and unbind the widgets without disposing the context. I could easily just add a null check but it's not clear to me if this is the pattern I should be using. Seems strange from a client point of view but maybe that's just me? (In reply to comment #4) > Before I forget (since I already forgot once and had to debug again), the NPEs > in the setInput method is because the client object has been uninjected (and > 'uiItem' has been reset to null). The reason this is _not_ observed when you > actually run the photo demo and close it is because PRE's stop() method will > simply dispose and unbind the widgets without disposing the context. I could > easily just add a null check but it's not clear to me if this is the pattern I > should be using. Seems strange from a client point of view but maybe that's > just me? Damn, this should've been in bug 293500. Using recursion to implement findPart(String) is straightforward and all but what happens if there are two parts with identical ids? In 3.x it is not possible for the client to alter the id programmatically but we have public methods for doing so in e4. Or is this one of those "just don't do it" case? (In reply to comment #2) > The PartService implementation also wrap the event publisher for any part > related events. Eric, can you clarify this? Right now I can attach an event handler to listen for an element container's active child changing though that doesn't provide me with sufficient information as the part might've just been "brought to the top". I could use a RAT but that'd mean climbing down the active child chain which I'm not particularly enthusiastic about. Are you suggesting that a client should be able to attach listeners to the part service similar to the methods that 3.x's IPartService provides? Created attachment 153159 [details]
EPartService patch v1
Feel free to reimplement the interface completely. ;)
I think that the PartService should both subscribe to the UI model events (to handle the 'on top' cases by publishing a partVisible event) as well as having a RAT for the currently active part (to publish the partActive events). This puts the 'listeners' in the correct domain; the UI model manages the widget hierarchy and the context manages part 'activity'. (In reply to comment #9) > I think that the PartService should both subscribe to the UI model events (to > handle the 'on top' cases by publishing a partVisible event) as well as having > a RAT for the currently active part (to publish the partActive events). So we would subscribe to the UI model events so when a goes on top that we can publish the partVisible and partBroughtToTop? PW Created attachment 153273 [details]
EPartService v02
OK, I've updated your patch and the tests all run and pass.
Things I think we need to think about:
This uses the mediator pattern, an object per context that requests it, with very little state that's injected from the context (usually the context itself, but in this case the it's the closest MElementContainer).
This works when we get our EPartService from the window, but will fail when if we were to get it from our own part's context (since it would potentially see the stack model element, not the window).
I think we should revisit how we want this to work (and possibly some of the other services I have working as well).
PW
I've released a first cut at the find(*) API (specific MWindow implementation). Now we should look at the activate and show set of methods. PW Beauty, I might want to scam the 'find' logic once I start implementing the 'ModelService' since 'findById' is useful for elements other than parts. Once I get this in it might be worth changing the PartService's implementation to use it... I've released support for activate as well. Just need a few more tests. PW While Paul was implementing this, we've come to realize that the model needs to be "complete" when it's being rendered. That is, there needs to be a logical chain of "active child" from the application down to the part. If the model that's parsed and passed to the renderer does not abide by this contract then it is up to the renderer to compensate for this oversight. The renderer also needs to ensure that there is a valid context chain from top to bottom as many lookup functions (such as the ActivePartLookupFunction) relies on the context chain. OK, I've updated the headless test to create a valid activeChild chain and to not use the UIScheduler Setting the active part is a little trickier, as it has to be done through EPartService.activate(MPart) to ensure context and model chain is intact. PW Created attachment 153815 [details]
EPartServiceTest test additions v1
Paul, this test fails, I think it looks right, am I missing something?
(In reply to comment #17) > Created an attachment (id=153815) [details] > EPartServiceTest test additions v1 > > Paul, this test fails, I think it looks right, am I missing something? We had a chat offline and the test itself doesn't actually perform all the necessary steps to move a part from one window to another though the second window should certainly have a null active part (first point of failure). Hence, there is something wrong with both the EPS implementation and the attached test. (In reply to comment #18) > We had a chat offline and the test itself doesn't actually perform all the > necessary steps to move a part from one window to another though the second > window should certainly have a null active part (first point of failure). > Hence, there is something wrong with both the EPS implementation and the > attached test. I modified the test, ActivePartLookupFunction, and PartServiceImpl so they would work in this scenario (highlighting another wrinkle when specifying @Optional properties :-) PW In 3.x, IWorkbenchPage has a getWorkbenchWindow() method. Do we want a similar getElementContainer() method in the EPartService? MElementContainer may not be the best choice though. Another idea may be to introduce a new tag interface, MPartManager or whatever. Created attachment 155410 [details]
EPartService v03
I've changed the "root" from being model based to context based The root is added to MWindow contexts currently (since they all manage the parts). Lookup for the root might need to be added to work with the application level context for global lookups, but I don't have that usecase yet.
There are 2 "pre-processing" steps that will need to be considered when we work through the lifecycle enhancements. 1) the root needs to be added to all window contexts that exist when the model is being initialized and 2) as any new window context is created the root needs to be added to them (this is currently done by registering an EventHandler on the Context/context topic).
PW
(In reply to comment #21) > Created an attachment (id=155410) [details] > EPartService v03 Released to HEAD PW (In reply to comment #21) > Created an attachment (id=155410) [details] > EPartService v03 We need to record an activation list to know what part to activate if the activated one is closed/hidden/invisible/whatever-the-term-is. Since perspectives have contexts, would it make sense to make them the EPS root? (In reply to comment #23) > We need to record an activation list to know what part to activate if the > activated one is closed/hidden/invisible/whatever-the-term-is. Since > perspectives have contexts, would it make sense to make them the EPS root? And of course, the activation list is perspective-dependent and not window-dependent, hence I asked. And of course, not all e4 applications want a perspective. (In reply to comment #23) > We need to record an activation list to know what part to activate if the > activated one is closed/hidden/invisible/whatever-the-term-is. See bug 299038. Created attachment 155599 [details]
EPartService v04
I've copy/pasted Oleg's MPartDescriptor code from his ShowViewHandler over to the EPS so we now have a showView(String) method.
(In reply to comment #26) > Created an attachment (id=155599) [details] > EPartService v04 Released to HEAD. showPart(String) was always creating a new part. This is obviously wrong. Fix released to HEAD. Should EPS go in org.eclipse.e4.ui.services? (In reply to comment #29) > Should EPS go in org.eclipse.e4.ui.services? It might be possible to move it into its own bundle in the long term ... but not yet. It will have dependencies on the model service, which is our intersection of model operations and IEclipseContext operations (with possibly some more). PW The shown part does not become the active part. Will need to investigate. (In reply to comment #31) > The shown part does not become the active part. Will need to investigate. The code below only fails at the end. There may be something wrong with the injection mechanism. assertEquals(part, part.getContext().get(IServiceConstants.ACTIVE_PART)); assertEquals(part, window.getContext().get( IServiceConstants.ACTIVE_PART)); assertEquals(part, partService.getActivePart()); (In reply to comment #31) > The shown part does not become the active part. Will need to investigate. Actually, it looks like active(MPart) is broken in general. Note that this can only be observed if you actually use the SWT renderer. The headless renderer does not have this problem. Created attachment 155880 [details] EPartService activate bug (In reply to comment #33) > Actually, it looks like active(MPart) is broken in general. Note that this can > only be observed if you actually use the SWT renderer. The headless renderer > does not have this problem. I'm not sure if PartRenderingEngineTests is the best place but since it seems to be a problem with the renderer, I've thrown it in there. (In reply to comment #34) > Created an attachment (id=155880) [details] > EPartService activate bug The bug surfaces with the SWT renderer because it is a scheduling/notification problem, see bug 295003. Need to update showPart(String) to set the category to the auto-generated stack or else subsequent calls from other views (of the same category) will just keep spawning more stacks. :/ Created attachment 156933 [details] EPartService v05 (In reply to comment #36) > Need to update showPart(String) to set the category to the auto-generated stack > or else subsequent calls from other views (of the same category) will just keep > spawning more stacks. :/ 1. Fixes this problem. 2. Add getDirtyParts() returning Collection<MSaveablePart>. 3. Add getSaveableParts() returning Collection<MSaveablePart>. (In reply to comment #37) > Created an attachment (id=156933) [details] > EPartService v05 Released to HEAD. In activate(MPart), we can get a null context for the part if the part hasn't been rendered which makes the code that adjusts the context chain incorrect/invalid, not too sure what to do here. Remy, under what conditions would a non-rendered control ever become active? (In reply to comment #40) > Remy, under what conditions would a non-rendered control ever become active? I start Eclipse, my 'Console' view is behind my 'Problems' view, the 'Console' hasn't been rendered yet. I run an application, now the 'Console' view needs to be rendered and activated.
Sure,
MPart console = findPart("o.e.ui.ConsoleView");
// 'bring to top' -> forces rendering
console.getParent().setActiveChild(console);
<...activate code goes here now that it's rendered and on top>
The context/rendering code needs to be swapped, see bug 295250. Created attachment 157181 [details]
EPartService v06
This patch adds a new enum, PartState, specifying ACTIVATE, VISIBLE, and CREATE, which mirrors the 3.x version of our IWorkbenchPage API. Corresponding tests have been added.
The HCPE has been heavily modified because it was originally creating contexts for everything. This is of course not the case for our SWT renderer so I modified it to be "lazy" with regards to context creation.
How do we get the active editor? Do we depend on the model service? Or do we get it from the "activation service" (bug 300742)? If there's one editor area, then the active editor is the editor on top ... if we are tracking the last active editor (and a part activation list) then it would be the last active editor still open. If we were to maintain a navigation history (which is specific to editors) then on closing an editor we could walk back the navigation history ... but potentially not all editors participate in the current navigation history, right? PW The Model Service should talk only in 'data layer' terms such as MUIElement so that it's appropriate for any model definition. When you ask questions about 'Editor' or even 'Part' the correct place for these would be in the Part Service. So it'll have various types of 'find' as well as 'bringToTop' and a variety of 'insert' options (mirroring closely the API in IPageLayout). Whether we put 'activate' here or not will depend on what its final shape is I guess. We might even want to add a layer specific to the legacy concepts; the Part service would deal with 'MPart', activation...while the legacy service would deal with the non-e4 concepts like Views and Editors. (In reply to comment #44) > Created an attachment (id=157181) [details] > EPartService v06 Released to HEAD with some javadoc additions. In particular, I've added a @noreference to deactivate() given Brian's recent attempts to use this method (that we plan to delete). Created attachment 157513 [details] EPartService v07 The patch adds a new save(MSaveablePart, boolean) method, tests, and javadoc. To ensure that something was actually saved, I've modified MCPE to instantiate client objects (if there is one that is, see bug 299272). (In reply to comment #49) > Created an attachment (id=157513) [details] > EPartService v07 Released to HEAD. Created attachment 157542 [details]
EPartService v08
Implements saveAll(boolean) + javadocs + 252 corresponding tests. Released to HEAD.
Boris told me to think in terms of a DOM. So I tried.
The client is welcome to add the part by hand instead of going through the service but then if it's not a multi-instance views and bad things happen, that is not our problem. Using the 'showPart' APIs will also help take care of things like actually asking the renderer to render the part in addition to placing the part in the proper place (instead of some random spot in the window).
/**
* Creates a new part of the given id.
*
* @param id
* the identifier of the part, must not be <code>null</code>
* @return a new part of the given id, or <code>null</code> if no part descriptors can be found
* that match the specified id
*/
public MPart createPart(String id);
/**
* Shows a part with the identified by the given id. In the event that there are multiple parts
* with the specified id, the client is recommended to use {@link #getParts()} and iterate over
* the collection to find the interested part and invoke {@link #showPart(MPart, PartState)} on
* it.
* <p>
* The behavior of this method is dictated by the supplied state. If <code>ACTIVATE</code> is
* supplied, then the part is made visible and granted focus. If <code>VISIBLE</code> is
* supplied, then the part will be made visible but not given focus. If <code>CREATE</code> is
* supplied, then the part will be instantiated though its contents may not necessarily be
* visible to the end user.
* </p>
*
* @param id
* the identifier of the part, must not be <code>null</code>
* @param partState
* the desired state of the shown part to be in
* @return the shown part, or <code>null</code> if no parts or part descriptors can be found
* that match the specified id
*/
public MPart showPart(String id, PartState partState);
/**
* Shows the given part.
* <p>
* <ul>
* <li>If there cannot be multiple parts of this type and a part already exists, the already
* existing part will be shown and returned.</li>
* <li>If multiple parts of this type is allowed, then the provided part will be shown and
* returned</li>
* </ul>
* </p>
* <p>
* The behavior of this method is dictated by the supplied state. If <code>ACTIVATE</code> is
* supplied, then the part is made visible and granted focus. If <code>VISIBLE</code> is
* supplied, then the part will be made visible but not given focus. If <code>CREATE</code> is
* supplied, then the part will be instantiated though its contents may not necessarily be
* visible to the end user.
* </p>
*
* @param part
* the part to show
* @param partState
* the desired state of the shown part to be in
* @return the shown part
*/
public MPart showPart(MPart part, PartState partState);
The implementation of the ActivePartLookupFunction is incorrect.
IContextFunction's must not depend on elements outside of the context tree. However, it depends on MWindow/MApplication#getContext:
IEclipseContext current = window.getContext();
if (current == null) {
return null;
}
If the MApplication object is added to the context before its own context field is set, the ActivePartLookupFunction exist early and does not create dependency on the context(ACTIVE_CHILD).
See bug 299529 comment 9, and bug 303218.
I've introduced a new REMOVE_ON_HIDE_TAG to remove the part from the model when hidePart(MPart) is called. Now that we have tags we'll have to be careful that they dont' become a dumping ground for hacks...at some point (post EclipseCon) I'd recommend that we go over what we want to do with tags vs the model on a case by case basis. It's mostly there PW |