Fit Table Tooltip in a single monitor #62 #2241

amartya4256 · 2025-06-16T14:04:46Z

This PR contributes to the fitting of the table tooltip inside a
single monitor if it spans over multiple monitor to avoid any
infinite loop because of rescaling triggered by any DPI_CHANGED events.

Adapted from #2142

contributes to
#62 and
#128

How to test

First we need to create a table with narrow columns.

Go to this method: org.eclipse.swt.examples.controlexample.TableTab.packColumns()
Change it with this:

void packColumns () {
		int columnCount = table1.getColumnCount();
		for (int i = 0; i < columnCount; i++) {
			TableColumn tableColumn = table1.getColumn(i);
			tableColumn.setWidth(8);
//			tableColumn.pack();
		}
	}

Launch ControlExample and go to table tab and position the shell in such a way that the table columns overlap on multiple monitors.
Hover over the overlappting columns and the tooltip must be positioned in the monitor where the cursor is contained, as shown in the picture.

github-actions · 2025-06-16T14:17:34Z

Test Results

539 files - 6 539 suites - 6 37m 5s ⏱️ + 6m 6s
4 368 tests - 37 4 352 ✅ - 35 15 💤 - 3 0 ❌ ±0 1 🔥 +1
16 704 runs - 37 16 564 ✅ - 35 139 💤 - 3 0 ❌ ±0 1 🔥 +1

For more details on these errors, see this check.

Results for commit fd4305e. ± Comparison against base commit e06a5ec.

This pull request removes 37 tests.

AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageDataForDifferentFractionalZoomsShouldBeDifferent
AllWin32Tests org.eclipse.swt.graphics.ImageWin32Tests ‑ testImageShouldHaveDimesionAsPerZoomLevel
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testByteArrayTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testFileTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testHtmlTransfer
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromCopiedImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageData
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testImageTransfer_fromImageDataFromImage
AllWin32Tests org.eclipse.swt.tests.win32.Test_org_eclipse_swt_dnd_DND ‑ testRtfTransfer
…

♻️ This comment has been updated with latest results.

HeikoKlare

The change seems to work fine. But I have to admit that I don't know what the actual changes are supposed to do. I see the expected coordinates adjustment for the tooltip, but there are some further changes around, which I do not understand at all (I have added comments to those lines). Can you please elaborate on why these changes are necessary and what they do?

HeikoKlare · 2025-06-17T07:59:35Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Table.java

-			if (hdr.code != OS.TTN_SHOW) tipRequested = true;
-			long code = callWindowProc (handle, OS.WM_NOTIFY, wParam, lParam);
-			if (hdr.code != OS.TTN_SHOW) tipRequested = false;


Why can this code be removed? It will also result in the code returned by this method to usually be null (in case we are not in the customToolTip case with TTN_SHOW.

It was made corresponding to this bug: https://bugs.eclipse.org/bugs/show_bug.cgi?id=178044
Commit id: ec9593c
This was made for rollovers in 2007 but seems to have been fixed for the newer versions of windows. With the block present, the code goes in an infinite loop at 7162 and gets infinite TTN_GETDISPINFO messages, when the tooltip is by default over 2 monitors.
Thus, this could be removed now.

The current windows works fine with rollover tooltips withut this block as well, however this block makes it worse when the tooltip is in the middle of 2 screens. You can try running the code with this block present and try to position your tooltip in the middle of 2 monitors.

I have to admit that I do not fully understand the assessment. The code was not introduced for the mentioned bug and in the mentioned commit. The commit only moved that code to a different place, such that it is also executed for the TTN_SHOW event. The commit that originally introduced the code is from 2005: dd05f31. So the question remains why the code is now completely unnecessary (not only for TTN_SHOW but only for the TTN_GETDISPINFO event).

Just wondering: is the reaction to the TTN_GETDISPINFO still necessary at all? I tested with the ControlExample and there it does not make any difference whether I keep the switch-case for TTN_GETDISPINFO or whether I remove it (even though those events are actually fired). But since I don't know what this is necessary for, I cannot assess if there are remaining cases (maybe with very specific configurations) where it is needed.

In general, I currently still lack understanding of what exactly happens here. I can confirm that the change works as expected in all cases I have tested, but likely I don't know about all the use cases this complex code has been written for. So I am bit concerned that we break something when just removing some code. On the other hand, the code obviously leads to an issue (namely an inifite execution loop), which we definitely need to resolve. So it might lead to the trade-off that we need to accept that we might miss some use case here that we do not understand but therefor resolve a known bug.

Thus, my proposal:

Please check again what this code is used for (as, like I said above, the existing assessment is not comprehensible for me)

Have the change reviewed and tested by @fedejeanne, such that he can hopefully merge it

There's no explanation in the commit dd05f31, however it relates to the bug report mentioned I suppose. (https://bugs.eclipse.org/bugs/show_bug.cgi?id=82739)

There's a snippet in the report and I tried that out and found no issues. I believe the block of code:

tipRequested = true;Add commentMore actions int code = callWindowProc (handle, OS.WM_NOTIFY, wParam, lParam); tipRequested = false;

Doesn't have any visual effect anymore.

Anyway it's hard to assess what it was introduced for but I gathered that the tooltip works similar to how it is implemented for Tree. Without having much changes in the original implementation, I tried to bring it as close as possible to Tree. However, I assessed the bug report https://bugs.eclipse.org/bugs/show_bug.cgi?id=82739 and the other bug report which was its duplicate (https://bugs.eclipse.org/bugs/show_bug.cgi?id=83703) and it doesn't seem to hamper the table tooltips in externalize stings view, the custom tooltip on the table in the control which comes using (ctrl+3) and also the snippets. These fixes were done for XP and Vista as it seems but I have tested them with the current windows 11 and my PR doesn't change any behaviour at all for them.

If I test this PR with the snippet provided in https://bugs.eclipse.org/bugs/show_bug.cgi?id=82739#c1, I can see that this PR breaks the so called "Rollovers" (a special kind of tooltip, see: https://bugs.eclipse.org/bugs/show_bug.cgi?id=178044#c12).

So this PR breaks a use case. Please fix.

Here are the screenshots.

On master, I see the rollover when I hover over the second column and it looks fine.

With this PR, I don't see the text in the rollover:

I'm on Windows 10

Oh that was because of my recent commit. GETDISPINFO block in positionTooltip is only supposed to be executed in case of isCustomToolTip() == true since it tends to clean the default text on the tooltip for custom draw.
I have pushed the changes and tested the snippet again. It should be fine now.

Thank you, that fixed the issue.

I went through the mentioned commits and issues and I also couldn't find any other use case which required the "fix" you are talking about so I assume this code is not necessary in newer Windows versions.

IMO this code is not necessary anymore so I'll go ahead and merge this PR. We can keep an eye open just in case the missing use case is revealed and this change actually breaks something.

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Table.java

fedejeanne

This PR breaks a use case: see my comment in #2241 (comment)

fedejeanne · 2025-06-30T08:41:02Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Table.java

-			if (hdr.code != OS.TTN_SHOW) tipRequested = true;
-			long code = callWindowProc (handle, OS.WM_NOTIFY, wParam, lParam);
-			if (hdr.code != OS.TTN_SHOW) tipRequested = false;


If I test this PR with the snippet provided in https://bugs.eclipse.org/bugs/show_bug.cgi?id=82739#c1, I can see that this PR breaks the so called "Rollovers" (a special kind of tooltip, see: https://bugs.eclipse.org/bugs/show_bug.cgi?id=178044#c12).

So this PR breaks a use case. Please fix.

This commit contributes to the fitting of the table tooltip inside a single monitor if it spans over multiple monitor to avoid any infinite loop because of rescaling triggered by any DPI_CHANGED events. contributes to eclipse-platform#62 and eclipse-platform#128

fedejeanne

It looks good now. Thank you @amartya4256 !

fedejeanne · 2025-06-30T11:55:16Z

bundles/org.eclipse.swt/Eclipse SWT/win32/org/eclipse/swt/widgets/Table.java

-			if (hdr.code != OS.TTN_SHOW) tipRequested = true;
-			long code = callWindowProc (handle, OS.WM_NOTIFY, wParam, lParam);
-			if (hdr.code != OS.TTN_SHOW) tipRequested = false;


Thank you, that fixed the issue.

I went through the mentioned commits and issues and I also couldn't find any other use case which required the "fix" you are talking about so I assume this code is not necessary in newer Windows versions.

IMO this code is not necessary anymore so I'll go ahead and merge this PR. We can keep an eye open just in case the missing use case is revealed and this change actually breaks something.

fedejeanne · 2025-06-30T11:58:32Z

Failing test is unrelated: #2098

amartya4256 force-pushed the amartya4256/table_tooltip branch 3 times, most recently from 5a97f16 to 8106e43 Compare June 17, 2025 07:42

HeikoKlare reviewed Jun 17, 2025

View reviewed changes

amartya4256 linked an issue Jun 18, 2025 that may be closed by this pull request

Tooltip of Table freezing on spanning across multiple monitors vi-eclipse/Eclipse-Platform#302

Closed

amartya4256 force-pushed the amartya4256/table_tooltip branch 3 times, most recently from 7a66ca6 to 18da1d6 Compare June 20, 2025 12:22

fedejeanne force-pushed the amartya4256/table_tooltip branch from 18da1d6 to 70ef650 Compare June 26, 2025 14:39

fedejeanne reviewed Jun 30, 2025

View reviewed changes

amartya4256 force-pushed the amartya4256/table_tooltip branch from 70ef650 to a9ec515 Compare June 30, 2025 10:37

fedejeanne force-pushed the amartya4256/table_tooltip branch from a9ec515 to fd4305e Compare June 30, 2025 11:21

fedejeanne approved these changes Jun 30, 2025

View reviewed changes

fedejeanne merged commit d94cc15 into eclipse-platform:master Jun 30, 2025
15 of 17 checks passed

fedejeanne deleted the amartya4256/table_tooltip branch June 30, 2025 11:58

Fit Table Tooltip in a single monitor #62 #2241

Fit Table Tooltip in a single monitor #62 #2241

Uh oh!

Conversation

amartya4256 commented Jun 16, 2025

How to test

Uh oh!

github-actions bot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

HeikoKlare left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fedejeanne left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedejeanne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedejeanne commented Jun 30, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jun 16, 2025 •

edited

Loading

fedejeanne left a comment •

edited

Loading