New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Only destroy editors that are not modified #1182

Merged
merged 2 commits into from Sep 21, 2017

Conversation

Projects
None yet
2 participants
@50Wliu
Member

50Wliu commented Sep 15, 2017

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

Description of the Change

This PR contains three bugfixes when deleting files through the Tree View.

  1. Deleting a file would close its associated editor(s), even if there was unsaved content.
  2. Deleting a path would cause other editors that started with the deleted path to be destroyed. For example, deleting test would destroy both test and test1's editors. The original intention was to destroy files under a deleted directory, but it was lacking appropriate checks to make sure that the deleted path was a) actually a directory, and b) that it wasn't inadvertently destroying a sibling folder's editors (e.g. deleting lib/ would destroy editors under both lib/ and lib1/).
  3. The entry-deleted event passed its argument under the path variable, which I believe would override the global path require. It and its companion events now pass the deleted path as pathToDelete.

Alternate Designs

There probably are other alternate designs, but I don't see any. I started at editor.isModified() and continued from there.

Benefits

The primary benefit is that data loss no longer occurs when deleting files that have unsaved content. The other benefit is some tightening of the editor-removal code.

Possible Drawbacks

I had to add two new events: onWillDeleteEntry and onDeleteEntryFailed, as by the time onEntryDeleted is called editor.isModified() will always return true, due to its underlying file already having been deleted.

Applicable Issues

Fixes #1051

Only destroy editors that are not modified
In addition, fix a bug where files that started with thd deleted file's
name would also have their editors destroyed (e.g. deleting test would
destroy test *and* test1's editors)
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 16, 2017

Member

Specs are up and running! I've confirmed that all four of them fail on master but pass on this PR.

Member

50Wliu commented Sep 16, 2017

Specs are up and running! I've confirmed that all four of them fail on master but pass on this PR.

@iolsen

This comment has been minimized.

Show comment
Hide comment
@iolsen

iolsen Sep 18, 2017

Contributor

👍 Good fix. The new events seem reasonable to me.

Contributor

iolsen commented Sep 18, 2017

👍 Good fix. The new events seem reasonable to me.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 18, 2017

Member

👍 I'll create a PR on atom-together then.

Member

50Wliu commented Sep 18, 2017

👍 I'll create a PR on atom-together then.

@50Wliu

This comment has been minimized.

Show comment
Hide comment
@50Wliu

50Wliu Sep 21, 2017

Member

Merging. I think it's safe to break one package to prevent data loss.

Member

50Wliu commented Sep 21, 2017

Merging. I think it's safe to break one package to prevent data loss.

@50Wliu 50Wliu merged commit e33e6c9 into master Sep 21, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@50Wliu 50Wliu deleted the wl-safe-remove branch Sep 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment