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

Teach Pane to always look for a pane item's onDidTerminatePendingState function #17744

Merged
merged 2 commits into from Jul 26, 2018

Conversation

Projects
None yet
2 participants
@jasonrudolph
Copy link
Member

jasonrudolph commented Jul 25, 2018

Fixes #17535

Description of the Change

Prior to this pull request, if a PaneItem implemented the onDidDestroy function and the onDidTerminatePendingState function, then the Pane would make use of both functions. However, if PaneItem implemented an onDidTerminatePendingState function but did not implement an onDidDestroy function, the Pane wrongly ignored the onDidTerminatePendingState function.

With the changes in this pull request, the onDidTerminatePendingState function and onDidDestroy functions are treated independently:

  • If the item only implements onDidTerminatePendingState, that's fine: Pane will use it
  • If the item only implements onDidDestroy, that's fine, too: Pane will use it
  • If the item implements both functions, Pane will use both functions
  • If the item implements neither function, that's fine as well

Verification Process

  • Verify correct behavior for PaneItem that implements onDidTerminatePendingState but not onDidDestroy (i.e., perform the "Steps to Reproduce" identified in #17535)

    1. Implement a new PaneItem with an opener. Implement onDidTerminatePendingState but not onDidDestroy

      Code
      const {Emitter} = require('atom')
      
      class SomePaneItem {
        constructor () {
          this.emitter = new Emitter()
        }
      
        getElement () {
          return document.createElement('div')
        }
      
        terminatePendingState () {
          this.emitter.emit('did-terminate-pending-state')
        }
      
        onDidTerminatePendingState (callback) {
          return this.emitter.on('did-terminate-pending-state', callback)
        }
      
        getTitle () {
          return 'some-pane-item'
        }
      
        getURI () {
          return 'atom://some-pane-item'
        }
      }
    2. Open it as a pending item with a call to atom.workspace.open(item, {pending: true})

    3. Double-click on the tab title

    4. Verify that the item terminates its pending state and becomes a non-pending item

  • Verify correct behavior for PaneItem that implements onDidDestroy but not onDidTerminatePendingState

    1. Implement a new PaneItem with an opener. Implement onDidDestroy but not onDidTerminatePendingState

      Code
      const {Emitter} = require('atom')
      
      class SomePaneItem {
        constructor () {
          this.emitter = new Emitter()
        }
      
        getElement () {
          return document.createElement('div')
        }
      
        destroy () {
          this.emitter.emit('did-destroy')
        }
      
        onDidDestroy (callback) {
          return this.emitter.on('did-destroy', callback)
        }
      
        getTitle () {
          return 'some-pane-item'
        }
      
        getURI () {
          return 'atom://some-pane-item'
        }
      }
    2. Call onDidDestroy to register a callback

      Code
      item.onDidDestroy(() => console.log('Destroyed'))
    3. Open the item with a call to atom.workspace.open(item)

    4. Destroy the item with a call to item.destroy()

    5. Verify that the callback is invoked and that the pane item is removed from the workspace

jasonrudolph added some commits Jul 24, 2018

Fix #17535
Treat `onDidTerminatePendingState` and `onDidDestroy` as independent 
optional functions that a PaneItem can implement. A PaneItem is free to 
implement neither, just one of them, or both of them.

@jasonrudolph jasonrudolph requested a review from smashwilson Jul 25, 2018

@smashwilson
Copy link
Member

smashwilson left a comment

👍

@jasonrudolph jasonrudolph merged commit c52dc23 into master Jul 26, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasonrudolph jasonrudolph deleted the fix-17535 branch Jul 26, 2018

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.