8000 Add prevent option to ::onWillDestroyPaneItem by ericcornelissen · Pull Request #22943 · atom/atom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Mar 3, 2023. It is now read-only.

Add prevent option to ::onWillDestroyPaneItem #22943

Merged
merged 1 commit into from
Sep 7, 2021
Merged

Add prevent option to ::onWillDestroyPaneItem #22943

merged 1 commit into from
Sep 7, 2021

Conversation

ericcornelissen
Copy link
Contributor

Issue or RFC Endorsed by Atom's Maintainers

#13812

Description of the Change

This PR implements the feature requested in issue #12376. It adds a method to the parameter of the ::onWillDestroyPaneItem callback named prevent which allows packages to prevent a tab from closing by adding an event listener via ::onWillDestroyPaneItem.

Alternate Designs

An alternative for the requested feature overall isn't really present, as the previous implementation would close the tab no matter what or request the user to safe in case of unsaved changes. Both of these are unwanted behaviour as described in #12376.

As for alternative ways to implement the functionality, I considered prevent to be a (boolean) value instead of a function. However, this seemed not such a good choice mainly because I find it more intuitive to use as a function, but also because JavaScript can't enforce the value to be a boolean.

Possible Drawbacks

When used incorrectly this functionality could make closing tabs impossible for users. However, simply disabling the package that uses this functionality incorrectly will solve the problem.

Verification Process

I tested the functionality by using it in the pinned-tabs-for-atom package when I created #13812. I have not yet been able to retest it yet.

Release Notes

Not applicable

@sadick254
Copy link
Contributor

@ericcornelissen Kidly take a look at the failing specs on the CI.

@ericcornelissen
Copy link
Contributor Author

Should be all good now @sadick254, I had forgotten to remove a line when I moved the changes from #13812 over to this new branch 😅

Copy link
Contributor
@sadick254 sadick254 left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@sadick254 sadick254 merged commit b3930b0 into atom:master Sep 7, 2021
@ericcornelissen ericcornelissen deleted the prevent-destroy-pane-item branch September 7, 2021 08:08
@asturur
Copy link
Contributor
asturur commented Sep 8, 2021

I moved to latest nightly (19) and atom is all broken for me now.
image

is this PR related?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0