Community
Participate
Working Groups
In a new workspace, when I open the "customize perspective" dialog, some toolbar (items) disappear. For instance, the save items and editor navigation items.
This is a regression in 4.13 (4.12 works), and the buttons stay lost until restart. Rolf, do you plan to work on this?
Regression from bug 378495, https://git.eclipse.org/r/145577 http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d367093508dbaab34123d19f555ed5c94f289178
This must be fixed for M3 or the fix for bug 378495 reverted AGAIN.
Indeed, Bug 378495 exposes this behavior. However, the original problem is present for a long time. Apparently with no real visible bugs. With the model spy, it can be seen that some of the toolbar items are moved from the main window toolbars to the CPD window. Before Bug 378495, this results in inconsistency between the model and the ToolbarManager. Later the items seem to re-appear in the model, but at a different location than original. As of the fixes of Bug 378495, the model en ToolbarManager are in sync. As a result the items disappear from the UI, which is the correct behavior if the UI elements are removed. Root cause of the issue is related to the assumption in ToolbarManager and CoolBarToTrimManager that there is a one-to-one mapping between contribution items and model items. This assumption is violated in CustomizePerspectiveDialog. CustomizePerspectiveDialog#loadMenuAndToolbarStructure calls WorkbenchWindow#fillActionBars with the ActionBarAdvisor.FILL_PROXY flag. In turn ActionBarAdvisor#fillActionBars is called, with the following documentation: * If <code>flags</code> does include <code>FILL_PROXY</code>, then this is a * request to describe the actions bars of the given workbench window (which * will already have been filled); again, the remaining flags indicate which * combination of the menu bar, the tool bar, and the status line are to be * described. The actions included in the proxy action bars can be the same * instances as in the actual window's action bars. When customizeActionBars.coolBarManager.update and toolbarMngrRenderer.reconcileManagerToModel are called in CustomizePerspectiveDialog the one-to-one mapping between contribution item and model item is broken. And existing model elements are moved to another toolbar. If only the CPD was querying the E4-model directly, instead of all this complex processing, see also Bug 516111.
(In reply to Dani Megert from comment #3) > This must be fixed for M3 or the fix for bug 378495 reverted AGAIN. -1 for revert of Bug 378495 (see also Rolfs explanation why the fix is correct). I have at least one RCP customer which required the fix done in Bug 378495.
(In reply to Lars Vogel from comment #5) > (In reply to Dani Megert from comment #3) > > This must be fixed for M3 or the fix for bug 378495 reverted AGAIN. > > -1 for revert of Bug 378495 (see also Rolfs explanation why the fix is > correct). I have at least one RCP customer which required the fix done in > Bug 378495. Sure, I also prefer a correct fix but it must be done for M3.
(In reply to Dani Megert from comment #6) > Sure, I also prefer a correct fix but it must be done for M3. A fix for the now visible problem is definitely desired. But if not possible for M3 we have to discuss which one is worse. So far no user has complained about the now visible bug in CPD. And Bug 378495 is definitely important for at least one customer.
(In reply to Lars Vogel from comment #7) > So far no user has complained about the now visible bug in CPD. For a simple reason: there aren't much users tried the M1 build. BTW I've complained - I'm using Eclipse every day so I'm the valid user :-) > And Bug > 378495 is definitely important for at least one customer. And the CPD for the rest of the world (N customers). Is N > 1? Is your customer more important than mine? Can we agree that breaking existing functionality to fix another bug is not a valid deal / the way we want to develop here, independently for whom we do the fix?
(In reply to Andrey Loskutov from comment #8) > (In reply to Lars Vogel from comment #7) > BTW I've complained - I'm using Eclipse every day so I'm the valid user :-) It sounded that you only found out about this bug, after Rolf reported it. So at least one milestone without affecting you even though you use Eclipse every day.
And the repeat the most important point: the fix of Bug 378495 is correct. The bug is in existing code not in the fix for Bug 378495.
I do agree that this bug is annoying, otherwise I would not have reported it. There is a quick fix, to check in ToolbarManager or CoolBarToTrimManager that the existing model-element is not moved to a new parent, and if it would to create a new model-element. But I have no clue yet what the possible side-effects of that change are. However I rather like to query the model directly, and fix Bug 516111 at the same time. This doesn't look too complex either. Though I have to choose were to spend my limited time on.
> Can we agree that breaking existing functionality to fix another bug is not a valid deal / the way we want to develop here, independently for whom we do the fix? +1 to the above statement. But this is not the case here. We have the special case that the existing code is buggy. So in this particular case we would have to discuss if we want to reintroduce more buggy code to hide existing bugs. I'm -1 for that, other PMC members might think different. As Rolf plan to work on the real bug, I hope we do not have to decide on the above.
(In reply to Lars Vogel from comment #9) > (In reply to Andrey Loskutov from comment #8) > > (In reply to Lars Vogel from comment #7) > > BTW I've complained - I'm using Eclipse every day so I'm the valid user :-) > > It sounded that you only found out about this bug, after Rolf reported it. > So at least one milestone without affecting you even though you use Eclipse > every day. Because I usually setup my workspace *once* (disable debug toolbar etc), and I had no need to create a new workspace recently. My customer however does this, and I don't want to manually revert this commit from SDK just to be able to push 4.13 to the customer.
(In reply to Andrey Loskutov from comment #13) > Because I usually setup my workspace *once* (disable debug toolbar etc), and > I had no need to create a new workspace recently. My customer however does > this, and I don't want to manually revert this commit from SDK just to be > able to push 4.13 to the customer. I completely understand your point of view. If this bug cannot be fixed. 1.) Hiding a bug from the UI and introducing another bug 2.) Making an exiting bug visible to the user Both options are IMHO bad. The PMC will have to decide what is worse.
(In reply to Lars Vogel from comment #14) > (In reply to Andrey Loskutov from comment #13) > > > Because I usually setup my workspace *once* (disable debug toolbar etc), and > > I had no need to create a new workspace recently. My customer however does > > this, and I don't want to manually revert this commit from SDK just to be > > able to push 4.13 to the customer. > > I completely understand your point of view. > > If this bug cannot be fixed. > > 1.) Hiding a bug from the UI and introducing another bug > 2.) Making an exiting bug visible to the user > > Both options are IMHO bad. The PMC will have to decide what is worse. I stand by comment: either we get a fix or workaround, or it must be reverted. I would not be surprised if it affects other parts than just CPD.
(In reply to Dani Megert from comment #15) > > Both options are IMHO bad. The PMC will have to decide what is worse. > I stand by comment: either we get a fix or workaround, or it must be > reverted. I would not be surprised if it affects other parts than just CPD. If this bug cannot be fixed, this must be decided by the PMC as committers disagrees here about the correct fix. It is not up to you (or me) to decide.
(In reply to Rolf Theunissen from comment #11) > I do agree that this bug is annoying, otherwise I would not have reported it. > > There is a quick fix, to check in ToolbarManager or CoolBarToTrimManager > that the existing model-element is not moved to a new parent, and if it > would to create a new model-element. But I have no clue yet what the > possible side-effects of that change are. Rolf, could you provide a Gerrit please? > However I rather like to query the model directly, and fix Bug 516111 at the > same time. This doesn't look too complex either. Though I have to choose > were to spend my limited time on. The time is also limited by M3 deadline (this Friday if I'm not mistaken?).
(In reply to Andrey Loskutov from comment #17) > The time is also limited by M3 deadline (this Friday if I'm not mistaken?). Code should be in by end of this week. M3 for the platform is August 23.
(In reply to Dani Megert from comment #18) > (In reply to Andrey Loskutov from comment #17) > > The time is also limited by M3 deadline (this Friday if I'm not mistaken?). > Code should be in by end of this week. M3 for the platform is August 23. IIRI in 4.12 Rc1 was the feature freeze. Why M3 for this fix?
(In reply to Lars Vogel from comment #19) > (In reply to Dani Megert from comment #18) > > (In reply to Andrey Loskutov from comment #17) > > > The time is also limited by M3 deadline (this Friday if I'm not mistaken?). > > Code should be in by end of this week. M3 for the platform is August 23. > > IIRI in 4.12 Rc1 was the feature freeze. Why M3 for this fix? Because this would give us more time to test the fix. As we learned that each version revealed a new issue/regression.
I have been trying to use the E4 model as primary source for the CPD. What was missing are the inactive action sets. Action sets are only added to the E4 model when activated. However, the action sets are not removed from the model when deactivated they are only hidden in the model. I have a solution in which all the action sets are initialized in the model at startup. Then the CPD can use the E4 model as source. This seem to work for toolbars, I want to do some more testing. I plan to push a Gerrit for review tonight. Menu's stay out of sync (in general they are in worse shape than toolbars), they keep using the old mechanisms for now.
New Gerrit change created: https://git.eclipse.org/r/147731
Gerrit change https://git.eclipse.org/r/147731 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=bcbbd47882496408306792cc02d9f7ffd3a28ad5
Thanks a bunch Rolf for the fix and Andrey for the review. Super nice to get into a consistent usage of the application model.
Bad news. We have a regression again. We've found it "by occasion". We have "old" Subclipse plugins in our product that define such action set. The actions use selection listeners to track enablement. The actions have a bug - they do not properly dispose themselves. This was not noticed before, because they are defined in an initially "invisible" action set. Now, with the addition of line 878 in WorkbenchWindow getActionPresentation().setActionSets(registry.getActionSets()); we seem to uncover this bug because now we unconditionally initialize and instantiate code from (previously hidden) action sets that are defined as "visible=false", which causes all the code in such extensions to be created and initialized. In our case this causes lot of NPE's during tests, but more severe impact is that we now instantiate code on workbench startup (in UI thread) that shouldn't be instantiated at all. This is both performance / functional issue, because the code not only slows down startup but also can cause other "unexpected" functional problems, including bundle activation chains that were not existing before this change.
(In reply to Andrey Loskutov from comment #25) > Bad news. We have a regression again. @Rolf, any proposals for the fix? @Lars, @Dani: we should think what do we plan for M3! 1) DO NOTHING: bug 378495 and this one (side effect of the fix for bug 378495) are fixed but we have new regression - all "invisible" actions sets are now instantiated at startup. 2) reverting change https://git.eclipse.org/r/147731 only => bug 378495 and "invisible actions sets are now instantiated at startup" are fixed but Customize Perspectives Dialog is non-functional (this bug). 3) reverting original change from bug 378495 and change for this bug (https://git.eclipse.org/r/145577 and https://git.eclipse.org/r/147731) => part Toolbar does not refresh when toolbar items are removed, not other known side effects. 4) Someone fixes new regression ""invisible" actions sets are now instantiated at startup" and we hope there is no new side effect. "Part Toolbar does not refresh when toolbar items are removed" (bug 378495) is IMO lower severity compared with "Customize Perspectives Dialog is non-functional" or "invisible actions sets are instantiated at startup". Unless we have solution for 4) yet I vote for 3), since it has lower severity compared with 1) or 2) and we know there are no other known side effects because it is broken since 2012.
Lets see what Rolfs says. Maybe we can only add the visible action sets in getActionPresentation().setActionSets(registry.getActionSets())?
(In reply to Andrey Loskutov from comment #26) > (In reply to Andrey Loskutov from comment #25) > > Bad news. We have a regression again. > > @Rolf, any proposals for the fix? The real fix (option 4) for this would be to have actions-sets added directly to the model, instead of using E3 API and reconciling it into the model. In this way, the actions-sets are not initialized before used. However, this seems not feasible for the 4.13 release. 5) Initialize all actions sets when the CPD is opened, this moves the issues from startup to opening the CPD. Issue here might be to get the enabled actionsets correct. 6) Revert this change and implement a solution/workaround based on my first experiment, make ToolbarManagerRenderer and/or CoolBarToTrimManager aware that there is a one-to-many relation between contribution items and model items. Main thing to resolve here is to prevent memory leaks or early disposals. I think 6) is feasible, though not sure about the time frame that is left to do this work.
(In reply to Rolf Theunissen from comment #28) > (In reply to Andrey Loskutov from comment #26) > > @Rolf, any proposals for the fix? > 5) Initialize all actions sets when the CPD is opened, this moves the issues > from startup to opening the CPD. Would be OK from my perspective.
(In reply to Andrey Loskutov from comment #29) > (In reply to Rolf Theunissen from comment #28) > > (In reply to Andrey Loskutov from comment #26) > > > @Rolf, any proposals for the fix? > > 5) Initialize all actions sets when the CPD is opened, this moves the issues > > from startup to opening the CPD. > > Would be OK from my perspective. +1
Andrey, just to confirm, if you make the broken actionSets visible in the CPD before the changes in Bug 378495 and this one, you would also receive the described errors?
(In reply to Lars Vogel from comment #31) > Andrey, just to confirm, if you make the broken actionSets visible in the > CPD before the changes in Bug 378495 and this one, you would also receive > the described errors? Yes. Means, the bug I see in Subversive plugin was always there and would be visible also before our changes here if they would have they action sets visible by default. Now, after the change https://git.eclipse.org/r/147731 we see this bug always because it triggers loading of classes that were never loaded before :-(.
New Gerrit change created: https://git.eclipse.org/r/147925
(In reply to Andrey Loskutov from comment #25) > We have "old" Subclipse plugins in our product that define such action set. > The actions use selection listeners to track enablement. The actions have a > bug - they do not properly dispose themselves. Actually there were few bugs in *Subversive* plugin (I'm always mix it with Subclipse), but also one from platform that I've reported now via bug 549898 (I have a patch for this too).
(In reply to Andrey Loskutov from comment #34) I've reported now via bug 549898 > (I have a patch for this too). Bug reference seem wrong, point to this bug
(In reply to Lars Vogel from comment #35) > (In reply to Andrey Loskutov from comment #34) I've reported now via bug > 549898 > > (I have a patch for this too). > > Bug reference seem wrong, point to this bug I meant bug 550222.
Gerrit change https://git.eclipse.org/r/147925 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3a10fb831c2d74e43b13194b52c78e9f8108a8f6
(In reply to Eclipse Genie from comment #37) > Gerrit change https://git.eclipse.org/r/147925 was merged to [master]. > Commit: > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3a10fb831c2d74e43b13194b52c78e9f8108a8f6 > Is this the final fix and can we close this bug?
(In reply to Dani Megert from comment #38) > (In reply to Eclipse Genie from comment #37) > > Gerrit change https://git.eclipse.org/r/147925 was merged to [master]. > > Commit: > > http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=3a10fb831c2d74e43b13194b52c78e9f8108a8f6 > > > > Is this the final fix and can we close this bug? Yes, we still have a regression - see bug 550299 , but I think it is something we can live with for *now*.
