-
Notifications
You must be signed in to change notification settings - Fork 118
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
Tree.java: correctly escape & in default Tooltip #801
base: master
Are you sure you want to change the base?
Conversation
This bug might also exist in GTK- or Cocoa-based implementations. I suspect that they have similar fixes. |
@@ -5639,7 +5639,8 @@ String toolTipText (NMTTDISPINFO hdr) { | |||
if (findCell (pt.x, pt.y, item, index, cellRect, itemRect)) { | |||
String text = null; | |||
if (index [0] == 0) { | |||
text = item [0].text; | |||
text = new String(item [0].text); | |||
text = text.replaceAll("&", "&&"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this needs to be replaceAll, where the first argument is interpreted as a regular expression rather than just a simple replace which also replaces all substrings using "literal" matching?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback, I agree with you and changed my code accordingly
Is that the right place to be changing the contents of the tooltip string? Something tells me that it's not, and perhaps ampersand escaping should be done further up the stack, or by the client. |
b78e8db
to
925be9e
Compare
@Phillipus Escaping the |
Yeah, I can see the problem. It's just that when I look at the Is this also happening on Mac and Linux? Edit - it's OK on Mac. |
@@ -5639,7 +5639,8 @@ String toolTipText (NMTTDISPINFO hdr) { | |||
if (findCell (pt.x, pt.y, item, index, cellRect, itemRect)) { | |||
String text = null; | |||
if (index [0] == 0) { | |||
text = item [0].text; | |||
text = new String(item [0].text); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is creating a new String necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have addressed this issue. @Phillipus
This fixes eclipse-platform/eclipse.platform.ui#1075 When TreeColumn's are too small, a ToolTip is displayed on hover to reveal the whole Label of an Item (The WIN32-Docs call them 'Inline ToolTips'). These ToolTips pass through the Composite-ToolTipText-generator which escapes all & in a Text - allowing to reuse Text from Buttons as Tooltips. This is a well documented Feature of setToolTipText. Since TreeItem's Label does not have Mnemonics, we do not assume that the & needs escaping. This Commit Escapes all & in default Tooltips that are handled by the default mechanism.
925be9e
to
18a56fc
Compare
Fix for
eclipse-platform/eclipse.platform.ui#238
eclipse-platform/eclipse.platform.ui#1075
When TreeColumn's are too small, a ToolTip is displayed on hover to reveal the whole Label of an Item (The WIN32-Docs call them In-Place Tooltips).
These ToolTips pass through the Composite-ToolTipText-generator which escapes all & in a Text - allowing to reuse Text from Buttons as Tooltips. This is a well documented Feature of setToolTipText. Since TreeItem's Label does not have Mnemonics, we do not assume that the & should require escaping by the User.
This Commit Escapes all & in default Tooltips that are handled by the default mechanism.