Bugzilla – Full Text Bug Listing
| Summary: | EPartService needs a cleanup | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Oleg Besedin <ob1.eclipse> | ||||||||
| Component: | UI | Assignee: | Platform UI Triaged <platform-ui-triaged> | ||||||||
| Status: | RESOLVED WORKSFORME | QA Contact: | |||||||||
| Severity: | major | ||||||||||
| Priority: | P3 | CC: | bokowski, emoffatt, Lars.Vogel, ob1.eclipse, pwebster, remy.suen | ||||||||
| Version: | 4.2 | ||||||||||
| Target Milestone: | --- | ||||||||||
| Hardware: | All | ||||||||||
| OS: | All | ||||||||||
| Whiteboard: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Oleg Besedin
(In reply to comment #0) > (1) the EPartService instances are created almost semi-randomly on different > contexts and yet they interact with each other via propagation of active > children. This makes any scenario involving EPartService, at best, very hard to > debug, or unpredictable at worst. I was trying to address this problem last week for the ESS by collapsing the code and introducing a slave server. EPS is next on my list. I can throw slave services in the ContributedPartRenderer but that seems like a horrible idea. Eric suggested that an addon should be handling this. Oleg and Paul, what do you think? > (2) the EPartService has 22 methods and a number of constants. Most of the > methods are used less than 3 times by the SDK. I guess when you say "SDK" you mean the 4.x org.eclipse.ui.workbench bundle. I think one generic reason for the low usage count is that we just have a few APIs that delegate to the EPS. Of course, everyone else just calls the 3.x APIs so there really aren't any direct references to the EPS in the 4.x workbench bundle. > addPartListener 1 > removePartListener 1 I'd like to remove these for 4.1 in favour of the event broker if possible. > deactivate() 0 Gonna delete this for sure. > isPartVisible() 1 Not sure how useful this is. > createPart 1 > createSharedPart 1 > createSharedPart(force) 2 Need to look at these again (bug 320578). > getInputParts 0 Tom wanted this. Even if bug 305477 is fixed, I am not convinced it will be used in the 4.x workbench bundle. > (3) combination of *changing* PART_SERVICE_ROOT computed function with multiple > interacting EPartService is way way way too complicated for what the service > supposed to do. Am hoping to address this when I collapse the code like what I'm trying to do with the ESS. > say #getParts() is used in some places to get parts in the > current window I'll have to check this. If there's a perspective, it should only return the ones that are in that perspective. > and in some other places it seems that the intenion was to get > all parts in the application. This is obviously wrong. Wherever this code is should be reexamined. The "application" bit brings up a good point though. Should there be an EPS for the application? Should application.getContext().get(EPartService.class) return a non-null value? Probably not, right? But, I believe handlers are executed from the application's context...so, if the preceding statement returns 'null', you cannot inject an EPS as a parameter in your @Execute method. This is a huge problem. (In reply to comment #1) > But, I believe handlers are executed > from the application's context...so, if the preceding statement returns 'null', > you cannot inject an EPS as a parameter in your @Execute method. This can be observed when saving contacts in the contacts demo. (In reply to comment #1) > I can throw slave services in the ContributedPartRenderer but that seems like a > horrible idea. Eric suggested that an addon should be handling this. Actually, this would require explicit opt-in which sounds wrong. I'll have to check with Eric to ensure I understood him correctly and that the two of us were on the same page when we were having this conversation. (In reply to comment #1) > I was trying to address this problem last week for the ESS by collapsing the > code and introducing a slave server. EPS is next on my list. > I can throw slave services in the ContributedPartRenderer but that seems like a > horrible idea. Eric suggested that an addon should be handling this. Oleg and > Paul, what do you think? I think that the central problem here comes from having multiple part services. It is not like part services stores any useful data. Logically, part service operates on a model, not on a sub-tree. For the current code: A number of EPS methods don't need the "root node" at all (and for some, like #saveAll() it is counter-intuitive). There are several methods that use "root node" implicitly, like #bringToTop() - those should be handled using active child chains, or have a version that says #bringToTop(in_this_window). Methods like #getParts() would be working on an application level, and will have an alternative version #getParts(root). And yes, we do need part services at the application level - say, to get all views, or to bring up a part without having to consult active child chain. So, I would suggest: - remove part service PartServiceCreationFunction - place EPartService implementation in the application context - all standard methods on EPS should have application scope - where needed, the version of methods taking in a "root part" (or, rather, workbench window?) can be added (In reply to comment #1) > I guess when you say "SDK" you mean the 4.x org.eclipse.ui.workbench bundle. Those numbers are for all e4 UI bundles. I agree that those numbers should not be a single deciding factor; I just wanted to point out that there seem to be a small number of widely used methods and large number of methods that see little use. Remy and myself dicussed this and, after going through some use cases, agreed that we need to have an EPartService at the application level, with some methods having a version to take a "root" model element. We just had a conversation with Eric and McQ on whether we want to have workbench windows interacting with each other or not, which caused me to reconsider comment 5. In the 3.x, workbench windows act as separate entities and, generally speaking, don't communicate with each other. Having a single application-level part service would cause them to communicate. For example, if a user asks to open a view and it is already opened in another workbench window (WW2), the view will be brought to top in WW2, rather then a new view opened in WW1. In my opinion, this might be a good thing, and a step to merge the three types of windows we have (workbench window, detached window, and modeless dialogs), however, it does not seem that we have any end-user requests for this. It would most certainly affect workflows for people using multiple WWs. Would that change be percieved as good or bad is hard to say. So, the new plan is to have EPartService at the workbench window level. (In reply to comment #6) > Having a single application-level part service would cause them to communicate. > For example, if a user asks to open a view and it is already opened in another > workbench window (WW2), the view will be brought to top in WW2, rather then a > new view opened in WW1. I was actually thinking about this scenario again today. Oleg, will you still have an EPS at the application level or will it be exclusive to windows? (In reply to comment #7) > Oleg, will you still have an EPS at the application level or will it be > exclusive to windows? From the practical side, keybindings will stop working on simple apps (such as contacts demo) if we don't have EPS at the aplication level. Also, adding EPS to MWindow's context when that context is created does not work either due to the timing problem with the selection service :-(. It seems that the only way to proceed without breaking something else is to add EPS to the application context and workbench window contexts. Created attachment 176711 [details]
Patch I - let's have fewer EPS instances
This patch:
- removes auto-creation function for EPartService
- creates EPS on the application and workbench windows
- removed EPS dependency on EPartService.PART_SERVICE_ROOT
The advantages are limited numbers of EPS instances created (1 per app + 1 per workbench window) and simplified behaviour (the root container is now "final" for the EPS).
(In reply to comment #9) > Created an attachment (id=176711) [details] > Patch I - let's have fewer EPS instances Why does the MWindow instance need to be set in the model again in WorkbenchWindow.java? If the presentation engine is @Optional in PartServiceImpl, it will throw NPEs in showPart(PartState, MPart, MPart). Created attachment 176916 [details]
Patch I v.2 - let's have fewer EPS instances
This patch takes a different approach:
- the EPS is only created for MWindow's
- if EPS is asked for on the Application context, it returns implementation for the active window (if none available, for the first window)
- if EPS is asked for on any low-level context (such as Part or Part stack), we walk up until we hit an implmentation on the MWindow level
Because contexts are created by the PartRenderingEngine and there is no good place to specify services available per model element, this makes somewhat complicated EPS creation function. I'll follow up on that angle later.
The end result is that we have one EPS instance per workbench window and no dependency on EPartService.PART_SERVICE_ROOT.
(In reply to comment #11) > The end result is that we have one EPS instance per workbench window I don't think this is actually true because the patched PSCF implementation only climbs up to the first MWindow instead of climbing to the top (MApplication). MWindows can have nested mwindows (getWindows()). Introducing EPSes for perspectives can come later. Created attachment 176927 [details] Patch I v.3 - let's have fewer EPS instances (In reply to comment #12) > (In reply to comment #11) > > The end result is that we have one EPS instance per workbench window > I don't think this is actually true because the patched PSCF implementation > only climbs up to the first MWindow instead of climbing to the top > (MApplication). MWindows can have nested mwindows (getWindows()). Good point, I changed the code to climb to the top-most MWindow to bypass detached windows. (In reply to comment #11) > Because contexts are created by the PartRenderingEngine and there is no good > place to specify services available per model element, this makes somewhat > complicated EPS creation function. I'll follow up on that angle later. Opened bug 323075. Applied patch I v.3; keeping this bug open. We know people want to do things with perspectives that they can't right now due to how IWorkbenchPage is implemented (see bug 57841 and bug 102268). We can introduce EPartServices at a perspective level, that's technically feasible, but I feel that it will introduce a source of confusion due to how DI works. Consider someone that @Injects an EPS into their POJO for a part and that part is in the inactive perspective. If they try to show/hide a part, does it actually make sense to the developer for the part to be shown/hidden in the inactive perspective? Would they not expect the part to be shown/hidden in the currently active perspective? Likewise, what should happen when someone tries to activate a part that's not in the currently active perspective? Should it be a no-op or should the part be explicitly shown in the active perspective? Ultimately, the question becomes how can we distinguish someone that wants to be manipulating inactive perspectives and someone who just wants to do stuff in the active perspective? (In reply to comment #16) > Ultimately, the question becomes how can we distinguish someone that wants to > be manipulating inactive perspectives and someone who just wants to do stuff in > the active perspective? I'm beginning to wonder if we should introduce an EPerspectiveService. We can then keep EPartService simple and EPerspectiveService will handle the "odd" cases. This will also help make the division between whether stuff should "just happen" or if a particular perspective is being manipulated explicitly. (In reply to comment #11) > - if EPS is asked for on the Application context, it returns implementation > for the active window (if none available, for the first window) This means that no one can actually "cache" the EPartService that's been queried from the application's context since the active window is dynamic. This is the cause behind bug 326177. *** Bug 367976 has been marked as a duplicate of this bug. *** AFAICS there is nothing concrete to be done left in this bug. |