Community
Participate
Working Groups
I have an application in which I build dynamically the user interface based on some recipe. It is done by having perspectives defined as snippets and all parts in window's shared elements. The perspectives then are cloned from snippets and filled with placeholders dynamically. The placeholders reference to the parts in shared elements. One of the part needs to display all available parts, so basically it retrieves all MParts from shared elements and display them using MPart.getLocalizedLabel(). It all works fine if the parts haven't been attached to any placeholder yet or they are assigned by they are visible in the same perspective. If I try to get localized label of the part which previously was referenced by the placeholder, I get just key (so exactly the same as MPart.getLabel()). After debugging it seems that the problem is that 'Cur Shared Ref' of the part is never cleared even if corresponding placeholder is removed from the model (e.g. by EModelService.removePerspectiveModel()). LocalizationHelper.getLocalized() uses ModelUtils.getContainingContext(). That has a special if-case for parts attached to placeholders, but the context of the placeholder is null in my case. I have checked with both SWT renderers and JavaFX renderers. If I just manually set null to 'Cur Shared Ref', then retrieving localized label works fine. I tried on Juno SR1 release. Please also see discussion with Tom Schindl: https://www.eclipse.org/forums/index.php/t/953042/
For reference - i fixed this bug in the fx-renderers in the perspective code when a perspective selection is changed. The logic (and I think it is the same in the SWT renderer is): * detect the selection change * recursively walk the widget tree of the old selection hiding stuff * recursively walk the widget tree of the new selection makeing stuff visible The fix in the fx-renders is simply that when doing the recurive hideing i set the curRef to null and which in turn gets reset on the recursive makeing visible call afterwards
New Gerrit change created: https://git.eclipse.org/r/41337
I think I've found the solution for this issue. @Tom It would be great if you could review the change. I had a hard time finding the right position, since there is no SWT placeholder renderer.
To evaluate my patch, have a look at my translation example on GitHub https://github.com/fipro78/e4translationexample If you start the application there is a subwindow that shows a "Kill perspective" button. On click a perspective is killed and the also killed shared part shows the key instead of the translation in the part. My patch should solve this according to the discussion in the forum and the solution Tom found for JavaFX.
The patch seems to raise another issue with missing checks for curSharedRef == null in intro code. see Bug 463043 for further information
Gerrit change https://git.eclipse.org/r/41337 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=21a06ea739e3c429b7ba3a60d0254673d0cd0b5e
Thanks Dirk. Fixed for M7.
Probably this fix caused regression, see bug 463957.
New Gerrit change created: https://git.eclipse.org/r/45314
(In reply to Eclipse Genie from comment #9) > New Gerrit change created: https://git.eclipse.org/r/45314 Patch https://git.eclipse.org/r/41337/ introduced serious regression in IDE (bug 463962). Reverting commit 21a06ea739e3c429b7ba3a60d0254673d0cd0b5e (patch https://git.eclipse.org/r/45314) fixes bug 463962.
(In reply to Andrey Loskutov from comment #10) > (In reply to Eclipse Genie from comment #9) > > New Gerrit change created: https://git.eclipse.org/r/45314 > > Patch https://git.eclipse.org/r/41337/ introduced serious regression in IDE > (bug 463962). > > Reverting commit 21a06ea739e3c429b7ba3a60d0254673d0cd0b5e (patch > https://git.eclipse.org/r/45314) fixes bug 463962. So you simply remove my contribution? Why not checking the cause for the NPE? Looking into the code this line is responsible for the NPE if (selectedPart.getParent().getRenderer() != null) { So selectedPart.getParent() is null. Shouldn't that be checked before going further?
(In reply to Dirk Fauth from comment #11) > (In reply to Andrey Loskutov from comment #10) > > (In reply to Eclipse Genie from comment #9) > > > New Gerrit change created: https://git.eclipse.org/r/45314 > > > > Patch https://git.eclipse.org/r/41337/ introduced serious regression in IDE > > (bug 463962). > > > > Reverting commit 21a06ea739e3c429b7ba3a60d0254673d0cd0b5e (patch > > https://git.eclipse.org/r/45314) fixes bug 463962. > > So you simply remove my contribution? Not yet, but I tend to do it. If you have a fix for bug 463962 I will be glad *not* to do it, but bug 463962 is a showstopper IMHO. > Why not checking the cause for the NPE? The problem is that the NPE seems to be NOT the root cause, it is another manifestation of the same problem. I'm still trying to figure out how to stable reproduce the issue. Sometimes steps described in bug 463962 are not enough, but it looks like reverting your change fixes the issue.
No the problem is that we already know that we are a MPlaceholder, but checking this information again by looking into the curSharedRef. I'll work on a solution, give me some time.
(In reply to Dirk Fauth from comment #13) > No the problem is that we already know that we are a MPlaceholder, but > checking this information again by looking into the curSharedRef. > > I'll work on a solution, give me some time. Dirk, thanks for quick response. Is this https://git.eclipse.org/r/45317/ the *final* fix, or are you still debugging? The fix seems to work for me.
(In reply to Andrey Loskutov from comment #14) > (In reply to Dirk Fauth from comment #13) > > No the problem is that we already know that we are a MPlaceholder, but > > checking this information again by looking into the curSharedRef. > > > > I'll work on a solution, give me some time. > > Dirk, thanks for quick response. Is this https://git.eclipse.org/r/45317/ > the *final* fix, or are you still debugging? The fix seems to work for me. From my point of view it should be the *final* fix, at least if it works for you. :-)
New Gerrit change created: https://git.eclipse.org/r/45333
Here are the problematic locations I've found: StackRenderer#createTab assumed the placeholder has a ref; I hit this problem while tracking down a different problem and can't remember how I created it :-( WorkbenchPage#showView(VIEW_VISIBLE) assumes parts shown EPartService#showPart() will have a curSharedRef, though the PartServiceImpl#bringToTop has checks in case the curSharedRef is null. I haven't been able to trigger this code path ever occurs in practice, so it's a defensive measure. MinMaxAddon: several places uses the curSharedRef without checking for null. But in my testing, this code assumes minimization and maximization only happens in the active perspective (i.e., setting 'Maximized' on part stacks in other perspectives fails in a spectacular manner). This is a bigger problem, so I've submitted this as bug 463985.
Follow up on https://dev.eclipse.org/mhonarc/lists/platform-ui-dev/msg06164.html The failures below appeared after integration of https://git.eclipse.org/r/41337/ (commit 21a06ea739e3c429b7ba3a60d0254673d0cd0b5e, bug 457939). Hope this helps. testBug334580_01(org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests) junit.framework.AssertionFailedError at junit.framework.Assert.fail(Assert.java:55) at junit.framework.Assert.assertTrue(Assert.java:22) at junit.framework.Assert.assertTrue(Assert.java:31) at junit.framework.TestCase.assertTrue(TestCase.java:201) at org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests.testBug334580_01(PartRenderingEngineTests.java:2290) testRemoveGuiBug324228_2(org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests) junit.framework.AssertionFailedError: expected:<PerspectiveImpl (perspectiveA) Context> but was:<PerspectiveImpl (perspectiveB) Context> at junit.framework.Assert.fail(Assert.java:57) at junit.framework.Assert.failNotEquals(Assert.java:329) at junit.framework.Assert.assertEquals(Assert.java:78) at junit.framework.Assert.assertEquals(Assert.java:86) at junit.framework.TestCase.assertEquals(TestCase.java:253) at org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests.testRemoveGuiBug324228_2(PartRenderingEngineTests.java:1149) testRemoveGuiBug324228_3(org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests) junit.framework.AssertionFailedError: expected:<PerspectiveImpl (perspectiveA) Context> but was:<PerspectiveImpl (perspectiveB) Context> at junit.framework.Assert.fail(Assert.java:57) at junit.framework.Assert.failNotEquals(Assert.java:329) at junit.framework.Assert.assertEquals(Assert.java:78) at junit.framework.Assert.assertEquals(Assert.java:86) at junit.framework.TestCase.assertEquals(TestCase.java:253) at org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests.testRemoveGuiBug324228_3(PartRenderingEngineTests.java:1208) testBug327701(org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests) junit.framework.AssertionFailedError: expected:<PerspectiveImplContext> but was:<null> at junit.framework.Assert.fail(Assert.java:57) at junit.framework.Assert.failNotEquals(Assert.java:329) at junit.framework.Assert.assertEquals(Assert.java:78) at junit.framework.Assert.assertEquals(Assert.java:86) at junit.framework.TestCase.assertEquals(TestCase.java:253) at org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests.testBug327701(PartRenderingEngineTests.java:1440) testRemoveGui_Bug332163(org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests) junit.framework.AssertionFailedError: expected:<PerspectiveImplContext> but was:<PerspectiveImplContext> at junit.framework.Assert.fail(Assert.java:57) at junit.framework.Assert.failNotEquals(Assert.java:329) at junit.framework.Assert.assertEquals(Assert.java:78) at junit.framework.Assert.assertEquals(Assert.java:86) at junit.framework.TestCase.assertEquals(TestCase.java:253) at org.eclipse.e4.ui.tests.workbench.PartRenderingEngineTests.testRemoveGui_Bug332163(PartRenderingEngineTests.java:1725)
New Gerrit change created: https://git.eclipse.org/r/45688
Gerrit change https://git.eclipse.org/r/45688 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3bc3891284f93ae7f0a43a941ed8325d03ad5613
Dirk, I backed out the change to LazyStackRenderer as it was resulting in test failures and doesn't seem necessary with your E4 translation test case.
(In reply to Brian de Alwis from comment #21) > Dirk, I backed out the change to LazyStackRenderer as it was resulting in > test failures and doesn't seem necessary with your E4 translation test case. Hi Brian, thanks for looking into this. I just pulled the latest changes and can confirm that with your modification the translation still works as intended by my initial bugfix. Greez, Dirk P.S. Shouldn't you update the copyright headers too?
Tested with my example application https://github.com/fipro78/e4translationexample and it looks good.
Tested with Eclipse 4.5.0 Integration Build: I20150428-0100 and it looks good.
