Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Request: onDidChangeTextDocument - differentiate between user input and redo/undo #120617

Closed
hediet opened this issue Apr 6, 2021 · 3 comments · Fixed by #124996
Closed
Assignees
Labels
api api-proposal editor-core Editor basic functionality feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@hediet
Copy link
Member

hediet commented Apr 6, 2021

The extension API of VS Code 1.54.3 offers the event workspace.onDidChangeTextDocument that can be used to subscribe to text changes.

Currently, it does not seem to be possible to differentiate between text changes caused by user-input and those caused by redo/undo.

This is problematic if an extension wants to use the onDidChangeTextDocument event to modify the document when certain characters are typed by the user.

For example, the abbreviation rewrite feature of the Lean VS Code extension makes use of it (if the user types \alpha it gets replaced by α immediately).

However, if the user undoes and redoes their changes, onDidChangeTextDocument is also triggered. When undoing/redoing, new text edits are very harmful though, as they destroy the redo stack. Thus, extensions must not modify the document on redos/undos.

If redos cannot be differentiated from user input, this problem cannot be avoided if an extension wants to modify the document on certain text changes.

Proposal

It would be helpful if VS Code could provide a reason that caused the text change.
Reasons could be:

  • userInput: The user typed a character.
  • redo: Caused by redo
  • undo: Caused by undo
  • unknown: E.g. caused by liveshare or use of workspace.applyEdit

The reason could be provided by the TextDocumentChangeEvent:

export enum TextDocumentChangeReason {
    /** The text change is caused by extensions or other unknown sources. */
    unknown,

    /** The text change is caused by the user editing the document. */
    userInput,

    /** The text change is caused as part of an redo operation. */
    redo,

    /** The text change is caused as part of an undo operation. */
    undo,
}

/**
 * An event describing a transactional [document](#TextDocument) change.
 */
export interface TextDocumentChangeEvent {

    /**
     * The affected document.
     */
    readonly document: TextDocument;

    /**
     * An array of content changes.
     */
    readonly contentChanges: ReadonlyArray<TextDocumentContentChangeEvent>;

    // >>>>>>>>> NEW
    readonly reason: TextDocumentChangeReason;
    // <<<<<<<<<
}

Thank you!

@jrieken
Copy link
Member

jrieken commented Apr 6, 2021

Should be easy'ish for undo/redo because the underlying change event has this information:

readonly isUndoing: boolean;

@jrieken jrieken added api editor-core Editor basic functionality feature-request Request for new features or functionality labels Apr 6, 2021
@jrieken jrieken added this to the Backlog milestone Apr 6, 2021
@hediet
Copy link
Member Author

hediet commented Apr 7, 2021

I think it actually might be better to provide flags instead of an enum. If ever more reasons are added, extensions cannot rely on covering all cases (and thus break).
Also, an isPasting flag could enable smart paste and solve parts of #30066.

/**
 * An event describing a transactional [document](#TextDocument) change.
 * At most one of the fields `isUndoing`, `isRedoing` or `isUserInput` is true.
 */
export interface TextDocumentChangeEvent {

    /**
     * The affected document.
     */
    readonly document: TextDocument;

    /**
     * An array of content changes.
     */
    readonly contentChanges: ReadonlyArray<TextDocumentContentChangeEvent>;

    /**
     * Indicates if this text change was caused by an undo operation.
     */
    readonly isUndoing: boolean;

    /**
     * Indicates if this text change was caused by an redo operation.
     */
    readonly isRedoing: boolean;

    /**
     * Indicates if this text change was caused by user input.
     */
    readonly isUserInput: boolean;
}

Alternatively, these flags could be put into an object and their exclusivity could be typed explicitly. It adds a lot of noise though and I'd prefer just plain flags.

...
    reason: TextDocumentChangeReason,
...

type TextDocumentChangeReason = {
    isUndoing: true,
    isRedoing: false,
    isUserInput: false,
    ...
} | {
    isUndoing: false,
    isRedoing: true,
    isUserInput: false,
    ...
} | {
    isUndoing: false,
    isRedoing: false,
    isUserInput: true,
    ...
};

@hediet
Copy link
Member Author

hediet commented Jul 7, 2021

Fixed by b81aebe.

@hediet hediet closed this as completed Jul 7, 2021
@hediet hediet modified the milestones: Backlog, July 2021 Jul 7, 2021
@rzhao271 rzhao271 added the verification-needed Verification of issue is requested label Jul 27, 2021
@DonJayamanne DonJayamanne added the verified Verification succeeded label Jul 28, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-proposal editor-core Editor basic functionality feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants