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

not dirty if auto-save soon without any save error state #189345

Merged
merged 3 commits into from
Jul 31, 2023

Conversation

amunger
Copy link
Contributor

@amunger amunger commented Jul 31, 2023

fixes #140069

@amunger amunger marked this pull request as ready for review July 31, 2023 19:53
@amunger
Copy link
Contributor Author

amunger commented Jul 31, 2023

cc @bpasero

@vscodenpa vscodenpa added this to the August 2023 milestone Jul 31, 2023
@bpasero
Copy link
Member

bpasero commented Jul 31, 2023

@amunger fyi stored file working copies know if they are in error state from saving or not, see:

/**
* Whether the stored file working copy is in the provided `state`
* or not.
*
* @param state the `FileWorkingCopyState` to check on.
*/
hasState(state: StoredFileWorkingCopyState): boolean;

@amunger
Copy link
Contributor Author

amunger commented Jul 31, 2023

@amunger fyi stored file working copies know if they are in error state from saving or not, see:

ah, thanks. That makes things simpler. I can also include conflict state in the same way, though I never figured if that would apply to notebooks.

@amunger amunger merged commit 895f824 into main Jul 31, 2023
@amunger amunger deleted the aamunger/notebookErrorState branch July 31, 2023 22:14
@bpasero
Copy link
Member

bpasero commented Aug 1, 2023

@amunger conflict state is also an error state, so I think you are fine:

// Flag as error state
this.inErrorMode = true;
// Look out for a save conflict
if ((error as FileOperationError).fileOperationResult === FileOperationResult.FILE_MODIFIED_SINCE) {
this.inConflictMode = true;
}

@@ -133,7 +149,7 @@ export class SimpleNotebookEditorModel extends EditorModel implements INotebookE
this._workingCopyListeners.add(this._workingCopy.onDidChangeOrphaned(() => this._onDidChangeOrphaned.fire()));
this._workingCopyListeners.add(this._workingCopy.onDidChangeReadonly(() => this._onDidChangeReadonly.fire()));
}
this._workingCopy.onDidChangeDirty(() => this._onDidChangeDirty.fire(), undefined, this._workingCopyListeners);
this._workingCopyListeners.add(this._workingCopy.onDidChangeDirty(() => this._onDidChangeDirty.fire(), undefined, this._workingCopyListeners));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amunger fyi this is not needed, you already pass the this._workingCopyListeners in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, I was wondering about that. It stands out as the only listener that was disposed that way in this method

@github-actions github-actions bot locked and limited conversation to collaborators Sep 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notebook shows dirty indicator with autosave on
4 participants