Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent display of menu tooltips #2483

Closed
ewillink opened this issue Nov 3, 2024 · 7 comments
Closed

Inconsistent display of menu tooltips #2483

ewillink opened this issue Nov 3, 2024 · 7 comments

Comments

@ewillink
Copy link

ewillink commented Nov 3, 2024

I found that some of my tooltips were not being shown and after fixing some bad XML (the editor does not diagnose a gratuitous extra premature ">") I found that just changing the org.eclipse.ui.menus extension point in plugin.xml from
<menuContribution locationURI="popup:org.eclipse.ocl.examples.ui.Menu?after=show">
to
<menuContribution locationURI="popup:org.eclipse.ocl.examples.ui.Menu?after=load">
could make the missing tooltip visible.

Debugging by setting a breakpoint at line 1353 of Menu.class
boolean success = OS.GetMenuItemRect (0, selectedMenuItem.parent.handle, selectedMenuItem.index, rect);
if (!success) return null;
revealed two problems. a) when success is true, b) when success is false.

The first (success true) is reprodible on a basic Eclipse platform. Many menu entries have no tooltip. e.g. Help->Tip of the Day however debugging reveals this is because itemToolTip is null. Surely a missing tooltip should be 'inferred' as the label as is often the case already.

The second (success false) appears to be related to plugin.xml extension of a dynamically defined menu - not reprodicible on a basic platform.

In so far as I can debug this, I suspect that the selectedMenuItem.index argument to OS.GetMenuItemRect fails to account for separators and/or invisible entries and/or sub-menus.

'Repro'. The problems I experience are reproducible using the context menu of the Sample Ecore Editor. Team->Show Local History has a tooltip that is not displayed because success=false. (A repro using my released OCL code will just uncover downstream effects of the bad XML.)

@ewillink
Copy link
Author

ewillink commented Nov 4, 2024

Indeed. The miniscule, should-be-irrelevant, difference is the separator in:

<extension point="org.eclipse.ui.menus">
		<menuContribution locationURI="popup:org.eclipse.ui.popup.any?after=additions">
         <menu id="org.eclipse.ocl.examples.ui.Menu" label="%MF_OCL" icon="icons/ocl16.gif">
            <separator name="load" visible="true"/>
            <separator name="save" visible="true"/>
            <separator name="set" visible="true"/>
            <separator name="show" visible="true"/>
			<visibleWhen checkEnabled="false">
				<test property="org.eclipse.ocl.examples.ui.resourceSetAvailable" forcePluginActivation="true"/>
			</visibleWhen>
         </menu>
      </menuContribution>
   </extension>

Once M3 is out I should be able to post a repro that avoids project GIT repos or bad XML.

@merks
Copy link
Contributor

merks commented Nov 5, 2024

In a debug launch, it worked correctly for the text editor. But if EGit is included in the launch it doesn't work. I can see it gets to this state:

So it knows the selected item and it is the one with a tool tip text, but the index of that item is 18 even though it is the first item in the menu (which unfortuately is black during the screen capture:

image

So it seems to me the problem is caused by the wrong index, likely because of items that have been removed or filtered or hidden separators. When EGit is not included in the launch, the index is zero which makes sense because it's the first visible item in the menu. I don't have time right now to debug further...

@merks
Copy link
Contributor

merks commented Nov 5, 2024

I'm not suggesting this is the fix, but if I change the code in org.eclipse.swt.widgets.Menu.wmTimer(long, long) like this:

	LRESULT wmTimer(long wParam, long lParam) {
		if (wParam == ID_TOOLTIP_TIMER) {
			POINT pt = new POINT();
			OS.GetCursorPos(pt);
			if (selectedMenuItem != null && selectedMenuItem.parent != null) {
				RECT rect = new RECT();
				MenuItem[] items = getItems();
				for (MenuItem menuItem : items) {
					System.err.println(menuItem.index + ":" + menuItem.getText() + "->" + menuItem.getToolTipText());
				}
				int index = Arrays.asList(items).indexOf(selectedMenuItem);
				// boolean success = OS.GetMenuItemRect (0, selectedMenuItem.parent.handle,
				// selectedMenuItem.index, rect);
				boolean success = OS.GetMenuItemRect(0, selectedMenuItem.parent.handle, index, rect);

Then the tool tip shows, I believe because the index is correct relative to the actual menu items current in in the menu.

With the print statement, without EGit deployed, I see this:

0:Show Local &History->Show Local &History
1:->null
2:Appl&y Patch...->Apply a patch to one or more workspace projects.
3:->null
4:&Share Project...->Share the project with others using a version and configuration management system.

With EGit's feature in the launch I see this instead:

18:Show Local &History->Show Local &History
24:->null
26:Appl&y Patch...->Apply a patch to one or more workspace projects.
35:->null
36:&Share Project...->Share the project with others using a version and configuration management system.

So it's quite clear that the menuItem.index value is simply wrong and this is the reason why the tool tip associated with that menu item is not shown.

I'm out of time again for now...

@ewillink
Copy link
Author

ewillink commented Nov 5, 2024

Thanks. I don't recognize the black area, but

clear that the menuItem.index value is simply wrong

seems very similar to my suspicion. I won't struggle to provide a rival M3 repro using my OCL code.

@merks
Copy link
Contributor

merks commented Nov 5, 2024

I won't struggle to provide a rival M3 repro using my OCL code.

Indeed. I can reproduce the problem and will continue to dig.

@merks
Copy link
Contributor

merks commented Nov 6, 2024

I think the design is just wrong/broken. The MenuItem.index is initialized upon construction and is never updated nor maintained. But a MenuItem.dispose can obviously change every other MenuItem's actual index in the disposed MenuItem's containing Menu.

merks added a commit to merks/eclipse.platform.swt that referenced this issue Nov 6, 2024
- Remove MenuItem.index which is now unused.
- Remove unused MenuItem.MenuItem(Menu, Menu, int, int)

eclipse-platform/eclipse.platform.ui#2483
HeikoKlare pushed a commit to merks/eclipse.platform.swt that referenced this issue Nov 7, 2024
- Remove MenuItem.index which is now unused.
- Remove unused MenuItem.MenuItem(Menu, Menu, int, int)

eclipse-platform/eclipse.platform.ui#2483
HeikoKlare pushed a commit to merks/eclipse.platform.swt that referenced this issue Nov 7, 2024
- Remove MenuItem.index which is now unused.
- Remove unused MenuItem.MenuItem(Menu, Menu, int, int)

eclipse-platform/eclipse.platform.ui#2483
merks added a commit to eclipse-platform/eclipse.platform.swt that referenced this issue Nov 7, 2024
- Remove MenuItem.index which is now unused.
- Remove unused MenuItem.MenuItem(Menu, Menu, int, int)

eclipse-platform/eclipse.platform.ui#2483
@HeikoKlare
Copy link
Contributor

If I am not mistaken, this has been fixed via eclipse-platform/eclipse.platform.swt#1575
Or is there something else to do for this, @merks?

@merks merks closed this as completed Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants