Enhance deleted state

[savetheclocktower]

Issue or RFC Endorsed by Atom's Maintainers

#12

Description of the Change

This improves handling of a “deleted” buffer state as follows:

• Distinguishes it from buffers that never had a backing file on disk; isDeleted is now a state that can be queried just like isModified or isInConflict.
• Changes isModified so that it no longer automatically returns true if the buffer's file doesn't exist. Instead, it returns false if the buffer was unmodified at time of deletion, as long as no subsequent edits have been made. Otherwise, it returns true in all scenarios in which it would've previously returned true.

This allows us to get closer to VS Code's behavior with deleted files: allow the user to remove their tab (representing a file that was deleted) from the workspace without an unnecessary “are you sure?” prompt.

Alternate Designs

#12 outlines the two possible designs. I went with the first one because it's more elegant.

Possible Drawbacks

There could be unintended side effects of narrowing the definition of isModified for one specific kind of pane item. I haven't found any yet, but we should keep an eye on this.

Verification Process

New unit tests.

Release Notes

• [text-buffer] Enhance “deleted” state to distinguish whether the buffer was in a modified state during time of file deletion.

[savetheclocktower]

#11 landed, so this can come out of draft.

I'm not aiming for 1.132 on this one; probably 1.133 if the first phase (conflict detection) goes smoothly. So this isn't urgent!

[Daeraxa]

This allows us to get closer to VS Code's behavior with deleted files: allow the user to remove their tab (representing a file that was deleted) from the workspace without an unnecessary “are you sure?” prompt.

Would this be configurable with the old behaviour? Or at least are we able to add some kind of confirm step options or warning here like a "this file has been deleted - close buffer?" prompt? My thinking here is that sometimes I end up with a deleted file that is still open in a buffer but I don't actually realise it has been deleted, so that prompt at the very least makes me think "why would this be asking me to save a modified file?". That pause has saved me from deleting data more than a few times - especially when viewing things like log files that might rotate.

Edit: Just tried it out:

• Added a new file to a project
• Opened file in a buffer
• Deleted file from the terminal

The "modified" state is no longer triggered so it doesn't prompt you to save the file or anything, it just closes if you close it. It looks the same as a saved file.

Do we have a follow up change in the main repo to actually "do" anything with this new state?

And are there any core packages that rely on the old behaviour somehow that would be affected by this? What about community packages? Do we need to be able to fall back somehow?

[savetheclocktower]

Would this be configurable with the old behaviour?

Yes — we've got a setting for the related behavior added in 1.132.0 (prompt on “conflicted” state) so we'd add a setting for this behavior as well.

Do we have a follow up change in the main repo to actually "do" anything with this new state?

Yes (EDIT: such a PR will exist after this one lands). Aside from actually adding the setting, we'll also want to modify the tabs package so that a class name is added for each of the new states (conflicted and deleted). That will allow us to match what VS Code does here and indicate the state of the buffer in the tab:

And are there any core packages that rely on the old behaviour somehow that would be affected by this? What about community packages? Do we need to be able to fall back somehow?

I cannot think of any. When I added the conflicted logic, I had to change some behavior (and specs) in the autosave package so that we wouldn't try to autosave a conflicted buffer under any circumstances. I don't think there's any sort of similar situation we have to revisit here because this is a pretty thorough behavior change — isModified() returns false for these editors, so we do not have to complicate the consuming code very much.

(Actually, since this will be governed by a setting on the Pulsar side, I'll do some testing to make sure I can preserve the existing behavior when that setting is unchecked.)

[Daeraxa]

Yes. Aside from actually adding the setting, we'll also want to modify the tabs package so that a class name is added for each of the new states (conflicted and deleted). That will allow us to match what VS Code does here and indicate the state of the buffer in the tab

Probably not the place for it as outside of the scope of this PR if we are making these further changes but we would need to decide on what takes priority in this case - are we checking the state based on the VCS or this? (if you have tabs: enableVcsColoring: true).

And the same for tree-view in theory.

[savetheclocktower]

Probably not the place for it as outside of the scope of this PR if we are making these further changes but we would need to decide on what takes priority in this case - are we checking the state based on the VCS or this? (if you have tabs: enableVcsColoring: true).

And the same for tree-view in theory.

For tree-view, certainly not. In the case of the deleted state, the file would be removed from the tree view altogether. I don't think it's particularly useful to expose the conflicted state to tree-view either.

As for tabs: the VCS statuses are actually namespaced. For instance, a modified status in VCS translates to the addition of a status-modified class name on the tab itself — whereas the more conventional non-VCS modified status (is the buffer dirty?) results in the addition of a modified class name.

The screenshots I showed you are just my proof-of-concept on my own custom UI theme. We'll want to add some sort of cosmetic treatment to the built-in UI themes; I haven't tried that yet, but I imagine it'll be similar to what VS Code does. (I know they do the red-plus-strikethrough treatment for deleted files, but am not 100% sure of how they depict conflicted files.)

Fair point about the tabs.enableVcsColoring setting; the conflicted state might want to involve a change in file icon just so it's not entirely down to the tab’s text color.

[Daeraxa]

In which case, whilst I can't comment on the code itself, the change does do what it advertises from my quick testing, and so long as we have the necessary config changes and other PRs to go with this when we bump this in the main repo, then I don't have any objections.

[savetheclocktower]

Yeah, I'll bump the version on text-buffer after landing this (either minor or major; haven't decided) and then do a release to NPM. Then the associated Pulsar PR will introduce both the necessary behavior changes and the text-buffer version bump at once so that we don't risk landing one but not the other.