Add selection highlight decorations to minimap #80596
Conversation
There was a problem hiding this comment.
This is implemented in a very smart way, without touching any editor rendering code, but maintaining N decorations in sync with the N editor selections is IMHO an overkill solution for this problem. There could be cases where there are thousands of selections and adding/removing/updating all of these decorations is IMHO not efficient. IMHO it is possible to implement this directly in the minimap and limit the performance cost of rendering to only the visible area of the minimap (by intersecting it with the selections...)
I suggest instead that the minimap directly paints the selection. The minimap can immediately read the selection every time it changes by adding a new listener here after the configuration change listener:
public onCursorStateChanged(e: viewEvents.ViewCursorStateChangedEvent): boolean {
this._selections = e.selections;
return true;
}The minimap can capture the view selections in a field and then use the stored selections in the renderDecorations method. In that method, you can loop also over selections and invoke renderDecorationOnLine... I think this could be doable only with minor changes to renderDecorationOnLine to allow a range and a color to be passed in instead of a ViewModelDecoration instance.
|
Nice, your proposal makes the code change very small! :) |
| @@ -453,6 +455,8 @@ export class Minimap extends ViewPart { | |||
| private _options: MinimapOptions; | |||
| private _lastRenderData: RenderData | null; | |||
| private _lastDecorations: ViewModelDecoration[] | undefined; | |||
| private _selections: Selection[] = []; | |||
| private _selectionColor: Color | undefined; | |||
There was a problem hiding this comment.
small nitpick: please use null instead of undefined (to keep in spirit the existing code style)
There was a problem hiding this comment.
Ah, in this case the undefined is from the return type of getColor, so will leave as is
| // Only bother calling render if decorations are currently shown | ||
| this._renderDecorations = !!this._lastDecorations; | ||
| return !!this._lastDecorations; | ||
| this._renderDecorations = !!this._lastDecorations || !!this._selections.length; |
There was a problem hiding this comment.
this._selections.length is always >= 1 ... (the editor will always have at least 1 selection), so we might as well say this._renderDecorations = true;
|
Thanks for the review! Addressed feedback, merging this in now. |
For #21404
This adds minimap decorations matching the current editor selections, and introduces a
minimap.selectionHighlighttheme color.