Community
Participate
Working Groups
To reproduce: open an inner, open an editor, make it the active part shut down and restart Note that the Package Explorer's stack indicates that it's the active part rather than the one that was active on the shutdown. Also, even though active the Tree doesn't have focus (it's actually on the Package Explorer stack's minimize button... We should be to at least determine the part that was active on shutdown by following the MApplication's 'selectedElement' chain down to either an MPart or MPlaceholder. Alternatively it may be OK (better?) to check if we can activate an open Editor. This will surely be noticed by folks...
Created attachment 197740 [details] Patch with an initial cut at using the model's 'selectedElement' info
(In reply to comment #2) > Created attachment 197740 [details] > Patch with an initial cut at using the model's 'selectedElement' info I can confirm that this patch fixes the problem although it introduces a test failure. When the perspective switching code was implemented I didn't want perspective switches to cause that window to become the active window as it didn't seem like such an operation should affect which part was the active part. It seems in 3.x switching perspectives of a window in the background will actually activate it and bring it to the foreground. I don't know if this is the behaviour we want in Eclipse 4 or not.
Created attachment 197800 [details] EPartService patch v2 This new patch removes the check so that the test no longer fails. I also removed the extra code that was added in PartActivationHistory as I am not comfortable with making changes in that code so late in the cycle.
I'm OK with the patch as far as it goes but I think that we really should at least try to re-activate the part that was active on shutdown since it's an obvious flaw if we don't. I do respect your concerns about changes in important code area like activationHistory...so I'll attach a patch with the 'selectedElement' code in it and we'll let McQ make the call...;-).
Created attachment 197806 [details] Remy's patch with code to attempt to re-active the last active part
Remy I've +1'd your patch, could you take another look at mine (both to ensure that it contains your changes as well as the new code in PAH)? The only real change from my original code is that I now fall through with null should the part that I find be TBR == false for whatever reason...
It should be noted that the approach taken in the code in the PAH will *not* find the last active part if it was in a Detached window but will instead fall through to the old code...(at least we tried...;-).
(In reply to comment #6) > Created attachment 197806 [details] > Remy's patch with code to attempt to re-active the last active part If you minimize a view first and then have it activated and then go 'File > Restart', you will get the focus on a tool item when the workbench comes up again.
kk, McQ's fine with your approach so we'll just skip mine (for now, we should revisit this post 4.1...)
-1 for putting this in for 4.1, we can think about it a bit more and do the right thing in 4.1.1
Sure, no need to review my patch in that case...
Created attachment 201339 [details] New patch using tags to identify the active part
(In reply to comment #13) > Created attachment 201339 [details] > New patch using tags to identify the active part When I run the tests I get a test failure. junit.framework.AssertionFailedError: expected:<org.eclipse.e4.ui.model.application.ui.basic.MWindow eclipse context> but was:<org.eclipse.e4.ui.model.application.ui.basic.MWindow eclipse context> at junit.framework.Assert.fail(Assert.java:47) at junit.framework.Assert.failNotEquals(Assert.java:283) at junit.framework.Assert.assertEquals(Assert.java:64) at junit.framework.Assert.assertEquals(Assert.java:71) at org.eclipse.e4.ui.tests.application.EPartServiceTest.testSwitchPerspective05(EPartServiceTest.java:10160)
The active part doesn't come back if you have multiple workbench windows it seems.
Created attachment 201346 [details] New patch that activates using the old PartService code Remy, this makes the test suite happy... Why do we seem to have so many different (and subtle) ways to active something ??
OK, second try...
(In reply to comment #16) > Created attachment 201346 [details] > New patch that activates using the old PartService code > > > Remy, this makes the test suite happy... Confirmed on my end. > Why do we seem to have so many different (and subtle) ways to active something > ?? That code is to prevent an inactive window switching perspectives from becoming the active window. (In reply to comment #15) > The active part doesn't come back if you have multiple workbench windows it > seems. Please open a new bug for this issue if you don't intend to have it covered by this bug. Otherwise, the code looks fine to me. I am not sure about declaring the tag as API. Do you expect clients to use this tag?
Eric, should this still be considered for 4.1.2? PW
As I understand, this is still not part of 4.2. I am not sure whether I understand the effect of this bug, but my pure e4 application is loosing SWT events: If an MPart is dynamically created from an MPartDescriptor, the created composite receives the focus through the framework, i,e, one doesn't have to call composite.setFocus() oneself, neither in buildUI() nor in the @Forus method. In this case everything works as expected. When the application is restarted, this MPart is fully visible, but neither the @Focus method nor the composite.setFocus() are called. Hence, the first SWT event, i.e. a click on a button in the composite, gets lost (I mean that, e.g., the SelectionListeners are not called). This first click only effects, that the @Focus method and the composite.setFocus() are called. Hence, one has to click twice on the button for, e.g., the SectionListeners to be called once. This is quite counterintuitive.
(In reply to comment #20) > When the application is restarted, this MPart is fully visible, but neither the > @Focus method nor the composite.setFocus() are called. Hence, the first SWT > event, i.e. a click on a button in the composite, gets lost (I mean that, e.g., > the SelectionListeners are not called). This first click only effects, that the > @Focus method and the composite.setFocus() are called. Hence, one has to click > twice on the button for, e.g., the SectionListeners to be called once. This is > quite counterintuitive. I just pushed a change to master, Karl. It fixes a problem where the @Focus is not called on startup. It may or may not solve the problem for you. If your application is still not working with this change, please open a new bug. http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=5926c3e628bd808c130379a1bcfd17322a729528
It is a major usability issue if one does not see the focus after restart and if one can e.g. not continue typing in the editor one was in when exiting. Can you please consider this for 4.2?
I agree with Dani, very inconvenient. I'm not sure but I think that would also impact Mylyn. Adding Steffen.
*** Bug 378645 has been marked as a duplicate of this bug. ***
commit e332177f7a8176558b5324bcadfff1475a42a25d (in pwebster/start_421) Works for all cases where the part is in the main WBW's presentation. Doesn't work if the active part is in a minimized stack or a DW yet...
Search seem to have problems with focus too, maybe the following bug is related: Bug 384023
*** Bug 384023 has been marked as a duplicate of this bug. ***
Also see Bug 329458...editors should get precedence over views on startup...
*** Bug 328703 has been marked as a duplicate of this bug. ***
The initial fix introduced by Eric has been later 'broken' by changes provided as fixes for two bugs: 1. bug 340695 - class org.eclipse.ui.internal.CoolBarToTrimManager 2. bug 375576 - class org.eclipse.jface.text.TextViewer The changes are independent and both (separately) breaks Eric's fix. The breaking change from the bug 340695 has been already fixed in bug 416746. Now I am trying to figure out whether the change introduced in the bug 375576 is the real cause of the problem or it only uncovered a problem located somewhere else.
Note: while working on this make sure to check that it correctly handles the initial state of split editors (bug 423894)...
It turns out that the reason of the problem is similar to the one in bug 375576. When opening a shell in WBWRenderer, the layout of the shell is outdated because of multiple deferred layouts. As a result, the shell is being detected by SWT/OS as invisible, thus focus cannot restored. To force update of the layout I revert all deffered layouts and immediatelly restore them - the same fix is applied in the bug 375576. As mentioned in the bug 375576, the root cause of the problems is extensive use of deferred layouts and optionally can be 'fixed' in SWT (BTW. is there any bug open for this issue?) Review URL: https://git.eclipse.org/r/22302
Nice one Wojciech ! I've pushed this change: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=5479fbcb2d0fc973f1279341eb87570555a12c8c The only thing we may want to do is to either remove the 'static' from the 'forceLayout' method or make it public (in case others need to force a layout), what do you think ?
+1 for removing static. The fix is a kind of workaround, so I think the method should not be public. Also, I wonder if restoring the reverted deferred layout (the second loop) should be done after opening the shell. I will investigate it.
Please do investigate...I haven't seen anything bad with the new code so I feel pretty sure that it's OK but if the 'deferred' count goes up whenever we use a layout call with SWT.DEFERRED my guess is that it should go down once that layout has actually been done (meaning that we might be winding the stack up too much). This might be worth making a SWT app based on one of the samples and having it be able to do both 'setlayoutDeferred' calls *and* having some composites that get their layouts with SWT.DEFER. Might be best to invent you own layout as well so that you can see when it actually runs... Does it run when the deferred count goes to 0 ?
I agree that the fix can get some more polish, but wanted to mention that it works in N20140225-2000.
Thanks Dani... I'll remove the 'static' and mark this one fixed. Wojciech, I've removed the 'static': http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=70990c140dfbcbba1f2bbf94840e795b48095f1c Thanks for the fix :) I'll mark this as fixed...if we discover anything bad about the unwinding / winding the 'deferred' state we'll re-open to fix it here...
It is worth to mention that the bug did not occur with non-text editors (at least some of them). Forcing layout in the fix for the bug 375576, uncovered this bug.
Verified in I20140303-2000.
The fix seems to cause another bug 431966. I am not sure where it should be fixed in, but would appreciate if you could take a look!
