Default `toggle` implementation has confusing edge case for beginners

[luca-aurelia]

The default implementation of toggle in the main module created by package-generator has an edge case that's confusing for beginners. I got tripped up by it while working through the Package: Word Count tutorial in the Flight Manual.

Reproduction Steps:

1. Execute Package Generator: Generate Package from the command palette
2. Name the new package corner-case (or any other name)
3. Reload the window (ctrl-alt-cmd-l) to load the newly created package
4. Execute Corner Case: Toggle from the command palette. A modal should appear that says The CornerCase package is Alive! It's ALIVE!
5. Execute Corner Case: Toggle from the command palette a second time. I expected the modal to disappear, but it remained. It can be successfully hidden through the menu, though: Packages -> corner-case -> Toggle

Expected behavior:

If the "It's Alive!" modal is visible, I expect executing Corner Case: Toggle from the command palette to hide the modal.

Observed behavior:

When the modal is already visible, Corner Case: Toggle does nothing if called from the command palette.

Here's a gif that shows the problem:

Atom version: 1.7.3 OS and version: OS X El Capitan 10.11.4 Installed packages:

This bug should only require the atom/package-generator to be installed.

---

It looks like the problem is caused by the call to this.modalPanel.isVisible. Here's the code for the default toggle implementation:

  toggle() {
    console.log('CornerCase was toggled!');
    return (
      this.modalPanel.isVisible() ? // this line is the problem
      this.modalPanel.hide() :
      this.modalPanel.show()
    );
  }

When the command palette is open, it hides the corner-case modal panel and this.modalPanel.isVisible() returns false. As such, control always flows into the second branch, and this.modalPanel.show() is executed.

I'd be happy to put together a small PR for this if you'd like.

[winstliu]

I'd be happy to put together a small PR for this if you'd like.

That would be awesome!

[uhlissuh]

@50Wliu @noise-machines hey, it doesn't look like a PR was ever made for this... or if it was, the problem persists. I'm thinking of taking another shot with a PR to fix this, as well as some other minor stuff.

[winstliu]

Go for it.

[bronson]

Hah, still an issue. Not sure what the right fix would be.

[saadq]

This tripped me up just now as well when I was attempting to write my first package. I guess a simple-but-not-very-elegant fix would be to just use a boolean variable like shouldDisplay that changes when toggle is called?