Always manage custom tooltip in Tree in Win32 #62 #2137
Conversation
The commit allows to always create custom tooltip handles in the Tree while creating items. The is necessary to always fit the tooltips inside a single monitor as there can be freezing problems while using per-monitor awareness if the tooltip spans over multiple monitors. contributes to eclipse-platform#62 and eclipse-platform#127
There was a problem hiding this comment.
Looks like a nice simplification of the tooltip handling. But I have to admit that I do not understand why it works. Without these changes, isCustomToolTip() never yields "false" for me, even for the problematic breakpoints view. Please see the detailed comments for that.
| boolean isCustomToolTip () { | ||
| return hooks (SWT.MeasureItem); | ||
| } |
There was a problem hiding this comment.
Can you please explain why this is not relevant anymore (or why it was relevant before)? In my understanding, it validates whether a MeasureItem listener was registered, but I have to admit that I do not understand the reasons. What will be the effect in case there was a tree for which such a listener was not registered and thus tool tips were not custom? I set a breakpoint for the case the !isCustomToolTip() and triggered tool tips in several Trees, but it was never triggered.
There was a problem hiding this comment.
It was never triggered because of the folowing reasons:
- For breakpoints view, Eclipse doesn't maintain the tooltipHandle as createItem(TreeItem, long, long, long) doesn't have a call to createItemToolTipHandles(), but only createItem(TreeColumn, int) and _addListener calls for that. For example, in case of outline view, the items are created using createItem(TreeItem, long, long, long) and then _addListener is called to register a measure listener, which also triggers createItemToolTipHandles().
- There are 2 checks to verify if custom tooltips exist: if itemToolTipHandle field is available and isCustomToolTip is true.
- In case of breakpoint, since neither tooltip handle is created not a measure event is registered, the tooltips are directly handled by windows, i.e. the painting event of tooltip is indeed recieved by Eclipse but no operation is performed and Windows is responsible to paint and position it.
Hence, Breakpoints view doesn't have any custom tooltips and cannot be manipulated to fit in the monitor. The only way we can make sure all the tooltips are fir in the monitor is by managing all the tooltip handle hence keeping them all custom and then repositioning and resizing them, which is done in this PR.
There was a problem hiding this comment.
My question remains: why was the usage of custom tooltip guarded with the MeasureItem hook being present before? I am a bit concerned that in case the hook is missing but we still handle tool items in a custom way, something does not work as expected.
There was a problem hiding this comment.
The only place where MeasureItem is relevant in tooltip is to find the bounds of the cell, in the method sendMeaureItemEvent. The event there however already has the bounds of the current item passed to the method. It is used in findCell, which already has the right reference to the item. So, in case there's no measureItem handler registered, it uses the default size of the cell (Which can be as long as the string, just like in the outline view).
coming back to isCustomToolTip, from my understanding, the usage of isCustomToolTip revolves around how it should be painted. If it is true, OS.CDDS_POSTPAINT event is sent here:
LRESULT wmNotifyToolTip (NMTTCUSTOMDRAW nmcd, long lParam) {
switch (nmcd.dwDrawStage) {
case OS.CDDS_PREPAINT: {
if (isCustomToolTip ()) {
//TEMPORARY CODE
//nmcd.uDrawFlags |= OS.DT_CALCRECT;
//OS.MoveMemory (lParam, nmcd, NMTTCUSTOMDRAW.sizeof);
return new LRESULT (OS.CDRF_NOTIFYPOSTPAINT | OS.CDRF_NEWFONT);
}
break;
}
...
which leads to painting the tooltip manually leading to the same thing painted manually on the tooltip as windows would. But with this we get the freedom to reposition it (which is the main goal to prevent an infinite cycle being triggered)
I did a before and after comparision of the tooltips in the tree in different places where they don't have any tooltTipHandle maintained manually originally. I'll attach the screenshots:
Before:
After:
Before:
After:
I do see a difference in the color of the text though.
| @@ -2200,7 +2200,7 @@ void createItem (TreeItem item, long hParent, long hInsertAfter, long hItem) { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| createItemToolTips (); | |||
There was a problem hiding this comment.
Why do we need this additional creation here?
This has some effect that I do no understand: without this addition, I do not face any calls to isCustomToolTip() that yields "false", even for the breakpoints view the original issue is about.
There was a problem hiding this comment.
The isCustomToolTip call in wmNotifyToolTip is guarded by itemToolTipHandle handle check. If that doesn't exist (which is the case when BReakpoint lets windows handle the tooltips completely and doesn't keep track of the handle) the isCustomToolTip will never be triggered.
There was a problem hiding this comment.
So maybe you can add an explanation of the overall solution: Do we basically need to call createItemTooltips() for every item being created now or what's the way it is supposed to work now?
There was a problem hiding this comment.
although we can bypass the itemToolTipHandle check when TTM_SHOW message is recieved. It is only recieved for the tooltip of the tree and we shouldn't care if we maintain the handle or not. If we recieve the message, we should try to find the right position of the tooltip and resposition it. If we can do that then we do not need to change the isCustomToolTip logic making the PR minimalistic.
| String[] strings = item [0].strings; | ||
| if (strings != null) text = strings [index [0]]; | ||
| } | ||
| //TEMPORARY CODE |
There was a problem hiding this comment.
Nice to get temporary removed from > 15 years ago removed :-)
| @@ -211,7 +211,7 @@ void _addListener (int eventType, Listener listener) { | |||
| case SWT.PaintItem: { | |||
| customDraw = true; | |||
| style |= SWT.DOUBLE_BUFFERED; | |||
| if (isCustomToolTip ()) createItemToolTips (); | |||
| createItemToolTips (); | |||
There was a problem hiding this comment.
There are three (?) places where createItemToolTips() is called. Can't we do it once e.g. in createHandle or doesn't that work?
There was a problem hiding this comment.
I tried but it seems to misbehave if I create tooltips for example in the constructor before there are any item in the tree.
| return new LRESULT (OS.CDRF_NOTIFYPOSTPAINT | OS.CDRF_NEWFONT); | ||
| } | ||
| break; | ||
| //TEMPORARY CODE |
There was a problem hiding this comment.
Would be interesting to understand what this does. If everything works fine, we could just remove that, right?
There was a problem hiding this comment.
would have a look.
|
See #2142 for a better approach. |
|
closing as taken over by #2142 |
The PR contributes to the creation of custom tooltip handles in the Tree while creating items. It is a necessity to always have custom tooltips as it needs to be fit inside a single monitor since there can be freezing problems while using per-monitor awareness if the tooltip spans over multiple monitors.
contributes to
#62 and #127