create a scratchpad untitled workingcopy for the IW model

[amunger]

for #174485

[bpasero]

I think I like the direction of the new capability and how it can be applied via the manager. But applying the capability needs to be done with care in all locations: sometimes we use the onDidChangeDirty event to update an indicator but then sometimes we use the dirtyCount or dirtyWorkingCopies or hasDirty. Since we are not disabling the dirty state for scratchpad working copies (which I think is good so that backups are triggered) we probably have to jump into each location and figure out how to treat scratchpad working copies...

[amunger]

I went through each usage of the dirtyWorkingCopies and dirtyCount getters and I'm pretty confident that they currently work as is with a couple exceptions:

saveAll after bulkEdit in bulkEditService
untitled files aren't affected? (bug #181576)

doSaveAllBeforeShutdown in the WorkingCopyBackupTracker would include scratchpad editors, except that method is never called with untitled: true
I'm also fuzzy about editor capabilities vs workingCopy capabilities and if I need to make changes there as well.

[amunger]

I'm also up for adding some tests - it looks like the WorkingCopyBackupTracker.tests doesn't currently run tests on untitled working copies, but I could try adding some in unless there is a more appropriate place to put those.

[bpasero]

@amunger I did a pass over every use of dirty state tracking for working copies and I am now even more convinced that the best way forward is to exclude scratchpad working copies from dirty tracking entirely, meaning:

• no dirty change event is emitted and they never report as dirty
• they are excluded from the various working copy service methods that allow to get dirty count or dirty working copies (this likely comes for free given the first point)

As soon as we do that, scrapbook working copies will not cause dirty indicators to appear, nor confirmation dialogs to appear, nor will any of the save actions trigger a save.

However, there are a few locations where we explicitly have to account for scrapbooks that have content. Here are the ones I found:

Editor Group View
We automatically turn a preview editor into non-preview if it is dirty (here). Otherwise, preview editors will automatically close and we probably do not want that for scratchpad that have content.

Editor Observer
Similar to the above, our editor observer will automatically close non-dirty editors based on user settings, if a maximum of editors is configured (here). We cannot allow that for scratchpads with content I think, because you would loose your data easily.

Working Copy Backup Tracker
This is the big one that I had mentioned already: dirty state is used to track backups in various locations. We will probably have to change all the usages to a new concept that:

• includes everything we have today for dirty working copies
• includes scratchpads that have content

For that we need something new that is not "dirty" but signals the intent. I am open for suggestions.

[bpasero]

Did a quick review

[bpasero]

I like all the changes but think we now need to focus on backup tracker. I am not convinced we have all the changes yet to ensure backups are present, including shutdown.

Backup tracker makes extensive use of tracking "dirty" working copies. Scratchpad working copies are never dirty. You schedule a backup in onDidChangeContent which is important, but there are other places that are not tracked. Can you make a pass and see what to do? I think we will have to introduce a new field on the working copy service and maybe on the working copy itself to signal back that content has changed in the past and that a backup should be done.

I think you only need to check all the (3?) workingCopyBackupTracker.ts files for this.

[amunger]

oof, yes I missed that since the IW does it's own backup at the moment (hopefully can integrate as a next step)

common
schedule backup when registered with Backup Tracker and dirty - update should also include scratchpads with content
schedule backup on Dirty Change - won't trigger for scratchpads, should be covered by onContentChange except for dropping the backups
schedule backup on Content Change - applies to scratchpad, update discard the backup when un-modified since we miss that from onDirtyChange

web
onFinalBeforeShutdown - update scratchpads that failed backup should veto
without hot-exit, scratchpads will be discarded by design

electron-sandbox
handleDirtyBeforeShutdown - update should include scratchpads with content
if backups fail, we should veto shutdown
without backups, scratchpads will be discarded by design

[bpasero]

I really like the term "modified" to signal backups are needed for a working copy. But then we should also make sure that when we have a comment, we refer to "modified" as term and not "dirty" or "unsaved changes". Makes it easier to understand the code imho 👍

I left lots of little review comments but on a high level think that you missed some cases where a modified working copy without backup should still block the shutdown from happening. If a user has a modified scrapbook and hot exit is disabled we still need to show a dialog, to not loose user data.

Overall I think we are getting close to done.

[amunger]

With the change to block shutdown for modified scratchpads, I had to remove the IWs implementation since we need to think more about what 'save' will do for an IW if we're going to force users to take that action.

[bpasero]

Getting there 👍

Can you search for dirty in src/vs/workbench/services/workingCopy/common/workingCopyBackupTracker.ts and review where we still need to deal with modified and not dirty, I think its not done yet.

[bpasero]

With the change to block shutdown for modified scratchpads, I had to remove the IWs implementation since we need to think more about what 'save' will do for an IW if we're going to force users to take that action.

I would expect that I can save a scratchpad when I do this explicitly and it would open a dialog asking where to save to, similar to untitled files.

[bpasero]

I think this is good to go. I spend some time thinking about 2 somewhat loose ends that are slightly off:

• do we need a onDidChangeModified event given we have introduced modified as a concept? or should we maybe rename the existing onDidChangeContent? Should we drop content-change notion and only use modified?
• if you search for markDirty in our sources you find a way for clients to mark a untitled working copy as dirty. should this be renamed to markModified or is the correct term as it is now?

I am not sure these 2 bullets warrant for blocking this PR any l longer though, thus approving.