Improved text selection by allowing multiple edit ranges to be highlighted #4725
Conversation
GauravVBhambhani
requested review from
tomasz-stefaniak
and removed request for
a team
✅ Deploy Preview for continuedev canceled.
|
There was a problem hiding this comment.
Hey thanks for working on this. I love that you're just going through our todo list and clearing it out ! :-) You're very quick.
I haven't tried this yet, but I made a few suggestions. As always Nate and Dallin get final call, but here are my thoughts just doing a quick read through.
The commit message also needs work. The top line of a git commit message is supposed to be a summary line that doesn't get much longer than 50 characters.
I'd suggest something like:
Decorate all active edit ranges
If a user selects multiple chunks of code with cmd-I, only the most recent chunk of code is decorated as a selection.
This commit addresses that problem by changing setDecorations to addDecorations, and carefully tracking which ranges are currently active in the EditDecorationManager.
| private activeRanges: vscode.Range[] = []; // Store active ranges to be highlighted | ||
| private activeRangeSet: Set<string> = new Set(); // Store active ranges as strings to avoid duplicates |
There was a problem hiding this comment.
I think it would be a little cleaner to combine these into a single map instead
private activeRangesMap: Map<string, vscode.Range> = new Map();
| setDecoration(editor: vscode.TextEditor, range: vscode.Range) { | ||
| if (this._lastEditor) { | ||
| this._lastEditor.setDecorations(this.decorationType, []); | ||
| // converting each range to a unique string for storing in set |
There was a problem hiding this comment.
// Converts each range to a unique string for storing in the map
| this._lastEditor.setDecorations(this.decorationType, []); | ||
| // converting each range to a unique string for storing in set | ||
| private rangeToString(range: vscode.Range): string { | ||
| return `${range.start.line}:${range.start.character}-${range.end.line}:${range.end.character}`; |
There was a problem hiding this comment.
I feel like some slightly different punctuation could make this clearer in console.log output or whatever...e.g, you have
1:1-4:80
but
(1,1)-(4,80)
would look a little clearer. doesn't really matter, but just food for though for later debugging sanity :-)
| return `${range.start.line}:${range.start.character}-${range.end.line}:${range.end.character}`; | ||
| } | ||
|
|
||
| setDecoration(editor: vscode.TextEditor, ranges: vscode.Range[]): void { |
There was a problem hiding this comment.
"set" usually means "replace" but this function augments not replaces now.should probably be renamed addDecorations
| // Filter out already highlighted ranges | ||
| const newRanges = ranges.filter(range => { | ||
| const rangeKey = this.rangeToString(range); | ||
| if (this.activeRangeSet.has(rangeKey)) { | ||
| return false; // already highlighted | ||
| } | ||
| this.activeRangeSet.add(rangeKey); | ||
| return true; | ||
| }); | ||
|
|
||
| if (newRanges.length === 0) return; // No new ranges to highlight | ||
|
|
||
| // Update active ranges and apply decorations | ||
| this.activeRanges.push(...newRanges); | ||
| editor.setDecorations(this.decorationType, this.activeRanges); |
There was a problem hiding this comment.
This doesn't really handle overlapping ranges right I think.
I think you need some code like this (assuming we switch to the map as suggested above):
private rangesCoincide(a: vscode.Range, b: vscode.Range): boolean {
if (a.end.isEqual(b.start)) return true;
if (b.end.isEqual(a.start)) return true;
if (a.intersection(b)) return true;
return false;
}
private mergeNewRange(newRange: vscode.Range): void {
let mergedRange = newRange;
const rangesToPrune: string[] = [];
for (const [key, existingRange] of this.activeRangesMap.entries()) {
if (!this.rangesCoincide(merged, existingRange)) continue;
mergedRange = mergedRange.union(existingRange);
rangesToPrune.push(key);
}
for (const key of rangesToPrune) this.activeRangesMap.delete(key);
this.activeRangesMap.set(this.rangeToString(mergedRange), mergedRange);
}
and then here like
for (const range of ranges) this.mergeNewRange(range);
const activeRanges = Array.from(this.activeRangesMap.values());
if (activeRanges.length === 0) return; // No new ranges to highlight
editor.setDecorations(this.decorationType, activeRanges);
or something like that.
There was a problem hiding this comment.
You pretty much fixed the whole thing😂, but thank you! Got to learn a few things. Appreciate it! 😊
GauravVBhambhani
force-pushed
the
multiple-range-highlighting
branch
from
5a91e51 to
fb03412
Compare
| this._lastEditor.setDecorations(this.decorationType, []); | ||
| // Converts each range to a unique string for storing in the map | ||
| private rangeToString(range: vscode.Range): string { | ||
| return `(${range.start.line},${range.start.character}-${range.end.line},${range.end.character})`; |
There was a problem hiding this comment.
i would still bracket it differently... like:
return `(${range.start.line},${range.start.character})-(${range.end.line},${range.end.character})`;
so dev reading logs knows immediately it's (a,b)-(c,d), not (a, b-c, d)
| } | ||
|
|
||
| // Adds a new decoration to the editor and merges it with existing ranges | ||
| addDecoration(editor: vscode.TextEditor, ranges: vscode.Range[]): void { |
There was a problem hiding this comment.
i would make this plural since it's takes an array of ranges and could end up with more than one
GauravVBhambhani
force-pushed
the
multiple-range-highlighting
branch
from
744df41 to
f80f755
Compare
If a user selects multiple chunks of code with cmd-I, only the most recent chunk of code is decorated as a selection. This commit addresses that problem by changing setDecorations to addDecorations, and carefully tracking which ranges are currently active in the EditDecorationManager. Additionally, overlapping or adjacent ranges are now merged seamlessly to prevent redundant highlights.
GauravVBhambhani
force-pushed
the
multiple-range-highlighting
branch
from
0cb7648 to
8699786
Compare
Description
Text Selection: Decorate all active edit ranges
If a user selects multiple chunks of code with cmd-I, only the most recent chunk of code is decorated as a selection.
This commit addresses that problem by changing setDecorations to addDecorations, and carefully tracking which ranges are currently active in the EditDecorationManager.
Additionally, overlapping or adjacent ranges are now merged seamlessly to prevent redundant highlights.
Screenshots
Granite.Code Issue #49
@halfline