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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/vs/workbench/contrib/notebook/common/notebookCommon.ts
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,8 @@ export interface INotebookEditorModel extends IEditorModel {
readonly resource: URI;
readonly viewType: string;
readonly notebook: INotebookTextModel | undefined;
readonly hasErrorState: boolean;
readonly hasConflictState: boolean;
isResolved(): this is IResolvedNotebookEditorModel;
isDirty(): boolean;
isModified(): boolean;
Expand Down
12 changes: 11 additions & 1 deletion src/vs/workbench/contrib/notebook/common/notebookEditorInput.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import { VSBuffer } from 'vs/base/common/buffer';
import { IWorkingCopyIdentifier } from 'vs/workbench/services/workingCopy/common/workingCopy';
import { NotebookProviderInfo } from 'vs/workbench/contrib/notebook/common/notebookProvider';
import { NotebookPerfMarks } from 'vs/workbench/contrib/notebook/common/notebookPerformance';
import { IFilesConfigurationService } from 'vs/workbench/services/filesConfiguration/common/filesConfigurationService';
import { AutoSaveMode, IFilesConfigurationService } from 'vs/workbench/services/filesConfiguration/common/filesConfigurationService';
import { IExtensionService } from 'vs/workbench/services/extensions/common/extensions';
import { localize } from 'vs/nls';
import { IEditorService } from 'vs/workbench/services/editor/common/editorService';
Expand Down Expand Up @@ -156,6 +156,16 @@ export class NotebookEditorInput extends AbstractResourceEditorInput {
return this._editorModelReference.object.isDirty();
}

override isSaving(): boolean {
const model = this._editorModelReference?.object;
if (!model || !model.isDirty() || model.hasErrorState || model.hasConflictState) {
return false; // require the model to be dirty and not in error or conflict state
}

// if a short auto save is configured, treat this as being saved
return this.filesConfigurationService.getAutoSaveMode() === AutoSaveMode.AFTER_SHORT_DELAY;
}

override async save(group: GroupIdentifier, options?: ISaveOptions): Promise<EditorInput | IUntypedEditorInput | undefined> {
if (this._editorModelReference) {

Expand Down
18 changes: 17 additions & 1 deletion src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,22 @@ export class SimpleNotebookEditorModel extends EditorModel implements INotebookE
}
}

get hasErrorState(): boolean {
if (this._workingCopy && 'hasState' in this._workingCopy) {
return this._workingCopy.hasState(StoredFileWorkingCopyState.ERROR);
}

return false;
}

get hasConflictState(): boolean {
if (this._workingCopy && 'hasState' in this._workingCopy) {
return this._workingCopy.hasState(StoredFileWorkingCopyState.CONFLICT);
}

return false;
}

revert(options?: IRevertOptions): Promise<void> {
assertType(this.isResolved());
return this._workingCopy!.revert(options);
Expand All @@ -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


this._workingCopyListeners.add(this._workingCopy.onWillDispose(() => {
this._workingCopyListeners.clear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,14 @@ export class NotebookEditorTestModel extends EditorModel implements INotebookEdi
return this._dirty;
}

get hasErrorState() {
return false;
}

get hasConflictState() {
return false;
}

isModified(): boolean {
return this._dirty;
}
Expand Down