Skip to content

Commit

Permalink
cleanup, comments, naming
Browse files Browse the repository at this point in the history
  • Loading branch information
@amunger
amunger committed May 15, 2023
1 parent 18c1847 commit 9aa7755
Show file tree
Hide file tree
Showing 5 changed files with 15 additions and 12 deletions.
2 changes: 1 addition & 1 deletion src/vs/workbench/contrib/files/common/dirtyFilesIndicator.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ export class DirtyFilesIndicator extends Disposable implements IWorkbenchContrib

private onWorkingCopyDidChangeDirty(workingCopy: IWorkingCopy): void {
const gotDirty = workingCopy.isDirty();

if (gotDirty && !(workingCopy.capabilities & WorkingCopyCapabilities.Untitled) && this.filesConfigurationService.getAutoSaveMode() === AutoSaveMode.AFTER_SHORT_DELAY) {
return; // do not indicate dirty of working copies that are auto saved after short delay
}
Expand All @@ -54,6 +53,7 @@ export class DirtyFilesIndicator extends Disposable implements IWorkbenchContrib

private updateActivityBadge(): void {
const dirtyCount = this.lastKnownDirtyCount = this.workingCopyService.dirtyCount;

// Indicate dirty count in badge if any
if (dirtyCount > 0) {
this.badgeHandle.value = this.activityService.showViewContainerActivity(
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/contrib/notebook/common/notebookEditorModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ export class SimpleNotebookEditorModel extends EditorModel implements INotebookE
if (this._hasAssociatedFilePath) {
this._workingCopy = await this._workingCopyManager.resolve({ associatedResource: this.resource });
} else {
this._workingCopy = await this._workingCopyManager.resolve({ untitledResource: this.resource, scratchPad: this.scratchPad });
this._workingCopy = await this._workingCopyManager.resolve({ untitledResource: this.resource, isScratchpad: this.scratchPad });
}
} else {
this._workingCopy = await this._workingCopyManager.resolve(this.resource, options?.forceReadFromFile ? { reload: { async: false, force: true } } : undefined);
Expand Down
11 changes: 5 additions & 6 deletions src/vs/workbench/services/workingCopy/common/untitledFileWorkingCopy.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export interface IUntitledFileWorkingCopyInitialContents {

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

readonly capabilities = this.scratchPad ? WorkingCopyCapabilities.Untitled | WorkingCopyCapabilities.Scratchpad : 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 @@ -137,15 +137,16 @@ export class UntitledFileWorkingCopy<M extends IUntitledFileWorkingCopyModel> ex

//#region Dirty

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

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

private setDirty(dirty: boolean): void {
if (this.dirty === dirty || this.scratchPad) {
// Scrathpad working copies are never dirty
if (this.dirty === dirty || this.isScratchpad) {
return;
}

Expand Down Expand Up @@ -192,9 +193,7 @@ export class UntitledFileWorkingCopy<M extends IUntitledFileWorkingCopyModel> ex
await this.doCreateModel(untitledContents);

// Untitled associated to file path are dirty right away as well as untitled with content
if (!this.scratchPad) {
this.setDirty(this.hasAssociatedFilePath || !!backup || Boolean(this.initialContents && this.initialContents.markDirty !== false));
}
this.setDirty(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
10 changes: 7 additions & 3 deletions src/vs/workbench/services/workingCopy/common/untitledFileWorkingCopyManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,11 @@ export interface INewOrExistingUntitledFileWorkingCopyOptions extends INewUntitl
*/
untitledResource: URI;

scratchPad?: boolean;
/**
* 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 @@ -169,7 +173,7 @@ export class UntitledFileWorkingCopyManager<M extends IUntitledFileWorkingCopyMo
// Handle untitled resource
else if (options.untitledResource?.scheme === Schemas.untitled) {
massagedOptions.untitledResource = options.untitledResource;
massagedOptions.scratchPad = options.scratchPad;
massagedOptions.isScratchpad = options.isScratchpad;

This comment has been minimized.

Sign in to view

Copy link
@bpasero

bpasero May 24, 2023

Member

@amunger while looking into adopting this for untitled text files, I noticed how here you only support the isScratchpad option if an untitled resource is passed in. But I wonder why? I think scratchpad is independent from having a resource, no? Then I think this could move into INewUntitledFileWorkingCopyOptions?

This comment has been minimized.

Sign in to view

Copy link
@amunger

amunger May 24, 2023

Author Contributor

my concern is that it would be difficult to differentiate between an untitled file and a scratchpad if they are both labelled untitled-1, and perhaps we would want something in the UI to help distinguish scratchpad tabs. Or is the lack of the dirty indicator enough?

This comment has been minimized.

Sign in to view

Copy link
@amunger

amunger May 24, 2023

Author Contributor

I suppose the same concern would apply to this case as well, so probably fine to move it to the generic options.

This comment has been minimized.

Sign in to view

Copy link
@bpasero

bpasero May 24, 2023

Member

@amunger great concern and funnily enough, when I worked on scratchpad support for text files, I made the differentiation:

vscode/src/vs/workbench/services/untitled/common/untitledTextEditorService.ts

Line 235 in 5323620

untitledResource = URI.from({ scheme: Schemas.untitled, path: options.isScratchpad ? `Scratchpad-${counter}` : `Untitled-${counter}` });

In that PR I can adjust the untitled working copy to behave the same!
}

// Take over initial value
Expand Down Expand Up @@ -202,7 +206,7 @@ export class UntitledFileWorkingCopyManager<M extends IUntitledFileWorkingCopyMo
untitledResource,
this.labelService.getUriBasenameLabel(untitledResource),
!!options.associatedResource,
!!options.scratchPad,
!!options.isScratchpad,
options.contents,
this.modelFactory,
this.saveDelegate,
Expand Down
2 changes: 1 addition & 1 deletion src/vs/workbench/services/workingCopy/test/browser/untitledFileWorkingCopyManager.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ suite('UntitledFileWorkingCopyManager', () => {

const workingCopy1 = await manager.resolve({
untitledResource: URI.from({ scheme: Schemas.untitled, path: `/myscratchpad` }),
scratchPad: true
isScratchpad: true
});

assert.strictEqual(workingCopy1.resource.scheme, Schemas.untitled);
Expand Down

0 comments on commit 9aa7755

Please sign in to comment.