Change terminal link hover widget position to be consistent#83175
Merged
Tyriar merged 8 commits intomicrosoft:masterfrom Oct 25, 2019
Merged
Change terminal link hover widget position to be consistent#83175Tyriar merged 8 commits intomicrosoft:masterfrom
Tyriar merged 8 commits intomicrosoft:masterfrom
Conversation
jmbockhorst
changed the title
Tyriar
requested changes
Oct 23, 2019
| } | ||
|
|
||
| public showMessage(left: number, top: number, text: string): void { | ||
| public showMessage(left: number, bottom: number, text: string, positionIsTopOfWidget: boolean = false): void { |
Member
There was a problem hiding this comment.
This could be a little nicer is bottom was renamed to y and we used something like this:
enum VerticalAlignment {
Bottom,
Top
}| let offsetRow = this._xterm.rows - location.start.row + 1; | ||
| let useTopPosition = false; | ||
|
|
||
| // Handle a link on the top row |
Member
There was a problem hiding this comment.
Suggested change
| // Handle a link on the top row | |
| // Show the tooltip on the top of the next row to avoid obscuring the first row |
Comment on lines
107
to
115
| if (this._configHelper.config.rendererType === 'dom') { | ||
| const target = (e.target as HTMLElement); | ||
| this._widgetManager.showMessage(target.offsetLeft, target.offsetTop, this._getLinkHoverString()); | ||
| const font = this._configHelper.getFont(); | ||
| const charWidth = font.charWidth; | ||
| const charHeight = font.charHeight; | ||
|
|
||
| const leftPosition = (location.start.col - 1) * (charWidth! + (font.letterSpacing / window.devicePixelRatio)); | ||
| const bottomPosition = offsetRow * (charHeight! * font.lineHeight); | ||
|
|
||
| this._widgetManager.showMessage(leftPosition, bottomPosition, this._getLinkHoverString(), useTopPosition); |
Member
There was a problem hiding this comment.
Got it in this state with the dom renderer when zoomed in (ctrl/cmd++)
Contributor
Author
|
Thanks for the feedback. I made the requested changes and fixed the issue when zoomed. |
Member
|
The bottom row doesn't get positions correctly:
|
Tyriar
requested changes
Oct 24, 2019
| this._domNode.style.bottom = `${Math.max(_y, WIDGET_HEIGHT) - WIDGET_HEIGHT}px`; | ||
| } else { | ||
| // Y position is to the bottom of the widget | ||
| this._domNode.style.bottom = `${Math.min(Math.max(_y, WIDGET_HEIGHT), _container.offsetHeight - WIDGET_HEIGHT)}px`; |
Member
There was a problem hiding this comment.
Looks like this is the problem, we need the row height to be passed through and used for the very bottom row.
Contributor
Author
There was a problem hiding this comment.
We don't actually want to check the max of _y and WIDGET_HEIGHT when y is to the bottom of the widget, since even y = 0 will still be in the viewport.
This was referenced Oct 25, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The PR changes the terminal link tooltip position to be consistent with the rest of the editor. This is part of #78647, and is the first step for #77964.
Currently, the tooltip position is where the mouse position is, and it sometimes creates unexpected behaviors.
With the pull request, the tooltip position is based on where the link position is.
To do this, I had to change the terminalWidgetManager to render Widgets using bottom position, since the terminal is snapped to the bottom of the container and has no knowledge about the extra space at the top between the actual terminal and the container. Since link tooltips are the only thing I found that uses the terminalWidgetManager, I figured it would be okay. I also added a new parameter that controls whether or not the bottom position is to the top or bottom of the widget.
I tested it for both renderers and with increased letter spacing and line heights.