Community
Participate
Working Groups
Created attachment 264182 [details] Demo RCP client demonstrating the problem The most intuitive approach to open a part programmatically is an example handler like the following one: public class BuggyShowViewHandler { public static final String VIEWS_SHOW_VIEW_PARM_ID = "buggy.views.showView.viewId"; //$NON-NLS-1$ @Execute public void execute(EPartService partService, @Named(VIEWS_SHOW_VIEW_PARM_ID) String viewId) { partService.showPart(viewId, PartState.ACTIVATE); } } Alas, in a multi-perspective RCP client where perspective A has a shared element part X which is already opened, but the user attempts to call EPartService#showPart(String, PartState) within perspective B (which doesn't contain part X at the moment), it will create a new X part instance even if the "allowMultiple" property is defined as "false". This severely breaks an important invariant of shared elements. The problem seems to be related to bug #491994. The attached demo client demonstrates the problem: 1) Start the RCP client (e.g. using the associated .product file 2) Press toolbar entry "Buggy Show" and notice the console output, which might be: @PostConstruct of ID=demo.showpart.multiple.perspectives.samplePart [Perspective A] (1367612102) 3) Press toolbar entry "Perspective B", which should be empty. 4) Again, press "Buggy Show" and notice the complete console output: @PostConstruct of ID=demo.showpart.multiple.perspectives.samplePart [Perspective A] (1367612102) @PostConstruct of ID=demo.showpart.multiple.perspectives.samplePart [Perspective B] (623446986) The different hash values show that now two different instances of the sample part exist at the same time. Please note that the same broken code can be found in org.eclipse.e4.ui.internal.workbench.swt.handlers.ShowViewHandler, which uses exactly the same showPart overload. Before attempting to make a fix suggestion, I would like to point out that the problem seems to start with the internal usage in PartServiceImpl of MPart part = findPart(id); which effectively calls modelService.findElements(workbenchWindow, id, cls, null, EModelService.OUTSIDE_PERSPECTIVE | EModelService.IN_ACTIVE_PERSPECTIVE | EModelService.IN_SHARED_AREA); This flag combination is equivalent to EModelService.PRESENTATION ("Searches for elements in the UI that the user is currently seeing (excluding trim)") The first question raises, why this search is restricted to the part of the UI that the user is seeing instead of e.g. EModelService.ANYWHERE? In a multi-perspective program naturally a previously opened part in another perspective cannot be seen, but should be found. Assuming that the initial search criteria were intended, the following logic surely is not, because if findPart(id) returns null, we enter MPartDescriptor descriptor = modelService.getPartDescriptor(id); part = createPart(descriptor); which unconditionally creates the new instance and thus causes the invariant breakage for non-multiple shared element parts. I noticed that from Eclipse 4.5.2 on the legacy handler org.eclipse.ui.handlers.ShowViewHandler contains a different code, without relating this change to any bugzilla bug number in the source file, so the motivation for this change is unclear. Ignoring all the legacy related stuff for the moment, the essence of that code is different from the one shown above: public class FixedShowViewHandler { public static final String VIEWS_SHOW_VIEW_PARM_ID = "fixed.views.showView.viewId"; //$NON-NLS-1$ @Execute public void execute(EPartService partService, @Named(VIEWS_SHOW_VIEW_PARM_ID) String viewId) { MPart part = partService.findPart(viewId); if (part == null) { MPlaceholder placeholder = partService.createSharedPart(viewId); part = (MPart) placeholder.getRef(); } partService.showPart(part, PartState.ACTIVATE); } } Note that the function calls EPartService#showPart(MPart, PartState) instead after the possible creation of a placeholder (instead of a part) in case of a failing findPart call (Note that findPart still considers only the current PRESENTATION instead of ANYWHERE). Indeed, if we repeat all reproducer steps above (Please restart the RCP client before attempting to do so!) but always press "Fixed Show" instead of "Buggy Show", the console output demonstrates that only a single part instance is created as it should be. While FixedShowViewHandler provides a workaround, it doesn't solve the fundamental problem caused by EPartService#showPart(String, PartState). A high level API service such as EPartService should never allow to break the one-instance guarantee of non-multiple shared element parts. Assuming that the internal search criteria using PRESENTATION instead of ANYWHERE of EPartService#findPart(String) are as intended, this should be documented. The existing documentation reads as if there is no constraint for this search. Under that assumption it seems that the fix for EPartService#showPart(String, PartState) should be the replacement of the lines MPartDescriptor descriptor = modelService.getPartDescriptor(id); part = createPart(descriptor); if (part == null) { return null; } by MPlaceholder placeholder = partService.createSharedPart(viewId); part = (MPart) placeholder.getRef(); The latter one should further be improved to be robust against a failing placeholder construction: MPlaceholder placeholder = partService.createSharedPart(viewId); if (placeholder != null) { part = (MPart) placeholder.getRef(); } Please provide guidance before I make a concrete patch suggestion.
Daniel, see bugs 471782 and bug 483699 for some more context on the change we made on org.eclipse.ui.handlers.ShowViewHandler. I think Brian is the one who knows best how the current model / part services work, so I hope he can help you with the right direction for the fix.
(In reply to Andrey Loskutov from comment #1) > Daniel, see bugs 471782 and bug 483699 for some more context on the change > we made on org.eclipse.ui.handlers.ShowViewHandler. > > I think Brian is the one who knows best how the current model / part > services work, so I hope he can help you with the right direction for the > fix. This issue exists now since 2016 awaiting feedback for a possible direction of a fix. Some guidance is very much appreciated - Thanks!
(In reply to Daniel Kruegler from comment #2) > (In reply to Andrey Loskutov from comment #1) > > Daniel, see bugs 471782 and bug 483699 for some more context on the change > > we made on org.eclipse.ui.handlers.ShowViewHandler. > > > > I think Brian is the one who knows best how the current model / part > > services work, so I hope he can help you with the right direction for the > > fix. > > This issue exists now since 2016 awaiting feedback for a possible direction > of a fix. Some guidance is very much appreciated - Thanks! You have to decide on you own risk, as it seem that no one with knowledge in this area has time to answer and those who has time has no knowledge :-). A Gerrit with some tests added would surely help to move forward.
(In reply to Andrey Loskutov from comment #3) > (In reply to Daniel Kruegler from comment #2) > > This issue exists now since 2016 awaiting feedback for a possible direction > > of a fix. Some guidance is very much appreciated - Thanks! > > You have to decide on you own risk, as it seem that no one with knowledge in > this area has time to answer and those who has time has no knowledge :-). > > A Gerrit with some tests added would surely help to move forward. Could you please give me a pointer to where I should add the tests to? Does there exists a special git repo to clone?
(In reply to Daniel Kruegler from comment #4) > (In reply to Andrey Loskutov from comment #3) > > (In reply to Daniel Kruegler from comment #2) > > > This issue exists now since 2016 awaiting feedback for a possible direction > > > of a fix. Some guidance is very much appreciated - Thanks! > > > > You have to decide on you own risk, as it seem that no one with knowledge in > > this area has time to answer and those who has time has no knowledge :-). > > > > A Gerrit with some tests added would surely help to move forward. > > Could you please give me a pointer to where I should add the tests to? Does > there exists a special git repo to clone? The tests are in the same repo as platform ui code, the possible bundle for your tests would be org.eclipse.e4.ui.tests. I was surprised to see that there are no tests which would work with EPartService interface, feel free to add some.
(In reply to Andrey Loskutov from comment #5) > The tests are in the same repo as platform ui code, the possible bundle for > your tests would be org.eclipse.e4.ui.tests. I was surprised to see that > there are no tests which would work with EPartService interface, feel free > to add some. I think the EPartService tests are in ./org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/application/EPartServiceTest.java @Daniel, all tests are located in the "test" folder.
(In reply to Lars Vogel from comment #6) > (In reply to Andrey Loskutov from comment #5) > > The tests are in the same repo as platform ui code, the possible bundle for > > your tests would be org.eclipse.e4.ui.tests. I was surprised to see that > > there are no tests which would work with EPartService interface, feel free > > to add some. > > I think the EPartService tests are in > ./org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/application/ > EPartServiceTest.java > > @Daniel, all tests are located in the "test" folder. Thanks Lars for the hint! It seems that importing org.eclipse.e4.ui.tests alone is not sufficient to make the tests contained within that bundle to run. I guess I need further dependencies for this bundle, because all current tests fail with an NPE in E4Workbench#getServiceContext(). I see a package import error regarding org.eclipse.test.performance, could that be the reason? If so, could someone help me finding the git repo containing the associated bundle?
(In reply to Daniel Kruegler from comment #7) > (In reply to Lars Vogel from comment #6) > > (In reply to Andrey Loskutov from comment #5) > > > The tests are in the same repo as platform ui code, the possible bundle for > > > your tests would be org.eclipse.e4.ui.tests. I was surprised to see that > > > there are no tests which would work with EPartService interface, feel free > > > to add some. > > > > I think the EPartService tests are in > > ./org.eclipse.e4.ui.tests/src/org/eclipse/e4/ui/tests/application/ > > EPartServiceTest.java > > > > @Daniel, all tests are located in the "test" folder. > > Thanks Lars for the hint! It seems that importing org.eclipse.e4.ui.tests > alone is not sufficient to make the tests contained within that bundle to > run. I guess I need further dependencies for this bundle, because all > current tests fail with an NPE in E4Workbench#getServiceContext(). I see a > package import error regarding org.eclipse.test.performance, could that be > the reason? If so, could someone help me finding the git repo containing the > associated bundle? I followed the discussion in Bug 358773 and in Bug 389737. There was a tip that says one should simply install the "test framework". I found something like this via "Install New Software" but the install itself didn't change anything. What else need I do to get the tests run?
(In reply to Daniel Kruegler from comment #8) > (In reply to Daniel Kruegler from comment #7) > I followed the discussion in Bug 358773 and in Bug 389737. There was a tip > that says one should simply install the "test framework". I found something > like this via "Install New Software" but the install itself didn't change > anything. What else need I do to get the tests run? I'm more and more confused about how the tests are supposed to work. I see now that when I run the "JUnit Test" context menu command for the complete org.eclipse.e4.ui.tests project, some tests(including EPartServiceTest) succeed but many (most?) tests fail. If I run the EPartServiceTest *alone*, all tests of this class fail. This doesn't give me much confidence into how the tests are supposed to work, but at least I can try to make progress here.
(In reply to Andrey Loskutov from comment #1) > Daniel, see bugs 471782 and bug 483699 for some more context on the change > we made on org.eclipse.ui.handlers.ShowViewHandler. I looked again at these issues, but they seem not related to the specific problem that I reported, since my example is a pure e4 application and does not depend on e3 legacy behaviour. The point that I'm trying to make is that a part whose associated part descriptor is configured for allowMultiple=false should never cause the EPartService to violate that invariant for a given window. Can someone help me deciding which criterion is sufficient to decide for the showPart function to decide to prefer partService.createSharedPart over partService.createPart? I'm asking because when I apply my suggested fix as suggested, other unit tests such as testShowPart_Id_CREATE4 regarding the assertion assertEquals(part, stack.getChildren().get(0)); The worst situation would be that showPart cannot decide without an additional parameter whether it needs to call createPart or createSharedPart. Opinions?
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. As such, we're closing this bug. If you have further information on the current state of the bug, please add it and reopen this bug. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant. -- The automated Eclipse Genie.
(In reply to Eclipse Genie from comment #11) > This bug hasn't had any activity in quite some time. Maybe the problem got > resolved, was a duplicate of something else, or became less pressing for > some reason - or maybe it's still relevant but just hasn't been looked at > yet. As such, we're closing this bug. > > If you have further information on the current state of the bug, please add > it and reopen this bug. The information can be, for example, that the > problem still occurs, that you still want the feature, that more information > is needed, or that the bug is (for whatever reason) no longer relevant. > I think it is a shame, that bugs are simply closed automatically. In my last comment I had asked for help, but no response. This is IMO an extremely demotivating policy for part-time contributors.
(In reply to Daniel Kruegler from comment #12) > (In reply to Eclipse Genie from comment #11) > > This bug hasn't had any activity in quite some time. Maybe the problem got > > resolved, was a duplicate of something else, or became less pressing for > > some reason - or maybe it's still relevant but just hasn't been looked at > > yet. As such, we're closing this bug. > > > > If you have further information on the current state of the bug, please add > > it and reopen this bug. The information can be, for example, that the > > problem still occurs, that you still want the feature, that more information > > is needed, or that the bug is (for whatever reason) no longer relevant. > > > > I think it is a shame, that bugs are simply closed automatically. In my last > comment I had asked for help, but no response. This is IMO an extremely > demotivating policy for part-time contributors. Please reopen and provide a suggested Gerrit so that others can have a look. Lots of developers are unpaid (including me) so without code it is hard to give advice.
