Added prevent option to ::onWillDestroyPaneItem() #13812
Conversation
Added a new key to the object provided to the onWillDestroyPaneItem callback with the name 'prevent'. The value for this key is a method that can be used to prevent closing the given pane item. This is done via a flag in the function calling the callback notifying that it shouldn't actually destroy the pane item.
Updated the documentation for the destroyItem method in src/pane.coffee to include a short note on the new prevent functionality.
Added a test case to the pane-spec.coffee testsuite testing the functionality of the prevent method on the ::onWillDestroyPaneItem callback parameter.
Added functionality to prevent closing a pinned tabs when "Close all tabs" has been initialized. This functionality depends on the following PR in the Atom core repository: atom/atom#13812
Fixed a CoffeeScript linting issue with an empty parameter list in pane.coffees destroyItem method.
Removed the unnecessary use of a fat arrow in the new test case in the pane spec.
Fixed a broken test in pane-container-spec which was introduced by the addition of the 'prevent' key.
|
@50Wliu (or anyone else), I was wonder if you could help me here? After I pushed some commits to this PR yesterday all CI systems have failed for some mysterious reason, I can build Atom locally. Also, the logs do not point to something related to my changes, but instead to a build issue related to node-gyp (see AppVeyor)... Maybe it possible to restart the CI systems? |
|
AWS was down yesterday. I've restarted all three. |
Readded the prevent function argument to the willDestroyPaneItem which was lost in the previous merge with atom/atom's master branch.
|
+1 for this pull request, I was looking for this functionality. |
Build timeout on AppVeyor (see link) @50Wliu 😞 |
sadick254
left a comment
sadick254
left a comment
There was a problem hiding this comment.
Hey @ericcornelissen. Apologies we didn't get to review this on time. This is a really good patch and I would like for it to be part of Atom. I have noticed on the CI, the linting step is failing. An easy fix for this would be to run script/lint --fix from the root of the project. Let me know if there is anything I can do to help out.
|
Hi @sadick254, better late than never I guess 😄 I would be more than happy to pick this up again but not sure how to go about that given that I removed my original fork. I tried to fork again and (force) push changes to the same branch, but that didn't seem to do anything... Any suggestions in that regard? |
|
Hey Eric, can you try allowing maintainers edit access to your PR? You can find instructions on how to do so here if you're not familiar with the option: https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork If that works, I can see if editing from the web UI is enough to get things back in order -- at the very least, GitHub isn't showing the PR coming from an unknown repository, which is a good sign 😊. |
|
I'm not seeing the option to allow maintainers to edit the PR in the UI 😕 Any other suggestion, or should we close this and create a new PR? |
|
@ericcornelissen Let's close this PR and open another one. |
4 participants
Description of the Change
This PR implements the feature requested in issue #12376. It adds a method to the parameter of the
::onWillDestroyPaneItemcallback namedpreventwhich allows packages to prevent a tab from closing by adding an event listener via::onWillDestroyPaneItemAlternate 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
preventto 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.Why Should This Be In Core?
As explained in #12376, the problem is that it isn't part of the core. As of now it is nearly impossible to implement this as a package, and non of the solutions I could think of are even remotely elegant (e.g. recording what tab(s) closed and reopening them after they've been closed).
Benefits
It will help developers of packages prevent the unwanted closing of "special" tabs e.g. when "Close all tabs" is called by the user. This has, for example, been requested in this issue on one of my own packages.
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.
Applicable Issues
N/A