Guard against a user saving while the text buffer is conflicted…

[savetheclocktower]

…by alerting them to the problem and giving them a choice between cancelling and overwriting the contents on disk.

This doesn't carry any cosmetic changes to the tabs, nor any change to the editor's behavior when the file on disk has been deleted. Those changes will come later.

This covers some of the enhancements mooted in #1040, but not all of them, so that issue will stay open even after this lands.

The problem

Suppose, like me, your attention span is not your strongest suit. You change git branches one day and fail to notice that you have uncommitted changes in one of your text buffers. When the branch changes, the contents of that buffer's backing file change on disk… but we won't update the buffer contents to match, because that would clobber the uncommitted changes you've made. We can't “rebase” your changes onto the new base file contents, either. So what should we do?

Right now, this is a time bomb. We indicate that the file is in a modified state, but we don't even try to alert you to the fact that you might be saving over the changes that were just introduced by the branch switch.

Luckily, this scenario isn't catastrophic; that's what version control is for. But this same situation can happen outside of version control; it's a hazard whenever another process can edit a file that you've got open in Pulsar.

The solution

This PR is the first step toward fixing this! It implements the bare minimum of what we ought to do in this scenario: detect the conflicted state and prompt the user about it, allowing them to decide how we proceed. Right now, the options are “Overwrite” and “Cancel”; but as an eventual enhancement, we could introduce a conflict resolution view where the user can do a three-way merge. (This UI exists in the github package, but it should be broken out into its own package (or maybe a service?) so that it can be used in non-VCS contexts.)

There is further work to be done around files that are deleted by external processes, whether or not those buffers were modified; but those scenarios are more nuanced and less urgent to fix.

Because this is experimental (and because we should let users opt out of this behavior even once it's no longer experimental), I've created a core.promptOnConflict setting that applies to pane items in general. When it's false, the pre-existing behavior is unchanged. When it's true, attempting to save a file in a conflicted state will trigger an are-you-sure dialog. While it's experimental, the default will be false so that it's opt-in; but once we're confident it behaves the way we want, we can flip the default value to true.

Also yet to be done is surfacing of these buffer states via the tabs package. We can detect the conflicted state of the buffer as soon as we notice the underlying file contents have changed; so there's no reason why we can't indicate it in the tab bar with some sort of symbol, or at least add a class name to the tab so that themes can decide if it warrants a different appearance in the first place. EDIT: Now this PR also adds a class name to each tab element — conflicted, to match existing class names like modified that are conditionally applied. No built-in UI themes have yet been updated to leverage this style, but it's there for the future.

Architecture

We particularly care about fixing this for all TextEditors… but this prompt is owned by the Pane code, and it can theoretically work identically for any pane item, TextEditor or otherwise. This PR introduces isInConflict (already a method on TextBuffer, and now present on TextEditor) as a generic way of indicating whether the current pane item is in a conflicted state. When that method returns true, Pane puts a confirmation dialog up before it calls through to the pane item's save method.

This comports with the current approach to pane item management: we use the presence of certain methods to draw conclusions about what can be done with the pane. The AbstractPaneItem interface in the types package tries to catalog all the various methods that package authors can implement for various purposes; soon isInConflict will join this list!

And in the future, if there are other ways to resolve the conflict instead of just deciding which copy wins, then that can be decided on a pane-item–by–pane-item basis. Pane items could implement an additional method whose purpose is to offer a third option to be shown in that prompt — one that aborts the save operation, but also introduces some sort of side effect that sets the user up to resolve the conflict.

Testing

I put the new specs in text-editor-spec.js, but they could arguably go into pane-spec.js for the reasons explained in the previous section.

[savetheclocktower]

Ah. This revealed some bugs with text-buffer’s implementation of conflicted status; I will have to fix those before I take this out of draft.

[savetheclocktower]

OK, this is now blocked on text-buffer#11.

[savetheclocktower]

OK, the underlying issue in text-buffer has been fixed and released, so I'm taking this back out of draft.

Out of an abundance of caution, I'm also hiding this behavior behind a config flag for now and making it opt-in. On the off chance that this regresses something that the specs don't catch, I want to ensure it doesn't disrupt the editing experience for most users. Hopefully enough of them read the release notes and opt into this behavior that we can iron out any possible kinks and perhaps make this config flag be enabled by default in time for 1.133.0.

[DeeDeeG]

Works for me! (macOS Catalina screenshot below):

Thanks for adding specs... Hopefully flipping this pref to on by default will go smoothly at some point down the line as mentioned somewhere above in this thread.

[savetheclocktower]

EDIT: Please hold, my local steps to reproduce the above may have been wrong, I may have failed to actually save out from nano when I thought I was saving out from nano.

Phew. I'd have believed this, too, considering the headaches we've had around pathwatcher!

[DeeDeeG]

BTW (likely reason(s) I got confused about that): Pulsar (mainly the github package, I think?) is rather noisy about any stuff on disk happening due to external programs (including git status runs and git branch changes) in open project dirs... Not due to this PR, I'm like 99% sure off the top of my head.

(So this comment is a bit off-topic for this PR. Thus not needing to be addressed for this PR, not even a bit blocking this PR.)

For the test binary, which I haven't granted any permissions to keychains or the like, I have to click "Deny" on like 9 "Pulsar Helper (Renderer) wants to use your credential information stored in "atom-github" in your keychain." popups in a row when saving out from nano with Pulsar open, for anything to reflect in the open Pulsar buffer of the same file. (There are three more popups for a total of 12 popups, just three of them are after the Pulsar buffer reflects the external (nano) file changes saved-to-disk.)

So I think this is what tripped me up the first time -- I observed Pulsar's behavior without clicking through enough popups to let Pulsar do its thing.

(Also: Ctrl + O in nano asks which filename to save to -- if you don't confirm then saving out doesn't occur. That also might have tripped me up if I forgot to read the little status bar at the bottom of nano and confirm the save out to disk.)