Skip to content

Commit 4560e52

Browse files
alexdimaalexdima
committed
Fixes microsoft#84897: Ranges and positions inside graphemes are considered valid (needed for editing)
1 parent d54ac9f commit 4560e52

3 files changed

Lines changed: 110 additions & 143 deletions

File tree

‎src/vs/editor/common/model/textModel.ts‎

Lines changed: 76 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,17 @@ class TextModelSnapshot implements model.ITextSnapshot {
158158

159159
const invalidFunc = () => { throw new Error(`Invalid change accessor`); };
160160

161+
const enum StringOffsetValidationType {
162+
/**
163+
* Even allowed in surrogate pairs
164+
*/
165+
Relaxed = 0,
166+
/**
167+
* Not allowed in surrogate pairs
168+
*/
169+
SurrogatePairs = 1,
170+
}
171+
161172
export class TextModel extends Disposable implements model.ITextModel {
162173

163174
private static readonly MODEL_SYNC_LIMIT = 50 * 1024 * 1024; // 50 MB
@@ -673,7 +684,7 @@ export class TextModel extends Disposable implements model.ITextModel {
673684

674685
public getOffsetAt(rawPosition: IPosition): number {
675686
this._assertNotDisposed();
676-
let position = this._validatePosition(rawPosition.lineNumber, rawPosition.column, false);
687+
let position = this._validatePosition(rawPosition.lineNumber, rawPosition.column, StringOffsetValidationType.Relaxed);
677688
return this._buffer.getOffsetAt(position.lineNumber, position.column);
678689
}
679690

@@ -868,10 +879,7 @@ export class TextModel extends Disposable implements model.ITextModel {
868879
return new Range(startLineNumber, startColumn, endLineNumber, endColumn);
869880
}
870881

871-
/**
872-
* @param strict Do NOT allow a position inside a high-low surrogate pair
873-
*/
874-
private _isValidPosition(lineNumber: number, column: number, strict: boolean): boolean {
882+
private _isValidPosition(lineNumber: number, column: number, validationType: StringOffsetValidationType): boolean {
875883
if (typeof lineNumber !== 'number' || typeof column !== 'number') {
876884
return false;
877885
}
@@ -893,25 +901,27 @@ export class TextModel extends Disposable implements model.ITextModel {
893901
return false;
894902
}
895903

904+
if (column === 1) {
905+
return true;
906+
}
907+
896908
const maxColumn = this.getLineMaxColumn(lineNumber);
897909
if (column > maxColumn) {
898910
return false;
899911
}
900912

901-
if (strict) {
902-
const [charStartOffset,] = strings.getCharContainingOffset(this._buffer.getLineContent(lineNumber), column - 1);
903-
if (column !== charStartOffset + 1) {
913+
if (validationType === StringOffsetValidationType.SurrogatePairs) {
914+
// !!At this point, column > 1
915+
const charCodeBefore = this._buffer.getLineCharCode(lineNumber, column - 2);
916+
if (strings.isHighSurrogate(charCodeBefore)) {
904917
return false;
905918
}
906919
}
907920

908921
return true;
909922
}
910923

911-
/**
912-
* @param strict Do NOT allow a position inside a high-low surrogate pair
913-
*/
914-
private _validatePosition(_lineNumber: number, _column: number, strict: boolean): Position {
924+
private _validatePosition(_lineNumber: number, _column: number, validationType: StringOffsetValidationType): Position {
915925
const lineNumber = Math.floor((typeof _lineNumber === 'number' && !isNaN(_lineNumber)) ? _lineNumber : 1);
916926
const column = Math.floor((typeof _column === 'number' && !isNaN(_column)) ? _column : 1);
917927
const lineCount = this._buffer.getLineCount();
@@ -933,105 +943,109 @@ export class TextModel extends Disposable implements model.ITextModel {
933943
return new Position(lineNumber, maxColumn);
934944
}
935945

936-
if (strict) {
937-
const [charStartOffset,] = strings.getCharContainingOffset(this._buffer.getLineContent(lineNumber), column - 1);
938-
if (column !== charStartOffset + 1) {
939-
return new Position(lineNumber, charStartOffset + 1);
946+
if (validationType === StringOffsetValidationType.SurrogatePairs) {
947+
// If the position would end up in the middle of a high-low surrogate pair,
948+
// we move it to before the pair
949+
// !!At this point, column > 1
950+
const charCodeBefore = this._buffer.getLineCharCode(lineNumber, column - 2);
951+
if (strings.isHighSurrogate(charCodeBefore)) {
952+
return new Position(lineNumber, column - 1);
940953
}
941954
}
942955

943956
return new Position(lineNumber, column);
944957
}
945958

946959
public validatePosition(position: IPosition): Position {
960+
const validationType = StringOffsetValidationType.SurrogatePairs;
947961
this._assertNotDisposed();
948962

949963
// Avoid object allocation and cover most likely case
950964
if (position instanceof Position) {
951-
if (this._isValidPosition(position.lineNumber, position.column, true)) {
965+
if (this._isValidPosition(position.lineNumber, position.column, validationType)) {
952966
return position;
953967
}
954968
}
955969

956-
return this._validatePosition(position.lineNumber, position.column, true);
970+
return this._validatePosition(position.lineNumber, position.column, validationType);
957971
}
958972

959-
/**
960-
* @param strict Do NOT allow a range to have its boundaries inside a high-low surrogate pair
961-
*/
962-
private _isValidRange(range: Range, strict: boolean): boolean {
973+
private _isValidRange(range: Range, validationType: StringOffsetValidationType): boolean {
963974
const startLineNumber = range.startLineNumber;
964975
const startColumn = range.startColumn;
965976
const endLineNumber = range.endLineNumber;
966977
const endColumn = range.endColumn;
967978

968-
if (!this._isValidPosition(startLineNumber, startColumn, false)) {
979+
if (!this._isValidPosition(startLineNumber, startColumn, StringOffsetValidationType.Relaxed)) {
969980
return false;
970981
}
971-
if (!this._isValidPosition(endLineNumber, endColumn, false)) {
982+
if (!this._isValidPosition(endLineNumber, endColumn, StringOffsetValidationType.Relaxed)) {
972983
return false;
973984
}
974985

975-
if (strict) {
976-
const startLineContent = this._buffer.getLineContent(startLineNumber);
977-
if (startColumn < startLineContent.length + 1) {
978-
const [charStartOffset,] = strings.getCharContainingOffset(startLineContent, startColumn - 1);
979-
if (startColumn !== charStartOffset + 1) {
980-
return false;
981-
}
982-
}
986+
if (validationType === StringOffsetValidationType.SurrogatePairs) {
987+
const charCodeBeforeStart = (startColumn > 1 ? this._buffer.getLineCharCode(startLineNumber, startColumn - 2) : 0);
988+
const charCodeBeforeEnd = (endColumn > 1 && endColumn <= this._buffer.getLineLength(endLineNumber) ? this._buffer.getLineCharCode(endLineNumber, endColumn - 2) : 0);
983989

984-
if (endColumn >= 2) {
985-
const endLineContent = (endLineNumber === startLineNumber ? startLineContent : this._buffer.getLineContent(endLineNumber));
986-
const [, charEndOffset] = strings.getCharContainingOffset(endLineContent, endColumn - 2);
987-
if (endColumn !== charEndOffset + 1) {
988-
return false;
989-
}
990-
}
990+
const startInsideSurrogatePair = strings.isHighSurrogate(charCodeBeforeStart);
991+
const endInsideSurrogatePair = strings.isHighSurrogate(charCodeBeforeEnd);
991992

992-
return true;
993+
if (!startInsideSurrogatePair && !endInsideSurrogatePair) {
994+
return true;
995+
}
996+
return false;
993997
}
994998

995999
return true;
9961000
}
9971001

9981002
public validateRange(_range: IRange): Range {
1003+
const validationType = StringOffsetValidationType.SurrogatePairs;
9991004
this._assertNotDisposed();
10001005

10011006
// Avoid object allocation and cover most likely case
10021007
if ((_range instanceof Range) && !(_range instanceof Selection)) {
1003-
if (this._isValidRange(_range, true)) {
1008+
if (this._isValidRange(_range, validationType)) {
10041009
return _range;
10051010
}
10061011
}
10071012

1008-
const start = this._validatePosition(_range.startLineNumber, _range.startColumn, false);
1009-
const end = this._validatePosition(_range.endLineNumber, _range.endColumn, false);
1013+
const start = this._validatePosition(_range.startLineNumber, _range.startColumn, StringOffsetValidationType.Relaxed);
1014+
const end = this._validatePosition(_range.endLineNumber, _range.endColumn, StringOffsetValidationType.Relaxed);
10101015

10111016
const startLineNumber = start.lineNumber;
1012-
let startColumn = start.column;
1017+
const startColumn = start.column;
10131018
const endLineNumber = end.lineNumber;
1014-
let endColumn = end.column;
1015-
const isEmpty = (startLineNumber === endLineNumber && startColumn === endColumn);
1016-
1017-
const startLineContent = this._buffer.getLineContent(startLineNumber);
1018-
if (startColumn < startLineContent.length + 1) {
1019-
const [charStartOffset,] = strings.getCharContainingOffset(startLineContent, startColumn - 1);
1020-
if (startColumn !== charStartOffset + 1) {
1021-
if (isEmpty) {
1022-
// do not expand a collapsed range, simply move it to a valid location
1023-
return new Range(startLineNumber, charStartOffset + 1, startLineNumber, charStartOffset + 1);
1024-
}
1025-
startColumn = charStartOffset + 1;
1019+
const endColumn = end.column;
1020+
1021+
if (validationType === StringOffsetValidationType.SurrogatePairs) {
1022+
const charCodeBeforeStart = (startColumn > 1 ? this._buffer.getLineCharCode(startLineNumber, startColumn - 2) : 0);
1023+
const charCodeBeforeEnd = (endColumn > 1 && endColumn <= this._buffer.getLineLength(endLineNumber) ? this._buffer.getLineCharCode(endLineNumber, endColumn - 2) : 0);
1024+
1025+
const startInsideSurrogatePair = strings.isHighSurrogate(charCodeBeforeStart);
1026+
const endInsideSurrogatePair = strings.isHighSurrogate(charCodeBeforeEnd);
1027+
1028+
if (!startInsideSurrogatePair && !endInsideSurrogatePair) {
1029+
return new Range(startLineNumber, startColumn, endLineNumber, endColumn);
1030+
}
1031+
1032+
if (startLineNumber === endLineNumber && startColumn === endColumn) {
1033+
// do not expand a collapsed range, simply move it to a valid location
1034+
return new Range(startLineNumber, startColumn - 1, endLineNumber, endColumn - 1);
1035+
}
1036+
1037+
if (startInsideSurrogatePair && endInsideSurrogatePair) {
1038+
// expand range at both ends
1039+
return new Range(startLineNumber, startColumn - 1, endLineNumber, endColumn + 1);
10261040
}
1027-
}
10281041

1029-
if (endColumn >= 2) {
1030-
const endLineContent = (endLineNumber === startLineNumber ? startLineContent : this._buffer.getLineContent(endLineNumber));
1031-
const [, charEndOffset] = strings.getCharContainingOffset(endLineContent, endColumn - 2);
1032-
if (endColumn !== charEndOffset + 1) {
1033-
endColumn = charEndOffset + 1;
1042+
if (startInsideSurrogatePair) {
1043+
// only expand range at the start
1044+
return new Range(startLineNumber, startColumn - 1, endLineNumber, endColumn);
10341045
}
1046+
1047+
// only expand range at the end
1048+
return new Range(startLineNumber, startColumn, endLineNumber, endColumn + 1);
10351049
}
10361050

10371051
return new Range(startLineNumber, startColumn, endLineNumber, endColumn);

‎src/vs/editor/test/browser/controller/cursor.test.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2303,6 +2303,40 @@ suite('Editor Controller - Regression tests', () => {
23032303

23042304
model.dispose();
23052305
});
2306+
2307+
test('issue #84897: Left delete behavior in some languages is changed', () => {
2308+
let model = createTextModel(
2309+
[
2310+
'สวัสดี'
2311+
].join('\n')
2312+
);
2313+
2314+
withTestCodeEditor(null, { model: model }, (editor, cursor) => {
2315+
editor.setSelections([
2316+
new Selection(1, 7, 1, 7)
2317+
]);
2318+
2319+
CoreEditingCommands.DeleteLeft.runEditorCommand(null, editor, null);
2320+
assert.equal(model.getValue(EndOfLinePreference.LF), 'สวัสด');
2321+
2322+
CoreEditingCommands.DeleteLeft.runEditorCommand(null, editor, null);
2323+
assert.equal(model.getValue(EndOfLinePreference.LF), 'สวัส');
2324+
2325+
CoreEditingCommands.DeleteLeft.runEditorCommand(null, editor, null);
2326+
assert.equal(model.getValue(EndOfLinePreference.LF), 'สวั');
2327+
2328+
CoreEditingCommands.DeleteLeft.runEditorCommand(null, editor, null);
2329+
assert.equal(model.getValue(EndOfLinePreference.LF), 'สว');
2330+
2331+
CoreEditingCommands.DeleteLeft.runEditorCommand(null, editor, null);
2332+
assert.equal(model.getValue(EndOfLinePreference.LF), 'ส');
2333+
2334+
CoreEditingCommands.DeleteLeft.runEditorCommand(null, editor, null);
2335+
assert.equal(model.getValue(EndOfLinePreference.LF), '');
2336+
});
2337+
2338+
model.dispose();
2339+
});
23062340
});
23072341

23082342
suite('Editor Controller - Cursor Configuration', () => {

‎src/vs/editor/test/common/model/textModel.test.ts‎

Lines changed: 0 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -743,87 +743,6 @@ suite('Editor Model - TextModel', () => {
743743
assert.deepEqual(m.validatePosition(new Position(2, 1.5)), new Position(2, 1), 'h');
744744
});
745745

746-
function assertValidatePosition(m: TextModel, lineNumber: number, column: number, expectedColumn: number): void {
747-
const input = new Position(lineNumber, column);
748-
const actual = m.validatePosition(input);
749-
const expected = new Position(lineNumber, expectedColumn);
750-
assert.deepEqual(actual, expected, `validatePosition for ${input}, got ${actual}, expected ${expected}`);
751-
}
752-
753-
function assertValidateRange(m: TextModel, input: Range, expected: Range): void {
754-
const actual = m.validateRange(input);
755-
assert.deepEqual(actual, expected, `validateRange for ${input}, got ${actual}, expected ${expected}`);
756-
}
757-
758-
test('grapheme breaking', () => {
759-
const m = TextModel.createFromString([
760-
'abcabc',
761-
'ãããããã',
762-
'辻󠄀辻󠄀辻󠄀',
763-
'புபுபு',
764-
].join('\n'));
765-
766-
assertValidatePosition(m, 2, 1, 1);
767-
assertValidatePosition(m, 2, 2, 1);
768-
assertValidatePosition(m, 2, 3, 3);
769-
assertValidatePosition(m, 2, 4, 3);
770-
assertValidatePosition(m, 2, 5, 5);
771-
assertValidatePosition(m, 2, 6, 5);
772-
assertValidatePosition(m, 2, 7, 7);
773-
assertValidatePosition(m, 2, 8, 7);
774-
assertValidatePosition(m, 2, 9, 9);
775-
assertValidatePosition(m, 2, 10, 9);
776-
assertValidatePosition(m, 2, 11, 11);
777-
assertValidatePosition(m, 2, 12, 11);
778-
assertValidatePosition(m, 2, 13, 13);
779-
assertValidatePosition(m, 2, 14, 13);
780-
781-
assertValidatePosition(m, 3, 1, 1);
782-
assertValidatePosition(m, 3, 2, 1);
783-
assertValidatePosition(m, 3, 3, 1);
784-
assertValidatePosition(m, 3, 4, 4);
785-
assertValidatePosition(m, 3, 5, 4);
786-
assertValidatePosition(m, 3, 6, 4);
787-
assertValidatePosition(m, 3, 7, 7);
788-
assertValidatePosition(m, 3, 8, 7);
789-
assertValidatePosition(m, 3, 9, 7);
790-
assertValidatePosition(m, 3, 10, 10);
791-
792-
assertValidatePosition(m, 4, 1, 1);
793-
assertValidatePosition(m, 4, 2, 1);
794-
assertValidatePosition(m, 4, 3, 3);
795-
assertValidatePosition(m, 4, 4, 3);
796-
assertValidatePosition(m, 4, 5, 5);
797-
assertValidatePosition(m, 4, 6, 5);
798-
assertValidatePosition(m, 4, 7, 7);
799-
800-
assertValidateRange(m, new Range(2, 1, 2, 1), new Range(2, 1, 2, 1));
801-
assertValidateRange(m, new Range(2, 1, 2, 2), new Range(2, 1, 2, 3));
802-
assertValidateRange(m, new Range(2, 1, 2, 3), new Range(2, 1, 2, 3));
803-
assertValidateRange(m, new Range(2, 1, 2, 4), new Range(2, 1, 2, 5));
804-
assertValidateRange(m, new Range(2, 1, 2, 5), new Range(2, 1, 2, 5));
805-
assertValidateRange(m, new Range(2, 2, 2, 2), new Range(2, 1, 2, 1));
806-
assertValidateRange(m, new Range(2, 2, 2, 3), new Range(2, 1, 2, 3));
807-
assertValidateRange(m, new Range(2, 2, 2, 4), new Range(2, 1, 2, 5));
808-
assertValidateRange(m, new Range(2, 2, 2, 5), new Range(2, 1, 2, 5));
809-
810-
assertValidateRange(m, new Range(3, 1, 3, 1), new Range(3, 1, 3, 1));
811-
assertValidateRange(m, new Range(3, 1, 3, 2), new Range(3, 1, 3, 4));
812-
assertValidateRange(m, new Range(3, 1, 3, 3), new Range(3, 1, 3, 4));
813-
assertValidateRange(m, new Range(3, 1, 3, 4), new Range(3, 1, 3, 4));
814-
assertValidateRange(m, new Range(3, 1, 3, 5), new Range(3, 1, 3, 7));
815-
assertValidateRange(m, new Range(3, 1, 3, 6), new Range(3, 1, 3, 7));
816-
assertValidateRange(m, new Range(3, 1, 3, 7), new Range(3, 1, 3, 7));
817-
assertValidateRange(m, new Range(3, 2, 3, 2), new Range(3, 1, 3, 1));
818-
assertValidateRange(m, new Range(3, 2, 3, 3), new Range(3, 1, 3, 4));
819-
assertValidateRange(m, new Range(3, 2, 3, 4), new Range(3, 1, 3, 4));
820-
assertValidateRange(m, new Range(3, 2, 3, 5), new Range(3, 1, 3, 7));
821-
assertValidateRange(m, new Range(3, 2, 3, 6), new Range(3, 1, 3, 7));
822-
assertValidateRange(m, new Range(3, 2, 3, 7), new Range(3, 1, 3, 7));
823-
824-
m.dispose();
825-
});
826-
827746
test('issue #71480: validateRange handle floats', () => {
828747
let m = TextModel.createFromString('line one\nline two');
829748

0 commit comments

Comments
 (0)