Skip to content
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

Added prevent option to ::onWillDestroyPaneItem() #13812

Closed
wants to merge 17 commits into from
Closed

Added prevent option to ::onWillDestroyPaneItem() #13812

wants to merge 17 commits into from

Conversation

@ericcornelissen
Copy link
Contributor

@ericcornelissen ericcornelissen commented Feb 14, 2017

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.

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

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.
ericcornelissen added a commit to ericcornelissen/pinned-tabs-for-atom that referenced this issue Feb 14, 2017
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.
@ericcornelissen
Copy link
Contributor Author

@ericcornelissen ericcornelissen commented Mar 1, 2017

@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?

@50Wliu
Copy link
Member

@50Wliu 50Wliu commented Mar 1, 2017

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.
@pmlodawski
Copy link

@pmlodawski pmlodawski commented Jul 28, 2017

+1 for this pull request, I was looking for this functionality.

@ericcornelissen
Copy link
Contributor Author

@ericcornelissen ericcornelissen commented Aug 18, 2017

Build execution time has reached the maximum allowed time for your plan (90 minutes).

Build timeout on AppVeyor (see link) @50Wliu 😞

Copy link
Contributor

@sadick254 sadick254 left a 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.

@ericcornelissen
Copy link
Contributor Author

@ericcornelissen ericcornelissen commented Sep 3, 2021

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?

@50Wliu
Copy link
Member

@50Wliu 50Wliu commented Sep 4, 2021

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 😊.

@ericcornelissen
Copy link
Contributor Author

@ericcornelissen ericcornelissen commented Sep 4, 2021

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?

@sadick254
Copy link
Contributor

@sadick254 sadick254 commented Sep 6, 2021

@ericcornelissen Let's close this PR and open another one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants