Fit Table Tooltip in a single monitor #62

[amartya4256]

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]

Test Results

   545 files  + 6     545 suites  +6   34m 46s ⏱️ -27s
 4 403 tests +37   4 385 ✅ +35   18 💤 +3  0 ❌ ±0 
16 733 runs  +37  16 593 ✅ +35  140 💤 +3  0 ❌ ±0 

Results for commit 70ef650. ± Comparison against base commit edc5529.

♻️ 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]

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.

[amartya4256]

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.

[amartya4256]

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.

[HeikoKlare]

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

[amartya4256]

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.