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

create a scratchpad untitled workingcopy for the IW model #181280

Merged
merged 32 commits into from
May 19, 2023
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
6b634eb
create a scratch untitled workingcopy for the IW model
amunger May 1, 2023
5924fe1
unused class
amunger May 1, 2023
d79de57
remove IW fs provider
amunger May 1, 2023
a3cf720
remove interactive resource schema references
amunger May 3, 2023
186d20e
remove another dirty indicator, do not save on close, feedback
amunger May 3, 2023
228afb9
use correct operator
amunger May 4, 2023
50d7514
added unsavedWorkingCopies helper
amunger May 4, 2023
aefc4d6
cleanup
amunger May 4, 2023
baa682c
Merge branch 'main' into aamunger/scrapbookIW
amunger May 9, 2023
076d230
Merge branch 'main' into aamunger/scrapbookIW
amunger May 11, 2023
51f124f
Scratchpads cannot be dirty
amunger May 12, 2023
ac30f78
cleanup
amunger May 12, 2023
ff0fc49
scratchpad dirty unit test
amunger May 12, 2023
3956aaf
Merge branch 'main' into aamunger/scrapbookIW
amunger May 12, 2023
1e335ea
Update src/vs/workbench/services/workingCopy/common/untitledFileWorki…
amunger May 15, 2023
18c1847
Merge branch 'main' into aamunger/scrapbookIW
amunger May 15, 2023
9aa7755
cleanup, comments, naming
amunger May 15, 2023
9af42e8
updated comment
amunger May 15, 2023
381a792
added editor capability
amunger May 15, 2023
dc41117
editor observer test
amunger May 15, 2023
7e3b8c8
fix
amunger May 15, 2023
fb9e06d
:lipstick:
bpasero May 16, 2023
ed51968
add `modified` member to implement backups for scratchpads which are …
amunger May 16, 2023
8a9bfa8
Merge remote-tracking branch 'origin/main' into aamunger/scrapbookIW
amunger May 16, 2023
cb2ba4c
broken test
amunger May 16, 2023
71cc49b
:lipstick:
bpasero May 17, 2023
e0d2557
feedback
amunger May 17, 2023
2aa63c8
prompt users when scratchpads will be lost on close
amunger May 17, 2023
6d4d5a5
cleanup
amunger May 17, 2023
f5e494e
reverted notebook/IW changes
amunger May 17, 2023
b0b5c4e
Merge branch 'main' into aamunger/scrapbookIW
bpasero May 18, 2023
4864d33
feedback
amunger May 18, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/vs/workbench/browser/parts/editor/editorGroupView.ts
Original file line number Diff line number Diff line change
Expand Up @@ -982,9 +982,15 @@ export class EditorGroupView extends Themable implements IEditorGroupView {
this._onWillOpenEditor.fire({ editor, groupId: this.id });

// Determine options
const pinned = options?.sticky
|| !this.accessor.partOptions.enablePreview
|| editor.isDirty()
|| (options?.pinned ?? typeof options?.index === 'number' /* unless specified, prefer to pin when opening with index */)
|| (typeof options?.index === 'number' && this.model.isSticky(options.index))
|| editor.hasCapability(EditorInputCapabilities.Scratchpad);
const openEditorOptions: IEditorOpenOptions = {
index: options ? options.index : undefined,
pinned: options?.sticky || !this.accessor.partOptions.enablePreview || editor.isDirty() || (options?.pinned ?? typeof options?.index === 'number' /* unless specified, prefer to pin when opening with index */) || (typeof options?.index === 'number' && this.model.isSticky(options.index)),
pinned,
sticky: options?.sticky || (typeof options?.index === 'number' && this.model.isSticky(options.index)),
active: this.count === 0 || !options || !options.inactive,
supportSideBySide: internalOptions?.supportSideBySide
Expand Down
10 changes: 5 additions & 5 deletions src/vs/workbench/browser/parts/editor/editorsObserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Licensed under the MIT License. See License.txt in the project root for license information.
*--------------------------------------------------------------------------------------------*/

import { IEditorFactoryRegistry, IEditorIdentifier, GroupIdentifier, EditorExtensions, IEditorPartOptionsChangeEvent, EditorsOrder, GroupModelChangeKind } from 'vs/workbench/common/editor';
import { IEditorFactoryRegistry, IEditorIdentifier, GroupIdentifier, EditorExtensions, IEditorPartOptionsChangeEvent, EditorsOrder, GroupModelChangeKind, EditorInputCapabilities } from 'vs/workbench/common/editor';
import { EditorInput } from 'vs/workbench/common/editor/editorInput';
import { SideBySideEditorInput } from 'vs/workbench/common/editor/sideBySideEditorInput';
import { dispose, Disposable, DisposableStore } from 'vs/base/common/lifecycle';
Expand Down Expand Up @@ -345,8 +345,8 @@ export class EditorsObserver extends Disposable {
let mostRecentEditorsCountingForLimit: IEditorIdentifier[];
if (this.editorGroupsService.partOptions.limit?.excludeDirty) {
mostRecentEditorsCountingForLimit = mostRecentEditors.filter(({ editor }) => {
if (editor.isDirty() && !editor.isSaving()) {
return false;
if ((editor.isDirty() && !editor.isSaving()) || editor.hasCapability(EditorInputCapabilities.Scratchpad)) {
return false; // not dirty editors (unless in the process of saving) or scratchpads
}

return true;
Expand All @@ -361,8 +361,8 @@ export class EditorsObserver extends Disposable {

// Extract least recently used editors that can be closed
const leastRecentlyClosableEditors = mostRecentEditorsCountingForLimit.reverse().filter(({ editor, groupId }) => {
if (editor.isDirty() && !editor.isSaving()) {
return false; // not dirty editors (unless in the process of saving)
if ((editor.isDirty() && !editor.isSaving()) || editor.hasCapability(EditorInputCapabilities.Scratchpad)) {
return false; // not dirty editors (unless in the process of saving) or scratchpads
}

if (exclude && editor === exclude.editor && groupId === exclude.groupId) {
Expand Down
8 changes: 7 additions & 1 deletion src/vs/workbench/common/editor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,13 @@ export const enum EditorInputCapabilities {
* Signals that the editor is composed of multiple editors
* within.
*/
MultipleEditors = 1 << 8
MultipleEditors = 1 << 8,

/**
* Signals that the editor cannot be in a dirty state
* and may still have unsaved changes
*/
Scratchpad = 1 << 9
}

export type IUntypedEditorInput = IResourceEditorInput | ITextResourceEditorInput | IUntitledTextResourceEditorInput | IResourceDiffEditorInput | IResourceSideBySideEditorInput | IResourceMergeEditorInput;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ export class SearchEditorInput extends EditorInput {
readonly onDidChangeContent = input.onDidChangeContent;
readonly onDidSave = input.onDidSave;
isDirty(): boolean { return input.isDirty(); }
isModified(): boolean { return input.isDirty(); }
backup(token: CancellationToken): Promise<IWorkingCopyBackup> { return input.backup(token); }
save(options?: ISaveOptions): Promise<boolean> { return input.save(0, options).then(editor => !!editor); }
revert(options?: IRevertOptions): Promise<void> { return input.revert(0, options); }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*--------------------------------------------------------------------------------------------*/

import * as assert from 'assert';
import { IEditorFactoryRegistry, EditorExtensions } from 'vs/workbench/common/editor';
import { IEditorFactoryRegistry, EditorExtensions, EditorInputCapabilities } from 'vs/workbench/common/editor';
import { URI } from 'vs/base/common/uri';
import { workbenchInstantiationService, TestFileEditorInput, registerTestEditor, TestEditorPart, createEditorPart, registerTestSideBySideEditor } from 'vs/workbench/test/browser/workbenchTestServices';
import { Registry } from 'vs/platform/registry/common/platform';
Expand Down Expand Up @@ -628,4 +628,35 @@ suite('EditorsObserver', function () {
assert.strictEqual(observer.hasEditor({ resource: input3.resource, typeId: input3.typeId, editorId: input3.editorId }), true);
assert.strictEqual(observer.hasEditor({ resource: input4.resource, typeId: input4.typeId, editorId: input4.editorId }), true);
});

test('observer does not close scratchpads', async () => {
const [part] = await createPart();
part.enforcePartOptions({ limit: { enabled: true, value: 3 } });

const storage = new TestStorageService();
const observer = disposables.add(new EditorsObserver(part, storage));

const rootGroup = part.activeGroup;

const input1 = new TestFileEditorInput(URI.parse('foo://bar1'), TEST_EDITOR_INPUT_ID);
input1.capabilities = EditorInputCapabilities.Untitled | EditorInputCapabilities.Scratchpad;
const input2 = new TestFileEditorInput(URI.parse('foo://bar2'), TEST_EDITOR_INPUT_ID);
const input3 = new TestFileEditorInput(URI.parse('foo://bar3'), TEST_EDITOR_INPUT_ID);
const input4 = new TestFileEditorInput(URI.parse('foo://bar4'), TEST_EDITOR_INPUT_ID);

await rootGroup.openEditor(input1, { pinned: true });
await rootGroup.openEditor(input2, { pinned: true });
await rootGroup.openEditor(input3, { pinned: true });
await rootGroup.openEditor(input4, { pinned: true });

assert.strictEqual(rootGroup.count, 3);
assert.strictEqual(rootGroup.contains(input1), true);
assert.strictEqual(rootGroup.contains(input2), false);
assert.strictEqual(rootGroup.contains(input3), true);
assert.strictEqual(rootGroup.contains(input4), true);
assert.strictEqual(observer.hasEditor({ resource: input1.resource, typeId: input1.typeId, editorId: input1.editorId }), true);
assert.strictEqual(observer.hasEditor({ resource: input2.resource, typeId: input2.typeId, editorId: input2.editorId }), false);
assert.strictEqual(observer.hasEditor({ resource: input3.resource, typeId: input3.typeId, editorId: input3.editorId }), true);
assert.strictEqual(observer.hasEditor({ resource: input4.resource, typeId: input4.typeId, editorId: input4.editorId }), true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,10 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil
return this.dirty;
}

isModified(): boolean {
amunger marked this conversation as resolved.
Show resolved Hide resolved
return this.isDirty();
}

setDirty(dirty: boolean): void {
if (!this.isResolved()) {
return; // only resolved models can be marked dirty
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ suite('Files - TextFileEditorModel', () => {
model.updateTextEditorModel(createTextBufferFactory('bar'));
assert.ok(getLastModifiedTime(model) <= Date.now());
assert.ok(model.hasState(TextFileEditorModelState.DIRTY));
assert.ok(model.isModified());

assert.strictEqual(accessor.workingCopyService.dirtyCount, 1);
assert.strictEqual(accessor.workingCopyService.isDirty(model.resource, model.typeId), true);
Expand All @@ -123,6 +124,7 @@ suite('Files - TextFileEditorModel', () => {

assert.ok(model.hasState(TextFileEditorModelState.SAVED));
assert.ok(!model.isDirty());
assert.ok(!model.isModified());
assert.ok(savedEvent);
assert.ok((savedEvent as ITextFileEditorModelSaveEvent).stat);
assert.strictEqual((savedEvent as ITextFileEditorModelSaveEvent).reason, SaveReason.AUTO);
Expand Down Expand Up @@ -182,6 +184,7 @@ suite('Files - TextFileEditorModel', () => {

assert.ok(model.hasState(TextFileEditorModelState.ERROR));
assert.ok(model.isDirty());
assert.ok(model.isModified());
assert.ok(saveErrorEvent);

assert.strictEqual(accessor.workingCopyService.dirtyCount, 1);
Expand Down Expand Up @@ -238,6 +241,7 @@ suite('Files - TextFileEditorModel', () => {

assert.ok(model.hasState(TextFileEditorModelState.ERROR));
assert.ok(model.isDirty());
assert.ok(model.isModified());
assert.ok(saveErrorEvent);

assert.strictEqual(accessor.workingCopyService.dirtyCount, 1);
Expand Down Expand Up @@ -268,6 +272,7 @@ suite('Files - TextFileEditorModel', () => {

assert.ok(model.hasState(TextFileEditorModelState.CONFLICT));
assert.ok(model.isDirty());
assert.ok(model.isModified());
assert.ok(saveErrorEvent);

assert.strictEqual(accessor.workingCopyService.dirtyCount, 1);
Expand Down Expand Up @@ -457,6 +462,7 @@ suite('Files - TextFileEditorModel', () => {
await model.resolve();
model.updateTextEditorModel(createTextBufferFactory('foo'));
assert.ok(model.isDirty());
assert.ok(model.isModified());

assert.strictEqual(accessor.workingCopyService.dirtyCount, 1);
assert.strictEqual(accessor.workingCopyService.isDirty(model.resource, model.typeId), true);
Expand All @@ -471,6 +477,7 @@ suite('Files - TextFileEditorModel', () => {
model = accessor.workingCopyService.get(model) as TextFileEditorModel;

assert.strictEqual(model.isDirty(), false);
assert.strictEqual(model.isModified(), false);
assert.strictEqual(eventCounter, 1);

assert.ok(workingCopyEvent);
Expand All @@ -497,12 +504,14 @@ suite('Files - TextFileEditorModel', () => {
await model.resolve();
model.updateTextEditorModel(createTextBufferFactory('foo'));
assert.ok(model.isDirty());
assert.ok(model.isModified());

assert.strictEqual(accessor.workingCopyService.dirtyCount, 1);
assert.strictEqual(accessor.workingCopyService.isDirty(model.resource, model.typeId), true);

await model.revert({ soft: true });
assert.strictEqual(model.isDirty(), false);
assert.strictEqual(model.isModified(), false);
assert.strictEqual(model.textEditorModel!.getValue(), 'foo');
assert.strictEqual(eventCounter, 1);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,10 @@ export class UntitledTextEditorModel extends BaseTextEditorModel implements IUnt
return this.dirty;
}

isModified(): boolean {
return this.isDirty();
}

private setDirty(dirty: boolean): void {
if (this.dirty === dirty) {
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,27 +32,27 @@ export class BrowserWorkingCopyBackupTracker extends WorkingCopyBackupTracker im
protected onFinalBeforeShutdown(reason: ShutdownReason): boolean {

// Web: we cannot perform long running in the shutdown phase
// As such we need to check sync if there are any dirty working
// As such we need to check sync if there are any modified working
// copies that have not been backed up yet and then prevent the
// shutdown if that is the case.

const dirtyWorkingCopies = this.workingCopyService.dirtyWorkingCopies;
if (!dirtyWorkingCopies.length) {
return false; // no dirty: no veto
const modifiedWorkingCopies = this.workingCopyService.modifiedWorkingCopies;
if (!modifiedWorkingCopies.length) {
return false; // nothing modified: no veto
}

if (!this.filesConfigurationService.isHotExitEnabled) {
return true; // dirty without backup: veto
return true; // modified without backup: veto
}

for (const dirtyWorkingCopy of dirtyWorkingCopies) {
if (!this.workingCopyBackupService.hasBackupSync(dirtyWorkingCopy, this.getContentVersion(dirtyWorkingCopy))) {
for (const modifiedWorkingCopy of modifiedWorkingCopies) {
if (!this.workingCopyBackupService.hasBackupSync(modifiedWorkingCopy, this.getContentVersion(modifiedWorkingCopy))) {
this.logService.warn('Unload veto: pending backups');

return true; // dirty without backup: veto
return true; // modified without backup: veto
}
}

return false; // dirty with backups: no veto
return false; // modified and backed up: no veto
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,15 @@ export abstract class ResourceWorkingCopy extends Disposable implements IResourc

//#endregion

//#region Modified Tracking

// Overridden when the Working Copies need to differentiate between
// dirty and modified e.g. scratchpads
isModified(): boolean {
return this.isDirty();
bpasero marked this conversation as resolved.
Show resolved Hide resolved
}

//#endregion

//#region Abstract

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,9 @@ export interface IUntitledFileWorkingCopyModelContentChangedEvent {

/**
* Flag that indicates that the content change should
* clear the dirty flag, e.g. because the contents are
* clear the dirty/modified flags, e.g. because the contents are
* back to being empty or back to an initial state that
* should not be considered as dirty.
* should not be considered as modified.
*/
readonly isInitial: boolean;
}
Expand Down Expand Up @@ -92,7 +92,7 @@ export interface IUntitledFileWorkingCopyInitialContents {

export class UntitledFileWorkingCopy<M extends IUntitledFileWorkingCopyModel> extends Disposable implements IUntitledFileWorkingCopy<M> {

readonly capabilities = WorkingCopyCapabilities.Untitled;
readonly capabilities = this.isScratchpad ? WorkingCopyCapabilities.Untitled | WorkingCopyCapabilities.Scratchpad : WorkingCopyCapabilities.Untitled;

private _model: M | undefined = undefined;
get model(): M | undefined { return this._model; }
Expand Down Expand Up @@ -121,6 +121,7 @@ export class UntitledFileWorkingCopy<M extends IUntitledFileWorkingCopyModel> ex
readonly resource: URI,
readonly name: string,
readonly hasAssociatedFilePath: boolean,
private readonly isScratchpad: boolean,
private readonly initialContents: IUntitledFileWorkingCopyInitialContents | undefined,
private readonly modelFactory: IUntitledFileWorkingCopyModelFactory<M>,
private readonly saveDelegate: IUntitledFileWorkingCopySaveDelegate<M>,
Expand All @@ -136,19 +137,25 @@ export class UntitledFileWorkingCopy<M extends IUntitledFileWorkingCopyModel> ex

//#region Dirty

private dirty = this.hasAssociatedFilePath || Boolean(this.initialContents && this.initialContents.markDirty !== false);
private modified = this.hasAssociatedFilePath || Boolean(this.initialContents && this.initialContents.markDirty !== false);

isDirty(): boolean {
return this.dirty;
return this.modified && !this.isScratchpad; // Scratchpad working copies are never dirty
}

private setDirty(dirty: boolean): void {
if (this.dirty === dirty) {
isModified(): boolean {
return this.modified;
}

private setModified(modified: boolean): void {
if (this.modified === modified) {
return;
}

this.dirty = dirty;
this._onDidChangeDirty.fire();
this.modified = modified;
if (!this.isScratchpad) {
this._onDidChangeDirty.fire();
}
}

//#endregion
Expand Down Expand Up @@ -189,8 +196,8 @@ export class UntitledFileWorkingCopy<M extends IUntitledFileWorkingCopyModel> ex
// Create model
await this.doCreateModel(untitledContents);

// Untitled associated to file path are dirty right away as well as untitled with content
this.setDirty(this.hasAssociatedFilePath || !!backup || Boolean(this.initialContents && this.initialContents.markDirty !== false));
// Untitled associated to file path are modified right away as well as untitled with content
this.setModified(this.hasAssociatedFilePath || !!backup || Boolean(this.initialContents && this.initialContents.markDirty !== false));

// If we have initial contents, make sure to emit this
// as the appropriate events to the outside.
Expand Down Expand Up @@ -220,16 +227,16 @@ export class UntitledFileWorkingCopy<M extends IUntitledFileWorkingCopyModel> ex

private onModelContentChanged(e: IUntitledFileWorkingCopyModelContentChangedEvent): void {

// Mark the untitled file working copy as non-dirty once its
// Mark the untitled file working copy as non-modified once its
// in case provided by the change event and in case we do not
// have an associated path set
if (!this.hasAssociatedFilePath && e.isInitial) {
this.setDirty(false);
this.setModified(false);
}

// Turn dirty otherwise
// Turn modified otherwise
else {
this.setDirty(true);
this.setModified(true);
}

// Emit as general content change event
Expand Down Expand Up @@ -287,8 +294,8 @@ export class UntitledFileWorkingCopy<M extends IUntitledFileWorkingCopyModel> ex
async revert(): Promise<void> {
this.trace('revert()');

// No longer dirty
this.setDirty(false);
// No longer modified
this.setModified(false);

// Emit as event
this._onDidRevert.fire();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ export interface INewOrExistingUntitledFileWorkingCopyOptions extends INewUntitl
* Note: the resource will not be used unless the scheme is `untitled`.
*/
untitledResource: URI;

amunger marked this conversation as resolved.
Show resolved Hide resolved
/**
* A flag that will prevent the working copy from appearing dirty in the UI
* and not show a confirmation dialog when closed with unsaved content.
*/
isScratchpad?: boolean;
}

type IInternalUntitledFileWorkingCopyOptions = INewUntitledFileWorkingCopyOptions & INewUntitledFileWorkingCopyWithAssociatedResourceOptions & INewOrExistingUntitledFileWorkingCopyOptions;
Expand Down Expand Up @@ -167,6 +173,7 @@ export class UntitledFileWorkingCopyManager<M extends IUntitledFileWorkingCopyMo
// Handle untitled resource
else if (options.untitledResource?.scheme === Schemas.untitled) {
massagedOptions.untitledResource = options.untitledResource;
massagedOptions.isScratchpad = options.isScratchpad;
}

// Take over initial value
Expand Down Expand Up @@ -199,6 +206,7 @@ export class UntitledFileWorkingCopyManager<M extends IUntitledFileWorkingCopyMo
untitledResource,
this.labelService.getUriBasenameLabel(untitledResource),
!!options.associatedResource,
!!options.isScratchpad,
options.contents,
this.modelFactory,
this.saveDelegate,
Expand Down
Loading