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

Open empty editor as a pending pane item #13760

Merged
merged 9 commits into from Apr 29, 2019

Conversation

Projects
None yet
3 participants
@50Wliu
Copy link
Member

commented Feb 6, 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

If the Open Empty Editor on Startup option is checked, open it as a pending pane item. This way, if you open a folder and then a file, it doesn't stick around. However, if you just wanted to open Atom for some note-taking purposes, the existing behavior is not affected as the pending pane item will become non-pending as soon as you write something. I've always been annoyed at having to manually close the untitled editor after I open a file.

Alternate Designs

None.

Why Should This Be In Core?

Already in core.

Benefits

No longer have to manually close the Untitled tab after opening a folder.

Possible Drawbacks

Maybe some people were relying on this? Though Ctrl+N shouldn't be that bad.

Applicable Issues

None that I could find.

@50Wliu 50Wliu added the needs-review label Feb 6, 2017

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2017

Yeah...so it looks like the crash is reproducible on CI too. If anyone has any ideas for how to improve this test, I'm all ears, especially since getPendingItem is private.

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Mar 23, 2017

Ready for review. I've managed to fix the crashing spec.

50Wliu added some commits Feb 6, 2017

@50Wliu 50Wliu force-pushed the wl-open-empty-editor-as-pending branch from bd59aea to b35f2d3 Oct 31, 2017

50Wliu added some commits Jan 18, 2019

@rafeca rafeca self-assigned this Apr 26, 2019

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2019

Thanks! 😍

As discussed offline, once the conflicts are resolved we can merge this one 😃

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

@smashwilson I'm a little confused by some of these specs - for example, shouldn't this one be opening an untitled editor, as per core.openEmptyEditorOnStart?

@smashwilson

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

@50Wliu: The initial empty editor is opened by AtomEnvironment. The AtomApplication tests stub out the windows that they open, so they don't create a real environment and the empty editor isn't accounted for by the assertions there.

We do have some other cases where AtomApplication triggers the creation of an empty editor by passing {pathsToOpen: [null]} (although I'm not sure I understand why). These are currently filtered out in StubWindow although arguably we should find a way to account for them 🤔

@rafeca rafeca merged commit 7e4dfa9 into master Apr 29, 2019

2 checks passed

Atom Pull Requests #20190427.2 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@rafeca rafeca deleted the wl-open-empty-editor-as-pending branch Apr 29, 2019

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Apr 29, 2019

@rafeca whoops, this wasn't quite ready yet - my merge conflict resolution simply got rid of all the tests I wrote for it as I waited for Ash's answer :P. I'll open a followup PR adding specs in the correct location, though 🙂.

@rafeca

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

@rafeca whoops, this wasn't quite ready yet - my merge conflict resolution simply got rid of all the tests I wrote for it as I waited for Ash's answer :P. I'll open a followup PR adding specs in the correct location, though 🙂.

Ups, sorry about it 😅

@50Wliu

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2019

Hah, just kidding, again! Looks like I did have a spec asserting the pending state in atom-environment already. Sorry for the false alarm!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.