Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Don't destroy buffer when underlying file is deleted. #178

Merged
merged 6 commits into from Jan 25, 2017

Conversation

iolsen
Copy link
Contributor

@iolsen iolsen commented Oct 29, 2016

WIP toward fixing atom/tabs#306.

I actually feel the existing behavior is crazytown and the people complaining in the issue above are 100% right. IMHO there shouldn't even be an option to maintain this anti-social, silently destructive behavior. 😜

But I see there were actually quite a few people advocating for it in atom/tabs#160 so, fine. Option it is.

However I do feel strongly that reverting the default is The Right Thing because the current behavior can cause lost data in a totally unexpected way. So people who want their tabs silently closing, as they do now, will have to go change an option.

With this change a buffer whose underlying file has been deleted is essentially treated as a new, unsaved file:

  • When opened in the scope of a project its state will be saved until it's explicitly saved to a real file.
  • When opened without a project (in tandem with the fix in Fix for #10474 atom#12922) there will be a prompt to save or discard.
  • When closing the tab there will be a prompt to save or discard.

Still TODO:

@vjeux
Copy link

vjeux commented Jan 15, 2017

That would be super useful! @kyldvs made it work as an init.js script while this is getting merged in and released.

let updatedPrototype = false;
atom.workspace.observeTextEditors(function (editor) {
  if (updatedPrototype) {
    return;
  }

  const TextBuffer = editor.buffer.constructor;

  let inSubscribeToFile = false;
  const subscribeToFile = TextBuffer.prototype.subscribeToFile;
  TextBuffer.prototype.subscribeToFile = function () {
    inSubscribeToFile = true;
    const ret = subscribeToFile.apply(this, arguments);
    inSubscribeToFile = false;
    return ret;
  };

  const File = editor.buffer.file.constructor;

  let inOnDidDeleteCallback = false;
  const onDidDelete = File.prototype.onDidDelete;
  File.prototype.onDidDelete = function () {
    let fn = arguments[0]; // Should be a function.
    if (inSubscribeToFile) {
      const ogFn = fn;
      fn = function () {
        inOnDidDeleteCallback = true;
        const ret = ogFn.apply(this, arguments);
        inOnDidDeleteCallback = false;
        return ret;
      };
    }
    return onDidDelete.apply(this, [fn]);
  };

  const destroy = TextBuffer.prototype.destroy;
  TextBuffer.prototype.destroy = function() {
    if (inOnDidDeleteCallback) {
      // Don't destroy it.
    } else {
      destroy.apply(this, arguments);
    }
  }

  updatedPrototype = true;
});

@iolsen
Copy link
Contributor Author

iolsen commented Jan 19, 2017

@nathansobo I don't love this, but it works so I went ahead and pushed it anyway for discussion. See also atom/atom#13657.

Copy link
Contributor

@nathansobo nathansobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach seems fine to me other than the interface for assigning the callback.

@@ -131,6 +133,9 @@ class TextBuffer
preferredLineEnding: @preferredLineEnding
nextMarkerId: @nextMarkerId

setConfigCallbacks: (shouldDestroyBufferOnFileDelete) ->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using a constructor parameter named shouldDestroyOnFileDelete instead?

@@ -79,7 +82,7 @@ class TextBuffer
@transactCallDepth = 0
@digestWhenLastPersisted = params?.digestWhenLastPersisted ? false

@shouldDestroyBufferOnFileDelete = -> false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@iolsen iolsen merged commit 35c072c into master Jan 25, 2017
@iolsen iolsen deleted the io-unsub-on-delete branch January 25, 2017 22:45
@nathansobo
Copy link
Contributor

facebook-github-bot pushed a commit to facebookarchive/nuclide that referenced this pull request Mar 1, 2017
… files

Summary:
This modifies `NuclideTextBuffer` to support the new `shouldDestroyOnFileDelete` parameter, if it exists, which prevents files closing on delete (a common complaint).

See atom/text-buffer#178.

The implementation here slightly differs in that we don't set the path to null.. but IMO this is a better behaviour, as this way you can just save the file again to preserve it.

Reviewed By: matthewwithanm

Differential Revision: D4616627

fbshipit-source-id: f6258f0bd11801ccfd226720d302ed9a5ace66d8
zardra pushed a commit to zardra/nuclide that referenced this pull request Apr 14, 2017
… files

Summary:
This modifies `NuclideTextBuffer` to support the new `shouldDestroyOnFileDelete` parameter, if it exists, which prevents files closing on delete (a common complaint).

See atom/text-buffer#178.

The implementation here slightly differs in that we don't set the path to null.. but IMO this is a better behaviour, as this way you can just save the file again to preserve it.

Reviewed By: matthewwithanm

Differential Revision: D4616627

fbshipit-source-id: f6258f0bd11801ccfd226720d302ed9a5ace66d8
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants