Community
Participate
Working Groups
Created attachment 226696 [details] Small project for visible state refresh not working As reported here http://www.eclipse.org/forums/index.php/t/452550/ core expressions are not working on HandledToolItems or DirectToolItems. Even changing the visibility flag at runtime doesn't rerender the toolbar or menu bar correctly. I have extended and reattached the toolbar example I created to show this issue. If you start the app you will see the "Debug Menu" menu item for the main menu. It shouldn't be visible though, because the core expression evaluates false. If you select "Toggle Menu Item" in the "Item Test Menu" two things should apply. 1. The item "Core Expression visible" should appear in the "Item Test Menu" - works 2. An additional item should become visible in the toolbar of the part. Note that even setting the visible flag directly to that toolitem doesn't rerender correctly. This can be tested by clicking the visible toolitem in the example, which calls a command that changes the visible flag of the second toolitem.
Update: I tested this with M6 and it is still not working. I ensured that the HandlerProcessingAddon is added prior testing with M6, so this doesn't seem to be the issue.
I face the same issue with eclipse 4.2 but I have succeed to make it work by adding my HandledToolItems to a toolbox contribution. But when I add my items directly to the toolbox the core expression is not evaluated.
I am facing the same problem with Eclipse 4.3.1.
I think this is a dup of Bug 391430
Hi, this ticket is closed and marked as fixed. But IMHO it is not fixed. I tried to test it with the Luna Release and the described issues still exist. Issue 1: The "Debug Menu" entry is visible in the main menu. But the core expression evaluates to false, so it shouldn't be visible. Issue 2: If you select "Toggle Menu Item" in the "Item Test Menu" two things should apply. 1. The item "Core Expression visible" should appear in the "Item Test Menu" 2. An additional item should become visible in the toolbar of the part. While 1. worked before, 2. still doesn't work. There is no item shown in the toolbar of the project. The issues still appear in the example app I attached. Is the example code wrong and does it need to be modified so the evaluation of core expressions are correctly handled? And now I even see a new issue. The toolitems in the part are never shown. Not even the one without a visibile-when expression. Is an addon missing? There is also no Target Milestone set. So I wonder, is the status wrong and this issue isn't fixed? If it is fixed, how can the example application I attached be modified so all the use cases work correctly? If it is just closed because it is a duplicate, it should be marked as such and not as fixed.
I still think it might be a dup of Bug Bug 391430, but lets keep this one open too.
It might be a duplicate, but int that case the state should be CLOSED DUPLICATE and not CLOSED FIXED ;)
I think I've found a solution. Not sure about side effects with the compat layer. In my example e4 application on GitHub everything seems to work as intended now. https://github.com/fipro78/e4toolbarexample To trigger core expression evaluation the UIEvent UIEvents.REQUEST_ENABLEMENT_UPDATE_TOPIC needs to be fired. From what I was told, this is the way to go since the periodic refresh was disabled.
New Gerrit change created: https://git.eclipse.org/r/42706
Gerrit change https://git.eclipse.org/r/42706 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=d3b82d5be4d5f42886501d912de163d2db5980e8
fixed with above patch
(In reply to Dirk Fauth from comment #11) > fixed with above patch Awesome Dirk, thanks. Can you add this to the N&N? As committer you can directly push or release a Gerrit review for it.
(In reply to Lars Vogel from comment #12) > (In reply to Dirk Fauth from comment #11) > > fixed with above patch > > Awesome Dirk, thanks. Can you add this to the N&N? As committer you can > directly push or release a Gerrit review for it. Do we really add bugfixes to the N&N? I always thought that new features or improvements are part of N&N, not bugfixes.
(In reply to Dirk Fauth from comment #13) > (In reply to Lars Vogel from comment #12) > > (In reply to Dirk Fauth from comment #11) > > > fixed with above patch > > > > Awesome Dirk, thanks. Can you add this to the N&N? As committer you can > > directly push or release a Gerrit review for it. > > Do we really add bugfixes to the N&N? No.
(In reply to Dani Megert from comment #14) > > Do we really add bugfixes to the N&N? > No. We typically add "bugfixes" to the N&N which are of high interest for customers. Remember the buzz about fixing "Customize Perspective"? In this case I think this is a new functionality that several customers are interested in so we should promote it. But I leave it to Dirk if he wants to add it.
Sorry, but I had to revert this with http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=4b69ab7360caf132ab71d986f3825689bf74a267 It causes lots of unexpected bundle loading and also lets our test fail that checks for unexpected bundle loading: http://download.eclipse.org/eclipse/downloads/drops4/I20150721-0800/testresults/html/org.eclipse.jdt.text.tests_linux.gtk.x86_64_8.0.html Wrong bundles loaded: - org.eclipse.debug.ui - org.eclipse.jdt.debug - org.eclipse.jdt.debug.ui - org.eclipse.ui.console junit.framework.AssertionFailedError: Wrong bundles loaded: - org.eclipse.debug.ui - org.eclipse.jdt.debug - org.eclipse.jdt.debug.ui - org.eclipse.ui.console at org.eclipse.jdt.text.tests.PluginsNotLoadedTest.testPluginsNotLoaded(PluginsNotLoadedTest.java:265) at junit.extensions.TestDecorator.basicRun(TestDecorator.java:23) at junit.extensions.TestSetup$1.protect(TestSetup.java:23) at junit.extensions.TestSetup.run(TestSetup.java:27) at org.eclipse.test.EclipseTestRunner.run(EclipseTestRunner.java:692) at org.eclipse.test.EclipseTestRunner.run(EclipseTestRunner.java:319) at org.eclipse.test.UITestApplication$2.run(UITestApplication.java:197) at org.eclipse.swt.widgets.RunnableLock.run(RunnableLock.java:35) at org.eclipse.swt.widgets.Synchronizer.runAsyncMessages(Synchronizer.java:135) at org.eclipse.swt.widgets.Display.runAsyncMessages(Display.java:3794) at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:3433) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$4.run(PartRenderingEngine.java:1127) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336) at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1018) at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:156) at org.eclipse.ui.internal.Workbench$5.run(Workbench.java:654) at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:336) at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:598) at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:150) at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:139) at org.eclipse.test.UITestApplication.runApplication(UITestApplication.java:140) at org.eclipse.test.UITestApplication.run(UITestApplication.java:62) at org.eclipse.test.UITestApplication.start(UITestApplication.java:212) at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:196) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:134) at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:104) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:380) at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:235) at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:669) at org.eclipse.equinox.launcher.Main.basicRun(Main.java:608) at org.eclipse.equinox.launcher.Main.run(Main.java:1515) at org.eclipse.equinox.launcher.Main.main(Main.java:1488) at org.eclipse.core.launcher.Main.main(Main.java:34) Here is an example contribution from 'org.eclipse.debug.ui' who surfaces one of those problems: <menuContribution locationURI="menu:org.eclipse.ui.run?after=breakpointGroup"> <menu id="breakpointTypes" label="%BreakpointTypesMenu.label"> <visibleWhen checkEnabled="false"> <and> <with variable="org.eclipse.core.runtime.Platform"> <test property="org.eclipse.core.runtime.bundleState" args="org.eclipse.debug.ui" value="ACTIVE"/> </with> <with variable="activeContexts"> <iterate operator="or"> <equals value="org.eclipse.debug.ui.breakpointActionSet"/> </iterate> </with> <systemTest property="org.eclipse.debug.ui.breakpoints.toggleFactoriesUsed" value="true"> </systemTest> </and> </visibleWhen> <dynamic id="org.eclipse.debug.ui.actions.BreakpointTypesContribution" class="org.eclipse.debug.ui.actions.BreakpointTypesContribution"> </dynamic> </menu> </menuContribution> Under normal circumstances (say e.g. in the Eclipse SDK), that menu will never be shown because of the toggleFactoriesUsed system property that is set to 'false'. With the wrong commit, this menu is now evaluated (but not shown), which causes the unexpected loading of 'org.eclipse.debug.ui.actions.BreakpointTypesContribution' and hence the Debug UI plug-in.
Furthermore, this commit also caused duplicated toolbar items in I20150721-0800. Steps: - new workspace - paste to Package Explorer: package p; public class C { public static void main(String[] args) { System.out.println(1); System.out.println(2); } } - set a breakpoint on the "println(2)" line. - Run > Debug (F11) - confirm switching to Debug perspective - Run > Debug (F11) again - Run > Debug (F11) a third time => most items in the Console view toolbar are duplicated now
And more fallout to check with the next fix attempt: The "Window > Editor > ..." submenu showed two items called "Split Editor". In 4.5 and after the reversal of this bug's commit, they were called "Toggle Split Editor" (bug 472806).
So this is another episode of "The compat layer strikes back" I thought about this for a while, and I have some questions because I don't really understand everything in detail. @Dani You say that the menu for which the systemTest returns false is now evaluated and wasn't before. Well, in fact that is what this patch intends. From the application model point of view this is exactly what should happen. The behavior before to not re-evaluate is wrong. I am thinking about this for a while now, and it is hurting my brain. Maybe because the assumptions and concepts behind the core expressions and the expected bundle loading are not clear to me in detail. If I understand you correctly org.eclipse.debug.ui should not be loaded because a systemTest core expression returns false. Therefore you assume that in such a case there should be no re-evaluation performed. This leads to the following questions: a) Why should the plugin not be loaded? It contributes a menu item which seems to get inspected. Otherwise how is the systemTest evaluated if the plugin isn't loaded? How can something be inspected that isn't loaded? b) From your statement the assumption is that if a visibleWhen core expression contains a systemTest that returns false, the contribution should not happen. From the application model point of view this means the contribution to the application model should not be performed. I wonder how this should work when not cleaning the persisted state but changing the system property. c) You statement assumes that systemTest should only be evaluated on startup and never again. But system properties can change at runtime via System.getProperties().setProperty(). This is fairly uncommon, but possible. So what is the semantic of systemTest? Is it intended that it only evaluates once and never again? In such a case the core expression evaluation needs to be changed somehow (and try to deal with b), maybe overridden to always return false or evaluate the core expression before it is transformed to the application model? Sorry for all those question, but IMHO my commit is correct from the application model point of view (plain Eclipse 4). But it raises issues with the compat layer. So hopefully with your help I will understand the concepts behind the plugin loading mechanism and the systemTest semantic. After that I will think about the other issues Markus found. It seems they are related on how the compat layer is dealing with dynamic menu contributions, but one step after the other, so my next "attempt" is better and also works with the compat layer.
Looking at the ConsoleView, well the toolbar is still created using actions. And AFAIK actions are deprecated for years. Maybe the ConsoleView should be updated to use commands & handlers instead of programmatically added actions? Just an idea to bring the migration forward.
(In reply to Dirk Fauth from comment #19) > @Dani > You say that the menu for which the systemTest returns false is now > evaluated and wasn't before. Sorry Dirk, I think I was not precise enough when I said "this menu is now evaluated". Of course it is evaluated each time the menu updater runs. However, since the <visibleWhen> expression is 'false', the menu is not shown, and hence, it is wrong and unnecessary to load BreakpointTypesContribution (which causes the plug to be started). This happens with your code changes. At a later point the system property might be 'true' and the Debug UI plug-in might have been loaded (first condition for the menu to become visible). In that case org.eclipse.debug.ui.actions.BreakpointTypesContribution should be loaded and used. Re-evaluation of the menu condition is desired and does happen fine in 4.5, e.g. via the menuUpdater in WorkbenchWindow. Maybe that already answered most of your questions. > a) Why should the plugin not be loaded? For performance and memory reasons as long as the real code of a bundle is not used. That's a fundamental part of the Eclipse plug-in architecture. > It contributes a menu item which > seems to get inspected. Otherwise how is the systemTest evaluated if the > plugin isn't loaded? How can something be inspected that isn't loaded? That's the whole idea behind the core expressions: allow to express as much as possible in the 'plugin.xml' file to avoid/defer the loading of the full plug-in. In this case there is a property tester which can do the test without loading Debug UI.
(In reply to Dani Megert from comment #21) > (In reply to Dirk Fauth from comment #19) > > @Dani > > You say that the menu for which the systemTest returns false is now > > evaluated and wasn't before. > > Sorry Dirk, I think I was not precise enough when I said "this menu is now > evaluated". Of course it is evaluated each time the menu updater runs. > However, since the <visibleWhen> expression is 'false', the menu is not > shown, and hence, it is wrong and unnecessary to load > BreakpointTypesContribution (which causes the plug to be started). This > happens with your code changes. > > At a later point the system property might be 'true' and the Debug UI > plug-in might have been loaded (first condition for the menu to become > visible). In that case > org.eclipse.debug.ui.actions.BreakpointTypesContribution should be loaded > and used. > > Re-evaluation of the menu condition is desired and does happen fine in 4.5, > e.g. via the menuUpdater in WorkbenchWindow. > > Maybe that already answered most of your questions. > > > > a) Why should the plugin not be loaded? > > For performance and memory reasons as long as the real code of a bundle is > not used. That's a fundamental part of the Eclipse plug-in architecture. > > > > It contributes a menu item which > > seems to get inspected. Otherwise how is the systemTest evaluated if the > > plugin isn't loaded? How can something be inspected that isn't loaded? > > That's the whole idea behind the core expressions: allow to express as much > as possible in the 'plugin.xml' file to avoid/defer the loading of the full > plug-in. In this case there is a property tester which can do the test > without loading Debug UI. Thanks for the explanation. So to repeat if I understand correctly: a) the bundle is loaded but not started. I guess that was the misunderstanding on my side, because it is of course loaded but not activated. correct? b) the re-evaluation is done in a menuUpdater in the compat layer. That means it works in compat mode, but not in plain e4. So a solution might be to have a similar behavior in the model as in this menuUpdater. which than should also fit better in the whole plugin architecture. This should help in finding a more suitable solution for both cases (compat and app model).
(In reply to Dirk Fauth from comment #22) > Thanks for the explanation. So to repeat if I understand correctly: > > a) the bundle is loaded but not started. I guess that was the > misunderstanding on my side, because it is of course loaded but not > activated. correct? In terms of OSGi speak, the bundle is not ACTIVE yet, it's in the STARTING state (see 'org.osgi.framework.Bundle' for more details on the different bundle states). Most of our bundles have Bundle-ActivationPolicy: lazy which means, the bundle gets activated when one of its classes gets loaded. Many bundles have an activator, e.g. Bundle-Activator: org.eclipse.debug.internal.ui.DebugUIPlugin and activating the bundle loads that class and calls #start(BundleContext) on it. > b) the re-evaluation is done in a menuUpdater in the compat layer. That > means it works in compat mode, but not in plain e4. So a solution might be > to have a similar behavior in the model as in this menuUpdater. which than > should also fit better in the whole plugin architecture. > > This should help in finding a more suitable solution for both cases (compat > and app model). The tricky part will be to make sure that you don't update the same item twice every time.
Mass move to 4.7 as M7 is approaching. Please move back in case you are planning to fix it for Neon.
Is there any progress on this? I also tried with @Evaluate Expression but even this seems to be not called by E4...
(In reply to Christoph Laeubrich from comment #25) > Is there any progress on this? I also tried with @Evaluate Expression but > even this seems to be not called by E4... No progress on this yet. I have the intention to start working on the visibility story. However, given the limited (spare) time I have, I was not able to start on it yet.
@Rolf that sound good, let me know if you need some help on this topic. If you are working on this, you maybe can consider Bug 538623 also?
(In reply to Christoph Laeubrich from comment #27) > @Rolf that sound good, let me know if you need some help on this topic. > > If you are working on this, you maybe can consider Bug 538623 also? The visibility story by itself is already complex enough, considering dynamic changing visibility in the model, visibility expressions, legacy/opaque items. At least a dozen bugs are open on these topics. I will not consider extending the functionality until the basics (finally) work.
@Rolf: No problem, I started to work on that mentioned ticket myself and noticed something that might be helpful: At least the 'Imperative Expression' (haven't checked Core Expressions yet but the code seems to not distinguish between the both) can be made to work in the following way: - create a Toolbar - add a direct Toolitem and Imperative Expression and add for example a simple system.out --> Nothing happens, method is not called - now create a fragment in another plugin and add an "Application Toolbar Contribution" (extend Application NOT Toolbar!) with the Toolbar id as the parent of the previous created Toolbar - add a direct Toolitem and Imperative Expression and add for example a simple system.out --> Method is called! This shows that ToolBarManagerRenderer.processContribution(MToolBar, String) do the necessary setup for contributions but not for its own items! Relaxing the interfaces for ToolBarContributionRecord a bit and calling the ToolBarManagerRenderer.processAddition(...) for each ModelChild all my Imperative Expressions are called as expected.
There is no complex issue why the expressions are not working, as you noticed. It is just not implemented for many items. I was thinking to introduce an addon in the model that controls the visibility of *all* items in the model. That addon would be responsible for setting the visible attribute of items, e.g. based on the visibility expression. The renders will react to the change of the visibility such that the items show up or get hidden. That addon would register runAndTrack methods similar to the one in processAddition, to evaluate the visibleWhen expressions. Futhermore, the addon could be made responsible for hiding empty toolbars. @Christoph, are you planning to work on visibility of 4.17? If so, I will open a umbrella bug to try to get an overview of all known issues.
@Rolf at least I plan to fix some bugs that hitting me whenever possible ;-) Your approach sounds interesting and much more consistent than scattering around the logic over all places.
