Merge editor and auto save

[bpasero]

(wasn't sure if we already had an issue for this)

In my mind, the merge editor should not auto save but stay dirty even when auto save is enabled. Especially since it opens dirty right when you click it, auto save fires right after.

Question is: should auto save still be supported if all conflicts are resolved? Maybe that would be a nice model:

• until there are conflicts, auto save ignores the editor
• once all conflicts are resolved, auto save kicks in

I think there is missing working copy support for this because currently auto save is driven by a capability:

vscode/src/vs/workbench/browser/parts/editor/editorAutoSave.ts

Lines 94 to 96 in 75362a6

	if (editorIdentifier?.editor.hasCapability(EditorInputCapabilities.Readonly) || editorIdentifier?.editor.hasCapability(EditorInputCapabilities.Untitled)) {
	return; // no auto save for readonly or untitled editors
	}

We could introduce a new capability CanAutoSave. But unsure how to wire this through because currently the merge editor uses the same text file model working copy as a normal editor.

Thoughts?

[hediet]

Question is: should auto save still be supported if all conflicts are resolved? Maybe that would be a nice model:

until there are conflicts, auto save ignores the editor
once all conflicts are resolved, auto save kicks in

Sounds interesting, we should try it out!

[bpasero]

Adding a new capability is quite easy, but adoption would not be very elegant: basically the merge editor when resolving the text model reference would have to update the working copy's capabilities ad-hoc and keep it up to date based on conflicts. It would have make sure to restore the original capabilities when being disposed.

Might be a bit confusing for users because this also means that when you open the original file, auto save will not work, pending conflict resolution. But that is the price for sharing the same model I guess...

[jrieken]

Might be a bit confusing for users because this also means that when you open the original file, auto save will not work, pending conflict resolution. But that is the price for sharing the same model I guess...

Yeah, I am a bit worried if that out-weights the gains, esp once we have the message to warn you when closing with unresolved conflicts

[bpasero]

esp once we have the message to warn you when closing with unresolved conflicts

With auto save enabled, the editor would not have a way to override closing of the editor because the editor is not dirty when the save occured. And even if we had a way to override closing, the fact that the editor opens dirty and is saved would still change the file right away. I think the problematic part is:

• as a user you just open a file with merge markers from scm view
• the merge editor removes all merge markers instantly
• auto save kicks in and saves the file

I see no other way solving this than blocking auto save for as long as there are conflicts, or at least as long as the merge editor is being used to edit the file.

The only alternative solution (I think we briefly discussed) is to find a way to NOT render merge decorations in the result editor without having to modify the file.

[bpasero]

60b7514 makes this a lot better, editor remains dirty, even with auto save. I wonder if we could sell this without further changes...

[jrieken]

I was wondering about the same and I think it's OK except for the fact that we are actually saving the file... E.g you see dirty, you save successfully, and still see dirty. That doesn't sound right tbh

[bpasero]

Hm yeah, maybe a topic for UX call. One could argue though that the merge editor simply needs more actions to get out of dirty state than just saving, you need to take action on all conflicts too and you cannot just do that by saving. We already have this problem irrespective of auto save because with those recent changes, the merge editor will remain dirty, even when saved, as long as there are conflicts.

Slightly related, I wonder if the merge editors confirm method should provide another option to restore the merge markers in the file, this would at least allow to revert the changes that the merge editor did to the result without having to "Undo" from the text file.

[jrieken]

Slightly related, I wonder if the merge editors confirm method should provide another option to restore the merge markers in the file, this would at least allow to revert the changes that the merge editor did to the result without having to "Undo" from the text file.

👍 I have implemented that in the joh/3wm-next branch

[jrieken]

@bpasero Given the "Discard" option I don't see a reason to do something special for preventing auto-save. Tho, what we would need is a way to trigger the confirm flow without the merge-editor being dirty. Can you help here?

[bpasero]

@jrieken maybe a new optional method/property on EditorInput such as shouldConfirm?: boolean which if not implemented will fallback to the isDirty check, would that be it? And then the merge editor would no longer mess with the dirty state but use that instead? Alternatively shouldConfirm could also just be implemented generic by checking for isDirty and you could override.

[jrieken]

Alternatively shouldConfirm could also just be implemented generic by checking for isDirty and you could override.

Pro shouldConfirm but against marrying that with dirty states. Also, I would prefer a single-property that handles close-confirmation. The classes from which I inherit have gotten soo wide that it is super hard to correlate what belongs to what and how things are supposed to be used. IMO a confirmHandler: { shouldConfirm: () => boolean, confirm:() => Promise<...> } is better than a the current flat list

[bpasero]

Yeah I like the idea of having a single property to drive this. To clarify: I would not marry this with dirty states, but if an editor decides to not implement the confirm handler, we simply do what we did before: check if the editor is dirty and bring up a generic confirm dialog if it is dirty.

[jrieken]

Closing - there is now a revert command to go back to the opening-time state

[jrieken]

To verify

• open merge editor from a conflict
• notice how the file is dirty and how some edits have already been applied
• with any further changes close the editor
• verify that get a modal that offers to discard any merge change (restore to the state at which the editor was opened)