Skip to content

Commit

Permalink
Fixes microsoft/monaco-editor#2774: Emit content change events before…
Browse files Browse the repository at this point in the history 
… cursor state change events during undo, redo, or recovery from markers
  • Loading branch information
@alexdima
alexdima committed Mar 23, 2022
1 parent 12f99e0 commit 63f82f6
Show file tree
Hide file tree
Showing 7 changed files with 67 additions and 37 deletions.
7 changes: 4 additions & 3 deletions src/vs/editor/common/cursor/cursor.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { Range, IRange } from 'vs/editor/common/core/range';
import { ISelection, Selection, SelectionDirection } from 'vs/editor/common/core/selection';
import * as editorCommon from 'vs/editor/common/editorCommon';
import { ITextModel, TrackedRangeStickiness, IModelDeltaDecoration, ICursorStateComputer, IIdentifiedSingleEditOperation, IValidEditOperation } from 'vs/editor/common/model';
import { RawContentChangedType, ModelRawContentChangedEvent, ModelInjectedTextChangedEvent } from 'vs/editor/common/textModelEvents';
import { RawContentChangedType, ModelInjectedTextChangedEvent, InternalModelContentChangeEvent } from 'vs/editor/common/textModelEvents';
import { VerticalRevealType, ViewCursorStateChangedEvent, ViewRevealRangeRequestEvent } from 'vs/editor/common/viewEvents';
import { dispose, Disposable } from 'vs/base/common/lifecycle';
import { ICoordinatesConverter } from 'vs/editor/common/viewModel';
Expand Down Expand Up @@ -216,8 +216,8 @@ export class CursorsController extends Disposable {
this.revealPrimary(eventsCollector, 'restoreState', false, VerticalRevealType.Simple, true, editorCommon.ScrollType.Immediate);
}

public onModelContentChanged(eventsCollector: ViewModelEventsCollector, e: ModelRawContentChangedEvent | ModelInjectedTextChangedEvent): void {
if (e instanceof ModelInjectedTextChangedEvent) {
public onModelContentChanged(eventsCollector: ViewModelEventsCollector, event: InternalModelContentChangeEvent | ModelInjectedTextChangedEvent): void {
if (event instanceof ModelInjectedTextChangedEvent) {
// If injected texts change, the view positions of all cursors need to be updated.
if (this._isHandling) {
// The view positions will be updated when handling finishes
Expand All @@ -234,6 +234,7 @@ export class CursorsController extends Disposable {
this._isHandling = false;
}
} else {
const e = event.rawContentChangedEvent;
this._knownModelVersionId = e.versionId;
if (this._isHandling) {
return;
Expand Down
4 changes: 2 additions & 2 deletions src/vs/editor/common/model.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { LineTokens } from 'vs/editor/common/tokens/lineTokens';
import { IPosition, Position } from 'vs/editor/common/core/position';
import { IRange, Range } from 'vs/editor/common/core/range';
import { Selection } from 'vs/editor/common/core/selection';
import { IModelContentChange, IModelContentChangedEvent, IModelDecorationsChangedEvent, IModelLanguageChangedEvent, IModelLanguageConfigurationChangedEvent, IModelOptionsChangedEvent, IModelTokensChangedEvent, ModelInjectedTextChangedEvent, ModelRawContentChangedEvent } from 'vs/editor/common/textModelEvents';
import { IModelContentChange, IModelContentChangedEvent, IModelDecorationsChangedEvent, IModelLanguageChangedEvent, IModelLanguageConfigurationChangedEvent, IModelOptionsChangedEvent, IModelTokensChangedEvent, InternalModelContentChangeEvent, ModelInjectedTextChangedEvent } from 'vs/editor/common/textModelEvents';
import { WordCharacterClassifier } from 'vs/editor/common/core/wordCharacterClassifier';
import { FormattingOptions, StandardTokenType } from 'vs/editor/common/languages';
import { ThemeColor } from 'vs/platform/theme/common/themeService';
Expand Down Expand Up @@ -1153,7 +1153,7 @@ export interface ITextModel {
* @internal
* @event
*/
readonly onDidChangeContentOrInjectedText: Event<ModelRawContentChangedEvent | ModelInjectedTextChangedEvent>;
readonly onDidChangeContentOrInjectedText: Event<InternalModelContentChangeEvent | ModelInjectedTextChangedEvent>;
/**
* An event emitted when the contents of the model have changed.
* @event
Expand Down
4 changes: 2 additions & 2 deletions src/vs/editor/common/model/textModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,9 @@ export class TextModel extends Disposable implements model.ITextModel, IDecorati
public onDidChangeContent(listener: (e: IModelContentChangedEvent) => void): IDisposable {
return this._eventEmitter.slowEvent((e: InternalModelContentChangeEvent) => listener(e.contentChangedEvent));
}
public onDidChangeContentOrInjectedText(listener: (e: ModelRawContentChangedEvent | ModelInjectedTextChangedEvent) => void): IDisposable {
public onDidChangeContentOrInjectedText(listener: (e: InternalModelContentChangeEvent | ModelInjectedTextChangedEvent) => void): IDisposable {
return combinedDisposable(
this._eventEmitter.fastEvent(e => listener(e.rawContentChangedEvent)),
this._eventEmitter.fastEvent(e => listener(e)),
this._onDidChangeInjectedText.event(e => listener(e))
);
}
Expand Down
11 changes: 5 additions & 6 deletions src/vs/editor/common/viewModel/viewModelImpl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,8 @@ export class ViewModel extends Disposable implements IViewModel {
let hadOtherModelChange = false;
let hadModelLineChangeThatChangedLineMapping = false;

const changes = e.changes;
const versionId = (e instanceof textModelEvents.ModelRawContentChangedEvent ? e.versionId : null);
const changes = (e instanceof textModelEvents.InternalModelContentChangeEvent ? e.rawContentChangedEvent.changes : e.changes);
const versionId = (e instanceof textModelEvents.InternalModelContentChangeEvent ? e.rawContentChangedEvent.versionId : null);

// Do a first pass to compute line mappings, and a second pass to actually interpret them
const lineBreaksComputer = this._lines.createLineBreaksComputer();
Expand Down Expand Up @@ -391,6 +391,9 @@ export class ViewModel extends Disposable implements IViewModel {

try {
const eventsCollector = this._eventDispatcher.beginEmitViewEvents();
if (e instanceof textModelEvents.InternalModelContentChangeEvent) {
eventsCollector.emitOutgoingEvent(new ModelContentChangedEvent(e.contentChangedEvent));
}
this._cursor.onModelContentChanged(eventsCollector, e);
} finally {
this._eventDispatcher.endEmitViewEvents();
Expand All @@ -399,10 +402,6 @@ export class ViewModel extends Disposable implements IViewModel {
this._tokenizeViewportSoon.schedule();
}));

this._register(this.model.onDidChangeContent((e) => {
this._eventDispatcher.emitOutgoingEvent(new ModelContentChangedEvent(e));
}));

this._register(this.model.onDidChangeTokens((e) => {
const viewRanges: { fromLineNumber: number; toLineNumber: number }[] = [];
for (let j = 0, lenJ = e.ranges.length; j < lenJ; j++) {
Expand Down
29 changes: 29 additions & 0 deletions src/vs/editor/test/browser/widget/codeEditorWidget.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,4 +122,33 @@ suite('CodeEditorWidget', () => {
});
});

test('monaco-editor issue #2774 - Wrong order of events onDidChangeModelContent and onDidChangeCursorSelection on redo', () => {
withTestCodeEditor('', {}, (editor, viewModel) => {
const disposables = new DisposableStore();

const calls: string[] = [];
disposables.add(editor.onDidChangeModelContent((e) => {
calls.push(`contentchange(${e.changes.reduce<any[]>((aggr, c) => [...aggr, c.text, c.rangeOffset, c.rangeLength], []).join(', ')})`);
}));
disposables.add(editor.onDidChangeCursorSelection((e) => {
calls.push(`cursorchange(${e.selection.positionLineNumber}, ${e.selection.positionColumn})`);
}));

viewModel.type('a', 'test');
viewModel.model.undo();
viewModel.model.redo();

assert.deepStrictEqual(calls, [
'contentchange(a, 0, 0)',
'cursorchange(1, 2)',
'contentchange(, 0, 1)',
'cursorchange(1, 1)',
'contentchange(a, 0, 0)',
'cursorchange(1, 2)'
]);

disposables.dispose();
});
});

});
44 changes: 22 additions & 22 deletions src/vs/editor/test/common/model/model.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { EditOperation } from 'vs/editor/common/core/editOperation';
import { Position } from 'vs/editor/common/core/position';
import { Range } from 'vs/editor/common/core/range';
import { TextModel } from 'vs/editor/common/model/textModel';
import { ModelInjectedTextChangedEvent, ModelRawContentChangedEvent, ModelRawFlush, ModelRawLineChanged, ModelRawLinesDeleted, ModelRawLinesInserted } from 'vs/editor/common/textModelEvents';
import { InternalModelContentChangeEvent, ModelRawContentChangedEvent, ModelRawFlush, ModelRawLineChanged, ModelRawLinesDeleted, ModelRawLinesInserted } from 'vs/editor/common/textModelEvents';
import { EncodedTokenizationResult, IState, MetadataConsts, TokenizationRegistry } from 'vs/editor/common/languages';
import { LanguageConfigurationRegistry } from 'vs/editor/common/languages/languageConfigurationRegistry';
import { NullState } from 'vs/editor/common/languages/nullTokenize';
Expand Down Expand Up @@ -103,12 +103,12 @@ suite('Editor Model - Model', () => {
});

test('model insert text without newline eventing', () => {
let e: ModelRawContentChangedEvent | ModelInjectedTextChangedEvent | null = null;
let e: ModelRawContentChangedEvent | null = null;
thisModel.onDidChangeContentOrInjectedText((_e) => {
if (e !== null) {
if (e !== null || !(_e instanceof InternalModelContentChangeEvent)) {
assert.fail('Unexpected assertion error');
}
e = _e;
e = _e.rawContentChangedEvent;
});
thisModel.applyEdits([EditOperation.insert(new Position(1, 1), 'foo ')]);
assert.deepStrictEqual(e, new ModelRawContentChangedEvent(
Expand All @@ -122,12 +122,12 @@ suite('Editor Model - Model', () => {
});

test('model insert text with one newline eventing', () => {
let e: ModelRawContentChangedEvent | ModelInjectedTextChangedEvent | null = null;
let e: ModelRawContentChangedEvent | null = null;
thisModel.onDidChangeContentOrInjectedText((_e) => {
if (e !== null) {
if (e !== null || !(_e instanceof InternalModelContentChangeEvent)) {
assert.fail('Unexpected assertion error');
}
e = _e;
e = _e.rawContentChangedEvent;
});
thisModel.applyEdits([EditOperation.insert(new Position(1, 3), ' new line\nNo longer')]);
assert.deepStrictEqual(e, new ModelRawContentChangedEvent(
Expand Down Expand Up @@ -199,12 +199,12 @@ suite('Editor Model - Model', () => {
});

test('model delete text from one line eventing', () => {
let e: ModelRawContentChangedEvent | ModelInjectedTextChangedEvent | null = null;
let e: ModelRawContentChangedEvent | null = null;
thisModel.onDidChangeContentOrInjectedText((_e) => {
if (e !== null) {
if (e !== null || !(_e instanceof InternalModelContentChangeEvent)) {
assert.fail('Unexpected assertion error');
}
e = _e;
e = _e.rawContentChangedEvent;
});
thisModel.applyEdits([EditOperation.delete(new Range(1, 1, 1, 2))]);
assert.deepStrictEqual(e, new ModelRawContentChangedEvent(
Expand All @@ -218,12 +218,12 @@ suite('Editor Model - Model', () => {
});

test('model delete all text from a line eventing', () => {
let e: ModelRawContentChangedEvent | ModelInjectedTextChangedEvent | null = null;
let e: ModelRawContentChangedEvent | null = null;
thisModel.onDidChangeContentOrInjectedText((_e) => {
if (e !== null) {
if (e !== null || !(_e instanceof InternalModelContentChangeEvent)) {
assert.fail('Unexpected assertion error');
}
e = _e;
e = _e.rawContentChangedEvent;
});
thisModel.applyEdits([EditOperation.delete(new Range(1, 1, 1, 14))]);
assert.deepStrictEqual(e, new ModelRawContentChangedEvent(
Expand All @@ -237,12 +237,12 @@ suite('Editor Model - Model', () => {
});

test('model delete text from two lines eventing', () => {
let e: ModelRawContentChangedEvent | ModelInjectedTextChangedEvent | null = null;
let e: ModelRawContentChangedEvent | null = null;
thisModel.onDidChangeContentOrInjectedText((_e) => {
if (e !== null) {
if (e !== null || !(_e instanceof InternalModelContentChangeEvent)) {
assert.fail('Unexpected assertion error');
}
e = _e;
e = _e.rawContentChangedEvent;
});
thisModel.applyEdits([EditOperation.delete(new Range(1, 4, 2, 6))]);
assert.deepStrictEqual(e, new ModelRawContentChangedEvent(
Expand All @@ -257,12 +257,12 @@ suite('Editor Model - Model', () => {
});

test('model delete text from many lines eventing', () => {
let e: ModelRawContentChangedEvent | ModelInjectedTextChangedEvent | null = null;
let e: ModelRawContentChangedEvent | null = null;
thisModel.onDidChangeContentOrInjectedText((_e) => {
if (e !== null) {
if (e !== null || !(_e instanceof InternalModelContentChangeEvent)) {
assert.fail('Unexpected assertion error');
}
e = _e;
e = _e.rawContentChangedEvent;
});
thisModel.applyEdits([EditOperation.delete(new Range(1, 4, 3, 5))]);
assert.deepStrictEqual(e, new ModelRawContentChangedEvent(
Expand Down Expand Up @@ -308,12 +308,12 @@ suite('Editor Model - Model', () => {

// --------- setValue
test('setValue eventing', () => {
let e: ModelRawContentChangedEvent | ModelInjectedTextChangedEvent | null = null;
let e: ModelRawContentChangedEvent | null = null;
thisModel.onDidChangeContentOrInjectedText((_e) => {
if (e !== null) {
if (e !== null || !(_e instanceof InternalModelContentChangeEvent)) {
assert.fail('Unexpected assertion error');
}
e = _e;
e = _e.rawContentChangedEvent;
});
thisModel.setValue('new value');
assert.deepStrictEqual(e, new ModelRawContentChangedEvent(
Expand Down
5 changes: 3 additions & 2 deletions src/vs/editor/test/common/model/modelInjectedText.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import * as assert from 'assert';
import { EditOperation } from 'vs/editor/common/core/editOperation';
import { Range } from 'vs/editor/common/core/range';
import { TextModel } from 'vs/editor/common/model/textModel';
import { LineInjectedText, ModelRawChange, RawContentChangedType } from 'vs/editor/common/textModelEvents';
import { InternalModelContentChangeEvent, LineInjectedText, ModelRawChange, RawContentChangedType } from 'vs/editor/common/textModelEvents';
import { createTextModel } from 'vs/editor/test/common/testTextModel';

suite('Editor Model - Injected Text Events', () => {
Expand All @@ -25,7 +25,8 @@ suite('Editor Model - Injected Text Events', () => {
const recordedChanges = new Array<unknown>();

thisModel.onDidChangeContentOrInjectedText((e) => {
for (const change of e.changes) {
const changes = (e instanceof InternalModelContentChangeEvent ? e.rawContentChangedEvent.changes : e.changes);
for (const change of changes) {
recordedChanges.push(mapChange(change));
}
});
Expand Down

0 comments on commit 63f82f6

Please sign in to comment.