eclipse.platform.ui icon indicating copy to clipboard operation
eclipse.platform.ui copied to clipboard

Double Separators can be created with empty DynamicMenuContributions

Open N1k145 opened this issue 1 year ago • 10 comments

Let's make sure issue is not already fixed in latest builds first.

Steps to reproduce

AFAIK, JFace has a logic to remove double Separators from Context Menus. But this only works if they are next to each other after the contributions are collected. When some of these contributions do not contribute anything, the double Separators are not removed.

I tried: grafik The Second Contribution in this screenshot creates a dynamic menu contribution and a separator before that. The dynamic contribution does not add any elements (This could be the case that in the current selection, none of the possible results are valid, depending on implementation)

I expected: That the double and end separators are removed

But got: There is a Separator at the end of the Context Menu that is not removed. grafik

Tested under this environment:

  • OS & version: Windows 11 and Linux
  • Eclipse IDE/Platform version (as shown in Help > About): latest 2024-03

Community

  • [x] I understand reporting an issue to this OSS project does not mandate anyone to fix it. Other contributors may consider the issue, or not, at their own convenience. The most efficient way to get it fixed is that I fix it myself and contribute it back as a good quality patch to the project.

N1k145 avatar Feb 12 '24 12:02 N1k145

The change https://github.com/eclipse-platform/eclipse.platform.ui/pull/1669 causes a severe regression. Almost every context menu invocation in almost every view I've tried (Package Explorer, Git Repositories, Error Log, History) results in one or multiple errors logged to the error log.

eclipse.buildId=4.32.0.I20240410-1800
java.version=21.0.2-13
java.vendor=N/A
BootLoader constants: OS=linux, ARCH=x86_64, WS=gtk, NL=en_US
Command-line arguments:  -os linux -ws gtk -arch x86_64 -data /data/4x_platform_workspace

org.eclipse.ui
Error
Thu Apr 11 08:28:28 CEST 2024
Unhandled event loop exception

org.eclipse.e4.core.di.InjectionException: org.eclipse.swt.SWTException: Widget is disposed
	at org.eclipse.e4.core.internal.di.MethodRequestor.execute(MethodRequestor.java:68)
	at org.eclipse.swt.widgets.Synchronizer.syncExec(Synchronizer.java:183)
	at org.eclipse.ui.internal.UISynchronizer.syncExec(UISynchronizer.java:133)
	at org.eclipse.swt.widgets.Display.syncExec(Display.java:5960)
	at org.eclipse.e4.ui.workbench.swt.DisplayUISynchronize.syncExec(DisplayUISynchronize.java:34)
	at org.eclipse.e4.ui.internal.di.UIEventObjectSupplier$UIEventHandler.handleEvent(UIEventObjectSupplier.java:64)
	at org.eclipse.equinox.internal.event.EventHandlerWrapper.handleEvent(EventHandlerWrapper.java:206)
	at org.eclipse.equinox.internal.event.EventHandlerTracker.dispatchEvent(EventHandlerTracker.java:201)
	at org.eclipse.equinox.internal.event.EventHandlerTracker.dispatchEvent(EventHandlerTracker.java:1)
	at org.eclipse.osgi.framework.eventmgr.EventManager.dispatchEvent(EventManager.java:230)
	at org.eclipse.osgi.framework.eventmgr.ListenerQueue.dispatchEventSynchronous(ListenerQueue.java:151)
	at org.eclipse.equinox.internal.event.EventAdminImpl.dispatchEvent(EventAdminImpl.java:131)
	at org.eclipse.equinox.internal.event.EventAdminImpl.sendEvent(EventAdminImpl.java:73)
	at org.eclipse.equinox.internal.event.EventComponent.sendEvent(EventComponent.java:44)
	at org.eclipse.e4.ui.services.internal.events.EventBroker.send(EventBroker.java:55)
	at org.eclipse.e4.ui.internal.workbench.UIEventPublisher.notifyChanged(UIEventPublisher.java:60)
	at org.eclipse.emf.common.notify.impl.BasicNotifierImpl.eNotify(BasicNotifierImpl.java:424)
	at org.eclipse.e4.ui.model.application.ui.impl.UIElementImpl.setVisible(UIElementImpl.java:365)
	at org.eclipse.e4.ui.workbench.renderers.swt.ContributionRecord.updateVisibility(ContributionRecord.java:102)
	at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRendererFilter.updateElementVisibility(MenuManagerRendererFilter.java:173)
	at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRendererFilter.updateElementVisibility(MenuManagerRendererFilter.java:184)
	at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerShowProcessor.showMenu(MenuManagerShowProcessor.java:248)
	at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerShowProcessor.menuAboutToHide(MenuManagerShowProcessor.java:114)
	at org.eclipse.jface.internal.MenuManagerEventHelper.showEventPostHelper(MenuManagerEventHelper.java:89)
	at org.eclipse.jface.action.MenuManager.handleAboutToShow(MenuManager.java:468)
	at org.eclipse.jface.action.MenuManager$2.menuShown(MenuManager.java:494)
	at org.eclipse.swt.widgets.TypedListener.handleEvent(TypedListener.java:272)
	at org.eclipse.swt.widgets.EventTable.sendEvent(EventTable.java:91)
	at org.eclipse.swt.widgets.Display.sendEvent(Display.java:5855)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1617)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1643)
	at org.eclipse.swt.widgets.Widget.sendEvent(Widget.java:1622)
	at org.eclipse.swt.widgets.Menu._setVisible(Menu.java:290)
	at org.eclipse.swt.widgets.Display.runPopups(Display.java:5107)
	at org.eclipse.swt.widgets.Display.readAndDispatch(Display.java:4508)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine$5.run(PartRenderingEngine.java:1151)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.e4.ui.internal.workbench.swt.PartRenderingEngine.run(PartRenderingEngine.java:1042)
	at org.eclipse.e4.ui.internal.workbench.E4Workbench.createAndRunUI(E4Workbench.java:152)
	at org.eclipse.ui.internal.Workbench.lambda$3(Workbench.java:639)
	at org.eclipse.core.databinding.observable.Realm.runWithDefault(Realm.java:339)
	at org.eclipse.ui.internal.Workbench.createAndRunWorkbench(Workbench.java:546)
	at org.eclipse.ui.PlatformUI.createAndRunWorkbench(PlatformUI.java:173)
	at org.eclipse.ui.internal.ide.application.IDEApplication.start(IDEApplication.java:152)
	at org.eclipse.equinox.internal.app.EclipseAppHandle.run(EclipseAppHandle.java:208)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.runApplication(EclipseAppLauncher.java:143)
	at org.eclipse.core.runtime.internal.adaptor.EclipseAppLauncher.start(EclipseAppLauncher.java:109)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:439)
	at org.eclipse.core.runtime.adaptor.EclipseStarter.run(EclipseStarter.java:271)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.eclipse.equinox.launcher.Main.invokeFramework(Main.java:679)
	at org.eclipse.equinox.launcher.Main.basicRun(Main.java:616)
	at org.eclipse.equinox.launcher.Main.run(Main.java:1492)
	at org.eclipse.equinox.launcher.Main.main(Main.java:1465)
Caused by: org.eclipse.swt.SWTException: Widget is disposed
	at org.eclipse.swt.SWT.error(SWT.java:4922)
	at org.eclipse.swt.SWT.error(SWT.java:4837)
	at org.eclipse.swt.SWT.error(SWT.java:4808)
	at org.eclipse.swt.widgets.Widget.error(Widget.java:597)
	at org.eclipse.swt.widgets.Widget.checkWidget(Widget.java:512)
	at org.eclipse.swt.widgets.Widget.getData(Widget.java:623)
	at org.eclipse.jface.action.MenuManager.update(MenuManager.java:842)
	at org.eclipse.jface.action.MenuManager.update(MenuManager.java:672)
	at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer.scheduleManagerUpdate(MenuManagerRenderer.java:1161)
	at org.eclipse.e4.ui.workbench.renderers.swt.MenuManagerRenderer.subscribeUIElementTopicVisible(MenuManagerRenderer.java:215)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.eclipse.e4.core.internal.di.MethodRequestor.execute(MethodRequestor.java:58)
	... 54 more

Please fix ASAP or revert the change.

iloveeclipse avatar Apr 11 '24 06:04 iloveeclipse

@iloveeclipse I'm sorry I look into this right now. Maybe there changed something else in between my implementation and the merging

N1k145 avatar Apr 11 '24 06:04 N1k145

@iloveeclipse I'm sorry I look into this right now.

Thanks.

Maybe there changed something else in between my implementation and the merging

Not really, there were no changes in this area recently AFAIK. It can be difference between Linux/Windows SWT implementations, assuming you were testing on Windows.

Looking on the code I assume before every (overoptimistic) call to item.getData() has to be guarded by check if the item is already disposed, as the code adds extra disposals on items it iterates.

iloveeclipse avatar Apr 11 '24 06:04 iloveeclipse

Not really, there were no changes in this area recently AFAIK. It can be difference between Linux/Windows SWT implementations, assuming you were testing on Windows.

I overlooked that in your stack trace. I developed this on Windows, I check again on GTK, I can't reproduce it right now on Windows.

N1k145 avatar Apr 11 '24 06:04 N1k145

@iloveeclipse Can you take a look at #1812 I'm still not able to reproduce the issue on GTK, added now isDisposed checks before accessing the data. Can you check if this works for you, without breaking something else?

N1k145 avatar Apr 11 '24 07:04 N1k145

I see no issue with plain SDK.

But if I install all the bundles from https://github.com/eclipse-platform/eclipse.platform.ui/blob/master/releng/org.eclipse.ui.releng/platformUiTools.p2f (via File -> Import -> Install -> Install Software Items from File) I see errors. So the "double" separators seem to be added by some bundles installed and that runs into the problem with new code.

iloveeclipse avatar Apr 11 '24 07:04 iloveeclipse

With the nightly build, I am able to reproduce this. But still not in the Debugger. I'm currently trying to build it locally.

N1k145 avatar Apr 11 '24 09:04 N1k145

@iloveeclipse I was now able to verify my fix in #1812 for me, it works now without any exceptions

N1k145 avatar Apr 11 '24 10:04 N1k145

As noticed while reviewing possible fix: not only errors are reported, but I see now duplicated separators in the SDK which were not present in the last build.

So I'm in favour of reverting the change completely.

iloveeclipse avatar Apr 11 '24 12:04 iloveeclipse

The change was reverted via https://github.com/eclipse-platform/eclipse.platform.ui/pull/1815

iloveeclipse avatar Apr 11 '24 14:04 iloveeclipse