From 6d1b62b1b6606f965368f82ebaa17f7ce9f5f1bc Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 10:43:50 -0400 Subject: [PATCH 01/11] Never render or update the active context after destruction --- lib/github-package.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/github-package.js b/lib/github-package.js index 5f727b7e8c..5b87b11331 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -188,6 +188,10 @@ export default class GithubPackage { @autobind rerender(callback) { + if (this.workspace.isDestroyed()) { + return; + } + if (!this.element) { this.element = document.createElement('div'); this.subscriptions.add(new Disposable(() => { @@ -391,6 +395,10 @@ export default class GithubPackage { } async updateActiveContext(savedState = {}) { + if (this.workspace.isDestroyed()) { + return; + } + this.switchboard.didBeginActiveContextUpdate(); const nextActiveContext = await this.getNextContext(savedState); From 9ca2a3cea3274f98f70d10401292b4b9fd16b12b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 10:44:33 -0400 Subject: [PATCH 02/11] Delegate git operations in async state methods to the current state --- lib/models/repository-states/state.js | 33 +++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 95a8dd743c..53dfee839e 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -407,6 +407,12 @@ export default class State { return this.repository.getWorkingDirectoryPath(); } + // Call methods on the active Repository state, even if the state has transitioned beneath you. + // Use this to perform operations within `start()` methods to guard against interrupted state transitions. + current() { + return this.repository.state; + } + // Return a Promise that will resolve once the state transitions from Loading. getLoadPromise() { return this.repository.getLoadPromise(); @@ -439,16 +445,39 @@ export default class State { return this.repository.emitter.emit('did-update'); } + // Direct git access + // Non-delegated git operations for internal use within states. + + directIsGitRepository() { + return Promise.resolve(false); + } + + directGetConfig(key, options = {}) { + return Promise.resolve(null); + } + + directGetBlobContents() { + return Promise.reject(new Error('Not a valid object name')); + } + + // Deferred operations + // Direct raw git operations to the current state, even if the state has been changed. Use these methods within + // start() methods. + + isGitRepository() { + return this.current().directIsGitRepository(); + } + // Parse a DiscardHistory payload from the SHA recorded in config. async loadHistoryPayload() { - const historySha = await this.git().getConfig('atomGithub.historySha'); + const historySha = await this.current().directGetConfig('atomGithub.historySha'); if (!historySha) { return {}; } let blob; try { - blob = await this.git().getBlobContents(historySha); + blob = await this.current().directBlobContents(historySha); } catch (e) { if (/Not a valid object name/.test(e.stdErr)) { return {}; From ae017aae76a236be717416db2bca1089d70ed323 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 10:44:54 -0400 Subject: [PATCH 03/11] Make Loading.start() cancellable --- lib/models/repository-states/loading.js | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/lib/models/repository-states/loading.js b/lib/models/repository-states/loading.js index a70420f557..00fe19d758 100644 --- a/lib/models/repository-states/loading.js +++ b/lib/models/repository-states/loading.js @@ -6,7 +6,7 @@ import State from './state'; */ export default class Loading extends State { async start() { - if (await this.git().isGitRepository()) { + if (await this.isGitRepository()) { const history = await this.loadHistoryPayload(); return this.transitionTo('Present', history); } else { @@ -31,6 +31,18 @@ export default class Loading extends State { showGitTabLoading() { return true; } + + directIsGitRepository() { + return this.git().isGitRepository(); + } + + directGetConfig(key, options) { + return this.git().getConfig(key, options); + } + + directGetBlobContents(sha) { + return this.git().getBlobContents(sha); + } } State.register(Loading); From d2ce1031388b7f1bcba4ba3594e084d49defe842 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 10:45:14 -0400 Subject: [PATCH 04/11] Only permit state transitions initiated by the current state --- lib/models/repository.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/models/repository.js b/lib/models/repository.js index e3f8d340a7..347d9af56e 100644 --- a/lib/models/repository.js +++ b/lib/models/repository.js @@ -67,11 +67,18 @@ export default class Repository { // State management ////////////////////////////////////////////////////////////////////////////////////////////////// transition(currentState, StateConstructor, ...payload) { + if (currentState !== this.state) { + // Attempted transition from a non-active state, most likely from an asynchronous start() method. + return Promise.resolve(); + } + const nextState = new StateConstructor(this, ...payload); this.state = nextState; this.emitter.emit('did-change-state', {from: currentState, to: this.state}); - this.emitter.emit('did-update'); + if (!this.isDestroyed()) { + this.emitter.emit('did-update'); + } return this.state.start(); } From fbbeb849d8e0b1eb39b3b6d7889499780b53aab6 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 10:45:45 -0400 Subject: [PATCH 05/11] Consolidate .then callbacks --- lib/views/dock-item.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/views/dock-item.js b/lib/views/dock-item.js index d6ac2a1c25..fde8766bf1 100644 --- a/lib/views/dock-item.js +++ b/lib/views/dock-item.js @@ -71,8 +71,8 @@ export default class DockItem extends React.Component { } } else { Promise.resolve(this.props.workspace.open(itemToAdd)) - .then(item => { this.dockItem = item; }) - .then(() => { + .then(item => { + this.dockItem = item; if (this.props.activate) { this.activate(); } }); } From 4304f5b75f794f86087e326f2ef6f8746eb931cf Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 10:45:58 -0400 Subject: [PATCH 06/11] Don't activate after close --- lib/views/dock-item.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/views/dock-item.js b/lib/views/dock-item.js index fde8766bf1..d4791ea9eb 100644 --- a/lib/views/dock-item.js +++ b/lib/views/dock-item.js @@ -102,7 +102,7 @@ export default class DockItem extends React.Component { activate() { setTimeout(() => { - if (!this.dockItem) { return; } + if (!this.dockItem || this.didCloseItem) { return; } const pane = this.props.workspace.paneForItem(this.dockItem); if (pane) { From a0160d6ffd6bceca714a44d5feb508ae485ea3df Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 11:36:05 -0400 Subject: [PATCH 07/11] Allow DockItem to handle pane activation instead of Workspace.open() --- lib/views/dock-item.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/views/dock-item.js b/lib/views/dock-item.js index d4791ea9eb..6959c96a17 100644 --- a/lib/views/dock-item.js +++ b/lib/views/dock-item.js @@ -70,7 +70,7 @@ export default class DockItem extends React.Component { this.activate(); } } else { - Promise.resolve(this.props.workspace.open(itemToAdd)) + Promise.resolve(this.props.workspace.open(itemToAdd, {activatePane: false})) .then(item => { this.dockItem = item; if (this.props.activate) { this.activate(); } From 3120e652f43675ac2c693da0da894e8c332e303d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 11:36:24 -0400 Subject: [PATCH 08/11] Prevent Pane activation if the Workspace is destroyed --- lib/views/dock-item.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/views/dock-item.js b/lib/views/dock-item.js index 6959c96a17..960720a74c 100644 --- a/lib/views/dock-item.js +++ b/lib/views/dock-item.js @@ -102,7 +102,9 @@ export default class DockItem extends React.Component { activate() { setTimeout(() => { - if (!this.dockItem || this.didCloseItem) { return; } + if (!this.dockItem || this.didCloseItem || this.props.workspace.isDestroyed()) { + return; + } const pane = this.props.workspace.paneForItem(this.dockItem); if (pane) { From 511b9ac74fc50ce2551c154fa1d010bfdfbdba80 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 14:04:17 -0400 Subject: [PATCH 09/11] Default useLegacyPanels correctly --- test/github-package.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/github-package.test.js b/test/github-package.test.js index 254aef9ff1..b8ccd66db2 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -410,6 +410,7 @@ describe('GithubPackage', function() { beforeEach(function() { // Necessary since we skip activate() githubPackage.savedState = {}; + githubPackage.useLegacyPanels = !workspace.getLeftDock; }); it('prefers the context of the active pane item', async function() { From 8e84ef5f423ddbb41b930e29674c1cb621634348 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 14:13:37 -0400 Subject: [PATCH 10/11] Make init and clone cancellable --- lib/models/repository-states/cloning.js | 6 +++++- lib/models/repository-states/initializing.js | 6 +++++- lib/models/repository-states/state.js | 16 ++++++++++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/lib/models/repository-states/cloning.js b/lib/models/repository-states/cloning.js index 8afe1c081a..368af07f45 100644 --- a/lib/models/repository-states/cloning.js +++ b/lib/models/repository-states/cloning.js @@ -13,7 +13,7 @@ export default class Cloning extends State { async start() { await mkdirs(this.workdir()); - await this.git().clone(this.remoteUrl, {recursive: true}); + await this.doClone(this.remoteUrl, {recursive: true}); await this.transitionTo('Loading'); } @@ -21,6 +21,10 @@ export default class Cloning extends State { showGitTabLoading() { return true; } + + directClone(remoteUrl, options) { + return this.git().clone(remoteUrl, options); + } } State.register(Cloning); diff --git a/lib/models/repository-states/initializing.js b/lib/models/repository-states/initializing.js index 385203d6ca..e96af6a2da 100644 --- a/lib/models/repository-states/initializing.js +++ b/lib/models/repository-states/initializing.js @@ -5,7 +5,7 @@ import State from './state'; */ export default class Initializing extends State { async start() { - await this.git().init(this.workdir()); + await this.doInit(this.workdir()); await this.transitionTo('Loading'); } @@ -13,6 +13,10 @@ export default class Initializing extends State { showGitTabLoading() { return true; } + + directInit(workdir) { + return this.git().init(workdir); + } } State.register(Initializing); diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index 53dfee839e..f4356e69dc 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -460,6 +460,14 @@ export default class State { return Promise.reject(new Error('Not a valid object name')); } + directInit() { + return Promise.resolve(); + } + + directClone(remoteUrl, options) { + return Promise.resolve(); + } + // Deferred operations // Direct raw git operations to the current state, even if the state has been changed. Use these methods within // start() methods. @@ -468,6 +476,14 @@ export default class State { return this.current().directIsGitRepository(); } + doInit(workdir) { + return this.current().directInit(); + } + + doClone(remoteUrl, options) { + return this.current().directClone(remoteUrl, options); + } + // Parse a DiscardHistory payload from the SHA recorded in config. async loadHistoryPayload() { const historySha = await this.current().directGetConfig('atomGithub.historySha'); From cc99c804e0580d360c5b091f9f90910bfd9a9b7c Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Fri, 21 Apr 2017 14:27:01 -0400 Subject: [PATCH 11/11] Typo fix --- lib/models/repository-states/state.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/models/repository-states/state.js b/lib/models/repository-states/state.js index f4356e69dc..9fb78f5b36 100644 --- a/lib/models/repository-states/state.js +++ b/lib/models/repository-states/state.js @@ -493,7 +493,7 @@ export default class State { let blob; try { - blob = await this.current().directBlobContents(historySha); + blob = await this.current().directGetBlobContents(historySha); } catch (e) { if (/Not a valid object name/.test(e.stdErr)) { return {};