From b32fc85d603e4698e8da32d20d6a43d0c34ddbb3 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 13:15:05 -0400 Subject: [PATCH 01/20] Rename "EventWatcher" to Switchboard --- lib/controllers/file-patch-controller.js | 6 +++--- lib/{event-watcher.js => switchboard.js} | 2 +- lib/views/file-patch-view.js | 6 +++--- test/controllers/file-patch-controller.test.js | 4 ++-- test/{event-watcher.test.js => switchboard.test.js} | 8 ++++---- 5 files changed, 13 insertions(+), 13 deletions(-) rename lib/{event-watcher.js => switchboard.js} (98%) rename test/{event-watcher.test.js => switchboard.test.js} (94%) diff --git a/lib/controllers/file-patch-controller.js b/lib/controllers/file-patch-controller.js index 5fe44fd3b1..b10cc78c79 100644 --- a/lib/controllers/file-patch-controller.js +++ b/lib/controllers/file-patch-controller.js @@ -3,7 +3,7 @@ import React from 'react'; import {Point, Emitter} from 'atom'; import {autobind} from 'core-decorators'; -import EventWatcher from '../event-watcher'; +import Switchboard from '../switchboard'; import FilePatchView from '../views/file-patch-view'; export default class FilePatchController extends React.Component { @@ -20,11 +20,11 @@ export default class FilePatchController extends React.Component { quietlySelectItem: React.PropTypes.func.isRequired, undoLastDiscard: React.PropTypes.func.isRequired, openFiles: React.PropTypes.func.isRequired, - eventWatcher: React.PropTypes.instanceOf(EventWatcher), + eventWatcher: React.PropTypes.instanceOf(Switchboard), } static defaultProps = { - eventWatcher: new EventWatcher(), + eventWatcher: new Switchboard(), } constructor(props, context) { diff --git a/lib/event-watcher.js b/lib/switchboard.js similarity index 98% rename from lib/event-watcher.js rename to lib/switchboard.js index 70d6979430..6edcfeced2 100644 --- a/lib/event-watcher.js +++ b/lib/switchboard.js @@ -2,7 +2,7 @@ * Construct Promises to wait for the next occurrence of specific events that occur throughout the data refresh * and rendering cycle. Resolve those promises when the corresponding events have been observed. */ -export default class EventWatcher { +export default class Switchboard { constructor() { this.promises = new Map(); } diff --git a/lib/views/file-patch-view.js b/lib/views/file-patch-view.js index b60d17ffd0..b8ba09027c 100644 --- a/lib/views/file-patch-view.js +++ b/lib/views/file-patch-view.js @@ -7,7 +7,7 @@ import {autobind} from 'core-decorators'; import HunkView from './hunk-view'; import Commands, {Command} from './commands'; import FilePatchSelection from './file-patch-selection'; -import EventWatcher from '../event-watcher'; +import Switchboard from '../switchboard'; export default class FilePatchView extends React.Component { static propTypes = { @@ -24,11 +24,11 @@ export default class FilePatchView extends React.Component { openCurrentFile: React.PropTypes.func.isRequired, didSurfaceFile: React.PropTypes.func.isRequired, didDiveIntoCorrespondingFilePatch: React.PropTypes.func.isRequired, - eventWatcher: React.PropTypes.instanceOf(EventWatcher), + eventWatcher: React.PropTypes.instanceOf(Switchboard), } static defaultProps = { - eventWatcher: new EventWatcher(), + eventWatcher: new Switchboard(), } constructor(props, context) { diff --git a/test/controllers/file-patch-controller.test.js b/test/controllers/file-patch-controller.test.js index f228a9ee1d..f64cf32817 100644 --- a/test/controllers/file-patch-controller.test.js +++ b/test/controllers/file-patch-controller.test.js @@ -9,7 +9,7 @@ import FilePatch from '../../lib/models/file-patch'; import FilePatchController from '../../lib/controllers/file-patch-controller'; import Hunk from '../../lib/models/hunk'; import HunkLine from '../../lib/models/hunk-line'; -import EventWatcher from '../../lib/event-watcher'; +import Switchboard from '../../lib/switchboard'; describe('FilePatchController', function() { let atomEnv, commandRegistry; @@ -20,7 +20,7 @@ describe('FilePatchController', function() { atomEnv = global.buildAtomEnvironment(); commandRegistry = atomEnv.commands; - eventWatcher = new EventWatcher(); + eventWatcher = new Switchboard(); discardLines = sinon.spy(); didSurfaceFile = sinon.spy(); diff --git a/test/event-watcher.test.js b/test/switchboard.test.js similarity index 94% rename from test/event-watcher.test.js rename to test/switchboard.test.js index fbd4a7fe16..7711ada458 100644 --- a/test/event-watcher.test.js +++ b/test/switchboard.test.js @@ -1,10 +1,10 @@ -import EventWatcher from '../lib/event-watcher'; +import Switchboard from '../lib/switchboard'; -describe('EventWatcher', function() { +describe('Switchboard', function() { let watcher; beforeEach(function() { - watcher = new EventWatcher(); + watcher = new Switchboard(); }); it('creates and resolves a Promise for an event', async function() { @@ -53,7 +53,7 @@ describe('EventWatcher', function() { }); describe('function pairs', function() { - const baseNames = Object.getOwnPropertyNames(EventWatcher.prototype) + const baseNames = Object.getOwnPropertyNames(Switchboard.prototype) .map(methodName => /^get(.+)Promise$/.exec(methodName)) .filter(match => match !== null) .map(match => match[1]); From 6bf2f49f91838c1dc72e10507c7a9a5c34a4f327 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 13:50:27 -0400 Subject: [PATCH 02/20] Rename resolve* methods --- lib/controllers/file-patch-controller.js | 12 +-- lib/switchboard.js | 91 ++++++++++--------- lib/views/file-patch-view.js | 2 +- .../controllers/file-patch-controller.test.js | 36 ++++---- test/switchboard.test.js | 88 ++++++++++-------- 5 files changed, 126 insertions(+), 103 deletions(-) diff --git a/lib/controllers/file-patch-controller.js b/lib/controllers/file-patch-controller.js index b10cc78c79..5f9c6d6dde 100644 --- a/lib/controllers/file-patch-controller.js +++ b/lib/controllers/file-patch-controller.js @@ -91,14 +91,14 @@ export default class FilePatchController extends React.Component { await this.props.repository.applyPatchToIndex( this.props.filePatch.getStagePatchForHunk(hunk), ); - this.props.eventWatcher.resolveStageOperationPromise(); + this.props.eventWatcher.didFinishStageOperation(); } async unstageHunk(hunk) { await this.props.repository.applyPatchToIndex( this.props.filePatch.getUnstagePatchForHunk(hunk), ); - this.props.eventWatcher.resolveStageOperationPromise(); + this.props.eventWatcher.didFinishStageOperation(); } stageOrUnstageHunk(hunk) { @@ -118,7 +118,7 @@ export default class FilePatchController extends React.Component { } this.stagingOperationInProgress = true; - this.props.eventWatcher.getPatchChangedPromise().then(() => { + this.props.eventWatcher.getChangePatchPromise().then(() => { this.stagingOperationInProgress = false; }); @@ -130,7 +130,7 @@ export default class FilePatchController extends React.Component { this.props.filePatch.getStagePatchForLines(lines), ); - this.props.eventWatcher.resolveStageOperationPromise(); + this.props.eventWatcher.didFinishStageOperation(); } async unstageLines(lines) { @@ -138,7 +138,7 @@ export default class FilePatchController extends React.Component { this.props.filePatch.getUnstagePatchForLines(lines), ); - this.props.eventWatcher.resolveStageOperationPromise(); + this.props.eventWatcher.didFinishStageOperation(); } stageOrUnstageLines(lines) { @@ -158,7 +158,7 @@ export default class FilePatchController extends React.Component { } this.stagingOperationInProgress = true; - this.props.eventWatcher.getPatchChangedPromise().then(() => { + this.props.eventWatcher.getChangePatchPromise().then(() => { this.stagingOperationInProgress = false; }); diff --git a/lib/switchboard.js b/lib/switchboard.js index 6edcfeced2..33b883df32 100644 --- a/lib/switchboard.js +++ b/lib/switchboard.js @@ -1,87 +1,96 @@ +import {Emitter} from 'atom'; // FIXME import from event-kit instead + /* - * Construct Promises to wait for the next occurrence of specific events that occur throughout the data refresh - * and rendering cycle. Resolve those promises when the corresponding events have been observed. + * Register callbacks and construct Promises to wait for the next occurrence of specific events that occur throughout + * the data refresh and rendering cycle. */ export default class Switchboard { constructor() { this.promises = new Map(); + this.emitter = new Emitter(); + } + + /* + * Invoke a callback each time that a desired event is observed. Return a Disposable that can be used to + * unsubscribe from events. + * + * In general, you should use the more specific `onDidXyz` methods. + */ + onDid(eventName, callback) { + return this.emitter.on(`did-${eventName}`, callback); + } + + /* + * Indicate that a named event has been observed, firing any callbacks and resolving any Promises that were created + * for this event. Optionally provide a payload with more information. + * + * In general, you should prefer the more specific `didXyz()` methods. + */ + did(eventName, payload) { + this.emitter.emit(`did-${eventName}`, payload); } /* * Retrieve a Promise that will be resolved the next time a desired event is observed. * - * In general, you should prefer the more specific `getXyzPromise()` methods instead to avoid a proliferation of - * "magic strings." + * In general, you should prefer the more specific `getXyzPromise()` methods. */ getPromise(eventName) { const existing = this.promises.get(eventName); if (existing !== undefined) { - return existing.promise; + return existing; } - let resolver, rejecter; const created = new Promise((resolve, reject) => { - resolver = resolve; - rejecter = reject; - }); - this.promises.set(eventName, { - promise: created, - resolver, - rejecter, + const subscription = this.onDid(eventName, payload => { + subscription.dispose(); + this.promises.delete(eventName); + resolve(payload); + }); }); + this.promises.set(eventName, created); return created; } /* - * Indicate that a named event has been observed, resolving any Promises that were created for this event. Optionally - * provide a payload. - * - * In general, you should prefer the more specific `resolveXyzPromise()` methods. + * Invoke callback each time a hunk or line staging or unstaging operation has completed in git. */ - resolvePromise(eventName, payload) { - const existing = this.promises.get(eventName); - if (existing !== undefined) { - this.promises.delete(eventName); - existing.resolver(payload); - } + onDidFinishStageOperation(callback) { + return this.onDid('finish-stage-operation', callback); } /* - * Indicate that a named event has had some kind of terrible problem. + * Notified when a hunk or line stage or unstage operation has completed. */ - rejectPromise(eventName, error) { - const existing = this.promises.get(eventName); - if (existing !== undefined) { - this.promises.delete(eventName); - existing.rejecter(error); - } + getFinishStageOperationPromise() { + return this.getPromise('finish-stage-operation'); } /* - * Notified when a hunk or line stage or unstage operation has completed. + * A hunk or line stage or unstage operation has completed. */ - getStageOperationPromise() { - return this.getPromise('stage-operation'); + didFinishStageOperation(payload) { + this.did('finish-stage-operation', payload); } /* - * Notified when an open FilePatchView's hunks have changed. + * Invoke callback each time a FilePatchView's hunks change. */ - getPatchChangedPromise() { - return this.getPromise('patch-changed'); + onDidChangePatch(callback) { + return this.onDid('change-patch', callback); } /* - * A hunk or line stage or unstage operation has completed. + * Notified when an open FilePatchView's hunks have changed. */ - resolveStageOperationPromise(payload) { - this.resolvePromise('stage-operation', payload); + getChangePatchPromise() { + return this.getPromise('change-patch'); } /* * An open FilePatchView's hunks have changed. */ - resolvePatchChangedPromise(payload) { - this.resolvePromise('patch-changed', payload); + didChangePatch(payload) { + this.did('change-patch', payload); } } diff --git a/lib/views/file-patch-view.js b/lib/views/file-patch-view.js index b8ba09027c..0cd00c2668 100644 --- a/lib/views/file-patch-view.js +++ b/lib/views/file-patch-view.js @@ -57,7 +57,7 @@ export default class FilePatchView extends React.Component { selection: prevState.selection.updateHunks(nextProps.hunks), }; }, () => { - nextProps.eventWatcher.resolvePatchChangedPromise(); + nextProps.eventWatcher.didChangePatch(); }); } } diff --git a/test/controllers/file-patch-controller.test.js b/test/controllers/file-patch-controller.test.js index f64cf32817..47797dc94c 100644 --- a/test/controllers/file-patch-controller.test.js +++ b/test/controllers/file-patch-controller.test.js @@ -169,7 +169,7 @@ describe('FilePatchController', function() { const hunkView0 = wrapper.find('HunkView').at(0); assert.isFalse(hunkView0.prop('isSelected')); - const opPromise0 = eventWatcher.getStageOperationPromise(); + const opPromise0 = eventWatcher.getFinishStageOperationPromise(); hunkView0.find('button.github-HunkView-stageButton').simulate('click'); await opPromise0; @@ -181,7 +181,7 @@ describe('FilePatchController', function() { ); assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedStagedLines.join('\n')); - const updatePromise0 = eventWatcher.getPatchChangedPromise(); + const updatePromise0 = eventWatcher.getChangePatchPromise(); const stagedFilePatch = await repository.getFilePatchForPath('sample.js', {staged: true}); wrapper.setProps({ filePatch: stagedFilePatch, @@ -190,7 +190,7 @@ describe('FilePatchController', function() { await updatePromise0; const hunkView1 = wrapper.find('HunkView').at(0); - const opPromise1 = eventWatcher.getStageOperationPromise(); + const opPromise1 = eventWatcher.getFinishStageOperationPromise(); hunkView1.find('button.github-HunkView-stageButton').simulate('click'); await opPromise1; @@ -221,7 +221,7 @@ describe('FilePatchController', function() { repository, })); - const opPromise0 = eventWatcher.getStageOperationPromise(); + const opPromise0 = eventWatcher.getFinishStageOperationPromise(); const hunkView0 = wrapper.find('HunkView').at(0); hunkView0.find('LineView').at(1).simulate('mousedown', {button: 0, detail: 1}); hunkView0.find('LineView').at(3).simulate('mousemove', {}); @@ -238,12 +238,12 @@ describe('FilePatchController', function() { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines0.join('\n')); // stage remaining lines in hunk - const updatePromise1 = eventWatcher.getPatchChangedPromise(); + const updatePromise1 = eventWatcher.getChangePatchPromise(); const unstagedFilePatch1 = await repository.getFilePatchForPath('sample.js'); wrapper.setProps({filePatch: unstagedFilePatch1}); await updatePromise1; - const opPromise1 = eventWatcher.getStageOperationPromise(); + const opPromise1 = eventWatcher.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('button.github-HunkView-stageButton').simulate('click'); await opPromise1; @@ -257,7 +257,7 @@ describe('FilePatchController', function() { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines1.join('\n')); // unstage a subset of lines from the first hunk - const updatePromise2 = eventWatcher.getPatchChangedPromise(); + const updatePromise2 = eventWatcher.getChangePatchPromise(); const stagedFilePatch2 = await repository.getFilePatchForPath('sample.js', {staged: true}); wrapper.setProps({ filePatch: stagedFilePatch2, @@ -271,7 +271,7 @@ describe('FilePatchController', function() { hunkView2.find('LineView').at(2).simulate('mousedown', {button: 0, detail: 1, metaKey: true}); window.dispatchEvent(new MouseEvent('mouseup')); - const opPromise2 = eventWatcher.getStageOperationPromise(); + const opPromise2 = eventWatcher.getFinishStageOperationPromise(); hunkView2.find('button.github-HunkView-stageButton').simulate('click'); await opPromise2; @@ -284,7 +284,7 @@ describe('FilePatchController', function() { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines2.join('\n')); // unstage the rest of the hunk - const updatePromise3 = eventWatcher.getPatchChangedPromise(); + const updatePromise3 = eventWatcher.getChangePatchPromise(); const stagedFilePatch3 = await repository.getFilePatchForPath('sample.js', {staged: true}); wrapper.setProps({ filePatch: stagedFilePatch3, @@ -293,7 +293,7 @@ describe('FilePatchController', function() { commandRegistry.dispatch(wrapper.find('FilePatchView').getDOMNode(), 'github:toggle-patch-selection-mode'); - const opPromise3 = eventWatcher.getStageOperationPromise(); + const opPromise3 = eventWatcher.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('button.github-HunkView-stageButton').simulate('click'); await opPromise3; @@ -317,7 +317,7 @@ describe('FilePatchController', function() { repository, })); - const opPromise = eventWatcher.getStageOperationPromise(); + const opPromise = eventWatcher.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('button.github-HunkView-stageButton').simulate('click'); await opPromise; @@ -344,7 +344,7 @@ describe('FilePatchController', function() { commandRegistry.dispatch(viewNode, 'github:toggle-patch-selection-mode'); commandRegistry.dispatch(viewNode, 'core:select-all'); - const opPromise = eventWatcher.getStageOperationPromise(); + const opPromise = eventWatcher.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('button.github-HunkView-stageButton').simulate('click'); await opPromise; @@ -384,7 +384,7 @@ describe('FilePatchController', function() { // stage lines in rapid succession // second stage action is a no-op since the first staging operation is in flight - const line1StagingPromise = eventWatcher.getStageOperationPromise(); + const line1StagingPromise = eventWatcher.getFinishStageOperationPromise(); hunkView0.find('.github-HunkView-stageButton').simulate('click'); hunkView0.find('.github-HunkView-stageButton').simulate('click'); await line1StagingPromise; @@ -399,7 +399,7 @@ describe('FilePatchController', function() { let actualLines = await repository.readFileFromIndex('sample.js'); assert.autocrlfEqual(actualLines, expectedLines.join('\n')); - const line1PatchPromise = eventWatcher.getPatchChangedPromise(); + const line1PatchPromise = eventWatcher.getChangePatchPromise(); wrapper.setProps({filePatch: modifiedFilePatch}); await line1PatchPromise; @@ -407,7 +407,7 @@ describe('FilePatchController', function() { hunkView1.find('LineView').at(2).simulate('mousedown', {button: 0, detail: 1}); window.dispatchEvent(new MouseEvent('mouseup')); - const line2StagingPromise = eventWatcher.getStageOperationPromise(); + const line2StagingPromise = eventWatcher.getFinishStageOperationPromise(); hunkView1.find('.github-HunkView-stageButton').simulate('click'); await line2StagingPromise; @@ -446,12 +446,12 @@ describe('FilePatchController', function() { // ensure staging the same hunk twice does not cause issues // second stage action is a no-op since the first staging operation is in flight - const hunk1StagingPromise = eventWatcher.getStageOperationPromise(); + const hunk1StagingPromise = eventWatcher.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('.github-HunkView-stageButton').simulate('click'); wrapper.find('HunkView').at(0).find('.github-HunkView-stageButton').simulate('click'); await hunk1StagingPromise; - const patchPromise0 = eventWatcher.getPatchChangedPromise(); + const patchPromise0 = eventWatcher.getChangePatchPromise(); repository.refresh(); // clear the cached file patches const modifiedFilePatch = await repository.getFilePatchForPath('sample.js'); wrapper.setProps({filePatch: modifiedFilePatch}); @@ -466,7 +466,7 @@ describe('FilePatchController', function() { let actualLines = await repository.readFileFromIndex('sample.js'); assert.autocrlfEqual(actualLines, expectedLines.join('\n')); - const hunk2StagingPromise = eventWatcher.getStageOperationPromise(); + const hunk2StagingPromise = eventWatcher.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('.github-HunkView-stageButton').simulate('click'); await hunk2StagingPromise; diff --git a/test/switchboard.test.js b/test/switchboard.test.js index 7711ada458..4e8bc77138 100644 --- a/test/switchboard.test.js +++ b/test/switchboard.test.js @@ -1,60 +1,73 @@ import Switchboard from '../lib/switchboard'; -describe('Switchboard', function() { - let watcher; +describe.only('Switchboard', function() { + let switchboard; beforeEach(function() { - watcher = new Switchboard(); + switchboard = new Switchboard(); }); - it('creates and resolves a Promise for an event', async function() { - const promise = watcher.getPromise('testing'); + describe('events', function() { + let sub; - const payload = {}; - watcher.resolvePromise('testing', payload); + afterEach(function() { + sub && sub.dispose(); + }); + + it('synchronously broadcasts events', function() { + let observed = 0; + sub = switchboard.onDid('test', () => observed++); - const result = await promise; - assert.strictEqual(result, payload); + assert.equal(observed, 0); + switchboard.did('test'); + assert.equal(observed, 1); + }); }); - it('supports multiple consumers of the same Promise', async function() { - const promise0 = watcher.getPromise('testing'); - const promise1 = watcher.getPromise('testing'); - assert.strictEqual(promise0, promise1); + describe('promises', function() { + it('creates and resolves a Promise for an event', async function() { + const promise = switchboard.getPromise('testing'); - const payload = {}; - watcher.resolvePromise('testing', payload); + const payload = {}; + switchboard.did('testing', payload); - assert.strictEqual(await promise0, payload); - assert.strictEqual(await promise1, payload); - }); + const result = await promise; + assert.strictEqual(result, payload); + }); - it('creates new Promises for repeated events', async function() { - const promise0 = watcher.getPromise('testing'); + it('supports multiple consumers of the same Promise', async function() { + const promise0 = switchboard.getPromise('testing'); + const promise1 = switchboard.getPromise('testing'); + assert.strictEqual(promise0, promise1); - watcher.resolvePromise('testing', 0); - assert.equal(await promise0, 0); + const payload = {}; + switchboard.did('testing', payload); - const promise1 = watcher.getPromise('testing'); + assert.strictEqual(await promise0, payload); + assert.strictEqual(await promise1, payload); + }); - watcher.resolvePromise('testing', 1); - assert.equal(await promise1, 1); - }); + it('creates new Promises for repeated events', async function() { + const promise0 = switchboard.getPromise('testing'); - it('"resolves" an event that has no Promise', function() { - watcher.resolvePromise('anybody-there', {}); - }); + switchboard.did('testing', 0); + assert.equal(await promise0, 0); - it('rejects a Promise with an error', async function() { - const promise = watcher.getPromise('testing'); + const promise1 = switchboard.getPromise('testing'); - watcher.rejectPromise('testing', new Error('oh shit')); - await assert.isRejected(promise, /oh shit/); + switchboard.did('testing', 1); + assert.equal(await promise1, 1); + }); + + it('"resolves" an event that has no Promise', function() { + switchboard.did('anybody-there', {}); + }); }); - describe('function pairs', function() { + // Ensure that all of the `didXyz`, `onDidXyz`, and `getXyzPromise` method triplets are aligned correctly. + describe('function triplets', function() { const baseNames = Object.getOwnPropertyNames(Switchboard.prototype) - .map(methodName => /^get(.+)Promise$/.exec(methodName)) + .map(methodName => /^did(.+)$/.exec(methodName)) .filter(match => match !== null) .map(match => match[1]); let functionPairs; @@ -63,8 +76,9 @@ describe('Switchboard', function() { functionPairs = baseNames.map(baseName => { return { baseName, - getter: watcher[`get${baseName}Promise`].bind(watcher), - resolver: watcher[`resolve${baseName}Promise`].bind(watcher), + subscriber: switchboard[`onDid${baseName}`].bind(switchboard), + getter: switchboard[`get${baseName}Promise`].bind(switchboard), + resolver: switchboard[`did${baseName}`].bind(switchboard), }; }); }); From c97b0da23ff9cb4a761e58e8db7382d3f5bb37a3 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 13:57:34 -0400 Subject: [PATCH 03/20] Test subscription method triples too --- test/switchboard.test.js | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/test/switchboard.test.js b/test/switchboard.test.js index 4e8bc77138..b2450d8088 100644 --- a/test/switchboard.test.js +++ b/test/switchboard.test.js @@ -1,3 +1,4 @@ +import {CompositeDisposable} from 'atom'; // FIXME import from event-kit import Switchboard from '../lib/switchboard'; describe.only('Switchboard', function() { @@ -70,10 +71,10 @@ describe.only('Switchboard', function() { .map(methodName => /^did(.+)$/.exec(methodName)) .filter(match => match !== null) .map(match => match[1]); - let functionPairs; + let functionTriples; beforeEach(function() { - functionPairs = baseNames.map(baseName => { + functionTriples = baseNames.map(baseName => { return { baseName, subscriber: switchboard[`onDid${baseName}`].bind(switchboard), @@ -92,23 +93,31 @@ describe.only('Switchboard', function() { let positiveResolver = null; const negativeResolvers = []; - for (let i = 0; i < functionPairs.length; i++) { - const functionPair = functionPairs[i]; + const subscriptions = new CompositeDisposable(); - if (functionPair.baseName === baseName) { - const positivePromise = functionPair.getter().then(payload => { + for (let i = 0; i < functionTriples.length; i++) { + const functionTriple = functionTriples[i]; + + if (functionTriple.baseName === baseName) { + const positivePromise = functionTriple.getter().then(payload => { positiveResults.push(payload); }); allPromises.push(positivePromise); - positiveResolver = functionPair.resolver; + positiveResolver = functionTriple.resolver; + + const positiveSubscription = functionTriple.subscriber(payload => positiveResults.push(payload)); + subscriptions.add(positiveSubscription); } else { - const negativePromise = functionPair.getter().then(payload => { + const negativePromise = functionTriple.getter().then(payload => { negativeResults.push(payload); }); allPromises.push(negativePromise); - negativeResolvers.push(functionPair.resolver); + negativeResolvers.push(functionTriple.resolver); + + const negativeSubscription = functionTriple.subscriber(payload => negativeResults.push(payload)); + subscriptions.add(negativeSubscription); } } @@ -118,10 +127,12 @@ describe.only('Switchboard', function() { await Promise.all(allPromises); - assert.lengthOf(positiveResults, 1); + subscriptions.dispose(); + + assert.lengthOf(positiveResults, 2); assert.isTrue(positiveResults.every(result => result === 'yes')); - assert.lengthOf(negativeResults, baseNames.length - 1); + assert.lengthOf(negativeResults, (baseNames.length - 1) * 2); assert.isTrue(negativeResults.every(result => result === 'no')); }); }); From 6a6838691a437fe90c95a66523d346b7173c694e Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 14:04:56 -0400 Subject: [PATCH 04/20] :candle: :skull: _metaprogramming_ :skull: :candle: --- lib/switchboard.js | 55 +++++++++++++--------------------------------- 1 file changed, 15 insertions(+), 40 deletions(-) diff --git a/lib/switchboard.js b/lib/switchboard.js index 33b883df32..96868e4f53 100644 --- a/lib/switchboard.js +++ b/lib/switchboard.js @@ -51,46 +51,21 @@ export default class Switchboard { this.promises.set(eventName, created); return created; } +} - /* - * Invoke callback each time a hunk or line staging or unstaging operation has completed in git. - */ - onDidFinishStageOperation(callback) { - return this.onDid('finish-stage-operation', callback); - } - - /* - * Notified when a hunk or line stage or unstage operation has completed. - */ - getFinishStageOperationPromise() { - return this.getPromise('finish-stage-operation'); - } - - /* - * A hunk or line stage or unstage operation has completed. - */ - didFinishStageOperation(payload) { - this.did('finish-stage-operation', payload); - } - - /* - * Invoke callback each time a FilePatchView's hunks change. - */ - onDidChangePatch(callback) { - return this.onDid('change-patch', callback); - } +[ + 'FinishStageOperation', + 'ChangePatch', +].forEach(eventName => { + Switchboard.prototype[`did${eventName}`] = function(payload) { + this.did(eventName, payload); + }; - /* - * Notified when an open FilePatchView's hunks have changed. - */ - getChangePatchPromise() { - return this.getPromise('change-patch'); - } + Switchboard.prototype[`get${eventName}Promise`] = function() { + return this.getPromise(eventName); + }; - /* - * An open FilePatchView's hunks have changed. - */ - didChangePatch(payload) { - this.did('change-patch', payload); - } -} + Switchboard.prototype[`onDid${eventName}`] = function(callback) { + return this.onDid(eventName, callback); + }; +}); From acf60dece2c4f908612321a8e0cf8764712c3c86 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 14:20:46 -0400 Subject: [PATCH 05/20] :fire: `.only(` --- test/switchboard.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/switchboard.test.js b/test/switchboard.test.js index b2450d8088..1403a6634e 100644 --- a/test/switchboard.test.js +++ b/test/switchboard.test.js @@ -1,7 +1,7 @@ import {CompositeDisposable} from 'atom'; // FIXME import from event-kit import Switchboard from '../lib/switchboard'; -describe.only('Switchboard', function() { +describe('Switchboard', function() { let switchboard; beforeEach(function() { From 371404d7ca1d7e5f47adf0ba14887b06dd8a4c4b Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 15:02:08 -0400 Subject: [PATCH 06/20] Measure successive durations from a starting point --- lib/yardstick.js | 59 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 lib/yardstick.js diff --git a/lib/yardstick.js b/lib/yardstick.js new file mode 100644 index 0000000000..9709669e0c --- /dev/null +++ b/lib/yardstick.js @@ -0,0 +1,59 @@ +// Measure elapsed durations from specific beginning points. + +import {writeFile} from './helpers'; + +// A sequence of durations since a fixed +class DurationSet { + constructor(name) { + this.name = name; + this.startTimestamp = performance.now(); + this.marks = []; + + // eslint-disable-next-line no-console + console.log('%cbegin %c%s:%s', + 'font-weight: bold', + 'font-weight: normal; font-style: italic; color: blue', this.name); + } + + mark(eventName) { + const duration = performance.now() - this.startTimestamp; + + // eslint-disable-next-line no-console + console.log('%cmark %c%s:%s %c%dms', + 'font-weight: bold', + 'font-weight: normal; font-style: italic; color: blue', this.name, eventName, + 'font-style: normal; color: black', duration); + + this.marks.push({eventName, duration}); + } + + serialize() { + return { + name: this.name, + markers: this.marks, + }; + } +} + +let durationSets = []; + +const yardstick = { + async saveTo(fileName) { + const payload = JSON.stringify(durationSets.map(set => set.serialize())); + await writeFile(fileName, payload); + durationSets = []; + }, +}; + +[ + 'stageFile', 'stageHunk', 'stageLine', + 'unstageFile', 'unstageHunk', 'unstageLine', +].forEach(opName => { + yardstick[opName] = function() { + const ds = new DurationSet(opName); + durationSets.push(ds); + return ds; + }; +}); + +export default yardstick; From a07b23103529ea3f10612d78d5b8c6a00f97faa0 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 16:06:08 -0400 Subject: [PATCH 07/20] Rename some Switchboard variables and add a few events --- lib/controllers/file-patch-controller.js | 27 ++++++++---- lib/decorators/observe-model.js | 9 +++- lib/switchboard.js | 3 ++ lib/views/file-patch-view.js | 6 +-- .../controllers/file-patch-controller.test.js | 42 +++++++++---------- 5 files changed, 53 insertions(+), 34 deletions(-) diff --git a/lib/controllers/file-patch-controller.js b/lib/controllers/file-patch-controller.js index 5f9c6d6dde..5df5933ad6 100644 --- a/lib/controllers/file-patch-controller.js +++ b/lib/controllers/file-patch-controller.js @@ -20,11 +20,11 @@ export default class FilePatchController extends React.Component { quietlySelectItem: React.PropTypes.func.isRequired, undoLastDiscard: React.PropTypes.func.isRequired, openFiles: React.PropTypes.func.isRequired, - eventWatcher: React.PropTypes.instanceOf(Switchboard), + switchboard: React.PropTypes.instanceOf(Switchboard), } static defaultProps = { - eventWatcher: new Switchboard(), + switchboard: new Switchboard(), } constructor(props, context) { @@ -68,7 +68,7 @@ export default class FilePatchController extends React.Component { attemptHunkStageOperation={this.attemptHunkStageOperation} didSurfaceFile={this.didSurfaceFile} didDiveIntoCorrespondingFilePatch={this.diveIntoCorrespondingFilePatch} - eventWatcher={this.props.eventWatcher} + switchboard={this.props.switchboard} openCurrentFile={this.openCurrentFile} discardLines={this.props.discardLines} undoLastDiscard={this.undoLastDiscard} @@ -88,17 +88,22 @@ export default class FilePatchController extends React.Component { } async stageHunk(hunk) { + this.props.switchboard.didBeginStageOperation({stage: true, hunk: true}); + await this.props.repository.applyPatchToIndex( this.props.filePatch.getStagePatchForHunk(hunk), ); - this.props.eventWatcher.didFinishStageOperation(); + this.props.switchboard.didFinishStageOperation({stage: true, hunk: true}); } async unstageHunk(hunk) { + this.props.switchboard.didBeginStageOperation({unstage: true, hunk: true}); + await this.props.repository.applyPatchToIndex( this.props.filePatch.getUnstagePatchForHunk(hunk), ); - this.props.eventWatcher.didFinishStageOperation(); + + this.props.switchboard.didFinishStageOperation({unstage: true, hunk: true}); } stageOrUnstageHunk(hunk) { @@ -118,7 +123,7 @@ export default class FilePatchController extends React.Component { } this.stagingOperationInProgress = true; - this.props.eventWatcher.getChangePatchPromise().then(() => { + this.props.switchboard.getChangePatchPromise().then(() => { this.stagingOperationInProgress = false; }); @@ -126,19 +131,23 @@ export default class FilePatchController extends React.Component { } async stageLines(lines) { + this.props.switchboard.didBeginStageOperation({stage: true, line: true}); + await this.props.repository.applyPatchToIndex( this.props.filePatch.getStagePatchForLines(lines), ); - this.props.eventWatcher.didFinishStageOperation(); + this.props.switchboard.didFinishStageOperation({stage: true, line: true}); } async unstageLines(lines) { + this.props.switchboard.didBeginStageOperation({unstage: true, line: true}); + await this.props.repository.applyPatchToIndex( this.props.filePatch.getUnstagePatchForLines(lines), ); - this.props.eventWatcher.didFinishStageOperation(); + this.props.switchboard.didFinishStageOperation({unstage: true, line: true}); } stageOrUnstageLines(lines) { @@ -158,7 +167,7 @@ export default class FilePatchController extends React.Component { } this.stagingOperationInProgress = true; - this.props.eventWatcher.getChangePatchPromise().then(() => { + this.props.switchboard.getChangePatchPromise().then(() => { this.stagingOperationInProgress = false; }); diff --git a/lib/decorators/observe-model.js b/lib/decorators/observe-model.js index ebc6c6bcec..af2f9f9832 100644 --- a/lib/decorators/observe-model.js +++ b/lib/decorators/observe-model.js @@ -45,7 +45,14 @@ export default function ObserveModelDecorator(spec) { fetchData: model => fetchData(model, this.props), didUpdate: () => { if (this.mounted) { - this.setState({modelData: this.modelObserver.getActiveModelData()}, this.resolve); + this.setState({modelData: this.modelObserver.getActiveModelData()}, () => { + /* eslint-disable react/prop-types */ + if (this.props.switchboard) { + this.props.switchboard.didFinishRender(); + } + /* eslint-enable react/prop-types */ + this.resolve(); + }); } }, }); diff --git a/lib/switchboard.js b/lib/switchboard.js index 96868e4f53..50c4e2d27d 100644 --- a/lib/switchboard.js +++ b/lib/switchboard.js @@ -54,8 +54,11 @@ export default class Switchboard { } [ + 'UpdateRepository', + 'BeginStageOperation', 'FinishStageOperation', 'ChangePatch', + 'FinishRender', ].forEach(eventName => { Switchboard.prototype[`did${eventName}`] = function(payload) { this.did(eventName, payload); diff --git a/lib/views/file-patch-view.js b/lib/views/file-patch-view.js index 0cd00c2668..5f69f596fc 100644 --- a/lib/views/file-patch-view.js +++ b/lib/views/file-patch-view.js @@ -24,11 +24,11 @@ export default class FilePatchView extends React.Component { openCurrentFile: React.PropTypes.func.isRequired, didSurfaceFile: React.PropTypes.func.isRequired, didDiveIntoCorrespondingFilePatch: React.PropTypes.func.isRequired, - eventWatcher: React.PropTypes.instanceOf(Switchboard), + switchboard: React.PropTypes.instanceOf(Switchboard), } static defaultProps = { - eventWatcher: new Switchboard(), + switchboard: new Switchboard(), } constructor(props, context) { @@ -57,7 +57,7 @@ export default class FilePatchView extends React.Component { selection: prevState.selection.updateHunks(nextProps.hunks), }; }, () => { - nextProps.eventWatcher.didChangePatch(); + nextProps.switchboard.didChangePatch(); }); } } diff --git a/test/controllers/file-patch-controller.test.js b/test/controllers/file-patch-controller.test.js index 47797dc94c..96012fcbef 100644 --- a/test/controllers/file-patch-controller.test.js +++ b/test/controllers/file-patch-controller.test.js @@ -13,14 +13,14 @@ import Switchboard from '../../lib/switchboard'; describe('FilePatchController', function() { let atomEnv, commandRegistry; - let component, eventWatcher; + let component, switchboard; let discardLines, didSurfaceFile, didDiveIntoFilePath, quietlySelectItem, undoLastDiscard, openFiles; beforeEach(function() { atomEnv = global.buildAtomEnvironment(); commandRegistry = atomEnv.commands; - eventWatcher = new Switchboard(); + switchboard = new Switchboard(); discardLines = sinon.spy(); didSurfaceFile = sinon.spy(); @@ -38,7 +38,7 @@ describe('FilePatchController', function() { stagingStatus="unstaged" isPartiallyStaged={false} isAmending={false} - eventWatcher={eventWatcher} + switchboard={switchboard} discardLines={discardLines} didSurfaceFile={didSurfaceFile} didDiveIntoFilePath={didDiveIntoFilePath} @@ -169,7 +169,7 @@ describe('FilePatchController', function() { const hunkView0 = wrapper.find('HunkView').at(0); assert.isFalse(hunkView0.prop('isSelected')); - const opPromise0 = eventWatcher.getFinishStageOperationPromise(); + const opPromise0 = switchboard.getFinishStageOperationPromise(); hunkView0.find('button.github-HunkView-stageButton').simulate('click'); await opPromise0; @@ -181,7 +181,7 @@ describe('FilePatchController', function() { ); assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedStagedLines.join('\n')); - const updatePromise0 = eventWatcher.getChangePatchPromise(); + const updatePromise0 = switchboard.getChangePatchPromise(); const stagedFilePatch = await repository.getFilePatchForPath('sample.js', {staged: true}); wrapper.setProps({ filePatch: stagedFilePatch, @@ -190,7 +190,7 @@ describe('FilePatchController', function() { await updatePromise0; const hunkView1 = wrapper.find('HunkView').at(0); - const opPromise1 = eventWatcher.getFinishStageOperationPromise(); + const opPromise1 = switchboard.getFinishStageOperationPromise(); hunkView1.find('button.github-HunkView-stageButton').simulate('click'); await opPromise1; @@ -221,7 +221,7 @@ describe('FilePatchController', function() { repository, })); - const opPromise0 = eventWatcher.getFinishStageOperationPromise(); + const opPromise0 = switchboard.getFinishStageOperationPromise(); const hunkView0 = wrapper.find('HunkView').at(0); hunkView0.find('LineView').at(1).simulate('mousedown', {button: 0, detail: 1}); hunkView0.find('LineView').at(3).simulate('mousemove', {}); @@ -238,12 +238,12 @@ describe('FilePatchController', function() { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines0.join('\n')); // stage remaining lines in hunk - const updatePromise1 = eventWatcher.getChangePatchPromise(); + const updatePromise1 = switchboard.getChangePatchPromise(); const unstagedFilePatch1 = await repository.getFilePatchForPath('sample.js'); wrapper.setProps({filePatch: unstagedFilePatch1}); await updatePromise1; - const opPromise1 = eventWatcher.getFinishStageOperationPromise(); + const opPromise1 = switchboard.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('button.github-HunkView-stageButton').simulate('click'); await opPromise1; @@ -257,7 +257,7 @@ describe('FilePatchController', function() { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines1.join('\n')); // unstage a subset of lines from the first hunk - const updatePromise2 = eventWatcher.getChangePatchPromise(); + const updatePromise2 = switchboard.getChangePatchPromise(); const stagedFilePatch2 = await repository.getFilePatchForPath('sample.js', {staged: true}); wrapper.setProps({ filePatch: stagedFilePatch2, @@ -271,7 +271,7 @@ describe('FilePatchController', function() { hunkView2.find('LineView').at(2).simulate('mousedown', {button: 0, detail: 1, metaKey: true}); window.dispatchEvent(new MouseEvent('mouseup')); - const opPromise2 = eventWatcher.getFinishStageOperationPromise(); + const opPromise2 = switchboard.getFinishStageOperationPromise(); hunkView2.find('button.github-HunkView-stageButton').simulate('click'); await opPromise2; @@ -284,7 +284,7 @@ describe('FilePatchController', function() { assert.autocrlfEqual(await repository.readFileFromIndex('sample.js'), expectedLines2.join('\n')); // unstage the rest of the hunk - const updatePromise3 = eventWatcher.getChangePatchPromise(); + const updatePromise3 = switchboard.getChangePatchPromise(); const stagedFilePatch3 = await repository.getFilePatchForPath('sample.js', {staged: true}); wrapper.setProps({ filePatch: stagedFilePatch3, @@ -293,7 +293,7 @@ describe('FilePatchController', function() { commandRegistry.dispatch(wrapper.find('FilePatchView').getDOMNode(), 'github:toggle-patch-selection-mode'); - const opPromise3 = eventWatcher.getFinishStageOperationPromise(); + const opPromise3 = switchboard.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('button.github-HunkView-stageButton').simulate('click'); await opPromise3; @@ -317,7 +317,7 @@ describe('FilePatchController', function() { repository, })); - const opPromise = eventWatcher.getFinishStageOperationPromise(); + const opPromise = switchboard.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('button.github-HunkView-stageButton').simulate('click'); await opPromise; @@ -344,7 +344,7 @@ describe('FilePatchController', function() { commandRegistry.dispatch(viewNode, 'github:toggle-patch-selection-mode'); commandRegistry.dispatch(viewNode, 'core:select-all'); - const opPromise = eventWatcher.getFinishStageOperationPromise(); + const opPromise = switchboard.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('button.github-HunkView-stageButton').simulate('click'); await opPromise; @@ -384,7 +384,7 @@ describe('FilePatchController', function() { // stage lines in rapid succession // second stage action is a no-op since the first staging operation is in flight - const line1StagingPromise = eventWatcher.getFinishStageOperationPromise(); + const line1StagingPromise = switchboard.getFinishStageOperationPromise(); hunkView0.find('.github-HunkView-stageButton').simulate('click'); hunkView0.find('.github-HunkView-stageButton').simulate('click'); await line1StagingPromise; @@ -399,7 +399,7 @@ describe('FilePatchController', function() { let actualLines = await repository.readFileFromIndex('sample.js'); assert.autocrlfEqual(actualLines, expectedLines.join('\n')); - const line1PatchPromise = eventWatcher.getChangePatchPromise(); + const line1PatchPromise = switchboard.getChangePatchPromise(); wrapper.setProps({filePatch: modifiedFilePatch}); await line1PatchPromise; @@ -407,7 +407,7 @@ describe('FilePatchController', function() { hunkView1.find('LineView').at(2).simulate('mousedown', {button: 0, detail: 1}); window.dispatchEvent(new MouseEvent('mouseup')); - const line2StagingPromise = eventWatcher.getFinishStageOperationPromise(); + const line2StagingPromise = switchboard.getFinishStageOperationPromise(); hunkView1.find('.github-HunkView-stageButton').simulate('click'); await line2StagingPromise; @@ -446,12 +446,12 @@ describe('FilePatchController', function() { // ensure staging the same hunk twice does not cause issues // second stage action is a no-op since the first staging operation is in flight - const hunk1StagingPromise = eventWatcher.getFinishStageOperationPromise(); + const hunk1StagingPromise = switchboard.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('.github-HunkView-stageButton').simulate('click'); wrapper.find('HunkView').at(0).find('.github-HunkView-stageButton').simulate('click'); await hunk1StagingPromise; - const patchPromise0 = eventWatcher.getChangePatchPromise(); + const patchPromise0 = switchboard.getChangePatchPromise(); repository.refresh(); // clear the cached file patches const modifiedFilePatch = await repository.getFilePatchForPath('sample.js'); wrapper.setProps({filePatch: modifiedFilePatch}); @@ -466,7 +466,7 @@ describe('FilePatchController', function() { let actualLines = await repository.readFileFromIndex('sample.js'); assert.autocrlfEqual(actualLines, expectedLines.join('\n')); - const hunk2StagingPromise = eventWatcher.getFinishStageOperationPromise(); + const hunk2StagingPromise = switchboard.getFinishStageOperationPromise(); wrapper.find('HunkView').at(0).find('.github-HunkView-stageButton').simulate('click'); await hunk2StagingPromise; From c4aa4e6b9025825be5e49ae2eb62612d505512c3 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 16:06:21 -0400 Subject: [PATCH 08/20] Fuss with the Yardstick API --- lib/yardstick.js | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/lib/yardstick.js b/lib/yardstick.js index 9709669e0c..aa16575ac3 100644 --- a/lib/yardstick.js +++ b/lib/yardstick.js @@ -10,7 +10,7 @@ class DurationSet { this.marks = []; // eslint-disable-next-line no-console - console.log('%cbegin %c%s:%s', + console.log('%cbegin %c%s', 'font-weight: bold', 'font-weight: normal; font-style: italic; color: blue', this.name); } @@ -36,6 +36,7 @@ class DurationSet { } let durationSets = []; +const activeSets = new Map(); const yardstick = { async saveTo(fileName) { @@ -43,17 +44,20 @@ const yardstick = { await writeFile(fileName, payload); durationSets = []; }, -}; -[ - 'stageFile', 'stageHunk', 'stageLine', - 'unstageFile', 'unstageHunk', 'unstageLine', -].forEach(opName => { - yardstick[opName] = function() { - const ds = new DurationSet(opName); + begin(seriesName) { + const ds = new DurationSet(seriesName); durationSets.push(ds); - return ds; - }; -}); + activeSets.set(seriesName, ds); + }, + + mark(seriesName, eventName) { + const ds = activeSets.get(seriesName); + if (ds === undefined) { + return; + } + ds.mark(eventName); + }, +}; export default yardstick; From 713583063b176569e82516cbd600ac82855a8132 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 16:06:33 -0400 Subject: [PATCH 09/20] Accept a Switchboard in RootController --- lib/controllers/root-controller.js | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/controllers/root-controller.js b/lib/controllers/root-controller.js index 7c4458e705..88194d394e 100644 --- a/lib/controllers/root-controller.js +++ b/lib/controllers/root-controller.js @@ -23,6 +23,7 @@ import RepositoryConflictController from './repository-conflict-controller'; import ModelObserver from '../models/model-observer'; import ModelStateRegistry from '../models/model-state-registry'; import Conflict from '../models/conflicts/conflict'; +import Switchboard from '../switchboard'; import {copyFile, deleteFileOrFolder} from '../helpers'; import {GitError} from '../git-shell-out-strategy'; @@ -47,10 +48,12 @@ export default class RootController extends React.Component { repository: React.PropTypes.object, resolutionProgress: React.PropTypes.object, statusBar: React.PropTypes.object, + switchboard: React.PropTypes.instanceOf(Switchboard), savedState: React.PropTypes.object, } static defaultProps = { + switchboard: new Switchboard(), savedState: {}, } @@ -211,6 +214,7 @@ export default class RootController extends React.Component { openFiles={this.openFiles} discardLines={this.discardLines} undoLastDiscard={this.undoLastDiscard} + switchboard={this.props.switchboard} /> @@ -316,10 +320,14 @@ export default class RootController extends React.Component { if (activate && this.filePatchControllerPane) { this.filePatchControllerPane.activate(); } + this.props.switchboard.didFinishRender(); resolve(); }); } else { - this.setState({...nullFilePatchState}, resolve); + this.setState({...nullFilePatchState}, () => { + this.props.switchboard.didFinishRender(); + resolve(); + }); } }); } From aa6ba373d1e6a88c662d2087f2e99d25e38dc6db Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Tue, 4 Apr 2017 16:06:52 -0400 Subject: [PATCH 10/20] Use the Switchboard to measure stuff with Yardstick --- lib/github-package.js | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/lib/github-package.js b/lib/github-package.js index a06d358e96..c971b6a318 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -15,6 +15,8 @@ import StyleCalculator from './models/style-calculator'; import FilePatchController from './controllers/file-patch-controller'; import RootController from './controllers/root-controller'; import IssueishPaneItem from './atom-items/issueish-pane-item'; +import Switchboard from './switchboard'; +import yardstick from './yardstick'; import GitTimingsView from './views/git-timings-view'; const defaultState = { @@ -40,6 +42,25 @@ export default class GithubPackage { this.subscriptions = new CompositeDisposable(); this.savedState = {}; + + this.switchboard = new Switchboard(); + this.setupYardstick(); + } + + setupYardstick() { + this.subscriptions.add( + this.switchboard.onDidBeginStageOperation(payload => { + if (payload.stage && payload.line) { + yardstick.begin('stageLine'); + } + }), + this.switchboard.onDidUpdateRepository(() => { + yardstick.mark('stageLine', 'update-repository'); + }), + this.switchboard.onDidFinishRender(() => { + yardstick.mark('stageLine', 'finish-render'); + }), + ); } activate(state = {}) { @@ -149,6 +170,7 @@ export default class GithubPackage { savedState={this.savedState.gitController} createRepositoryForProjectPath={this.createRepositoryForProjectPath} cloneRepositoryForProjectPath={this.cloneRepositoryForProjectPath} + switchboard={this.switchboard} />, this.element, ); } @@ -278,6 +300,13 @@ export default class GithubPackage { } setActiveModels(repository, resolutionProgress) { + if (this.activeRepository !== repository) { + if (this.changedRepositorySubscription) { this.changedRepositorySubscription.dispose(); } + this.changedRepositorySubscription = repository.onDidUpdate(() => { + this.switchboard.didUpdateRepository(); + }); + } + this.activeRepository = repository; this.activeResolutionProgress = resolutionProgress; this.rerender(); From 1b62b02145163a06fd3cb862589929ea0575fa35 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 08:57:07 -0400 Subject: [PATCH 11/20] Configuration options to tune what and how to measure --- package.json | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/package.json b/package.json index cebf776ce0..36a28ef378 100644 --- a/package.json +++ b/package.json @@ -113,6 +113,29 @@ "type": "boolean", "default": false, "description": "Write detailed diagnostic information about git operations to the console" + }, + "performanceMask": { + "type": "array", + "default": [".*"], + "items": { + "type": "string" + }, + "description": "Performance event stream patterns to capture" + }, + "performanceToConsole": { + "type": "boolean", + "default": false, + "description": "Log performance data to the console" + }, + "performanceToDirectory": { + "type": "string", + "default": "", + "description": "Log performance data to JSON files in a directory" + }, + "performanceToProfile": { + "type": "boolean", + "default": false, + "description": "Capture CPU profiles" } }, "deserializers": { From 76966ef45cb8128ef45361bc8a8faa9c48fd4f1d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 08:58:39 -0400 Subject: [PATCH 12/20] Capture profiles or log to console --- lib/yardstick.js | 105 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 92 insertions(+), 13 deletions(-) diff --git a/lib/yardstick.js b/lib/yardstick.js index aa16575ac3..3e6ca2d16d 100644 --- a/lib/yardstick.js +++ b/lib/yardstick.js @@ -1,30 +1,52 @@ // Measure elapsed durations from specific beginning points. +import fs from 'fs-extra'; +import path from 'path'; import {writeFile} from './helpers'; -// A sequence of durations since a fixed +// A sequence of durations since a fixed beginning point class DurationSet { constructor(name) { this.name = name; this.startTimestamp = performance.now(); this.marks = []; - // eslint-disable-next-line no-console - console.log('%cbegin %c%s', - 'font-weight: bold', - 'font-weight: normal; font-style: italic; color: blue', this.name); + if (atom.config.get('github.performanceToConsole')) { + // eslint-disable-next-line no-console + console.log('%cbegin %c%s:begin', + 'font-weight: bold', + 'font-weight: normal; font-style: italic; color: blue', this.name); + } + + if (atom.config.get('github.performanceToProfile')) { + // eslint-disable-next-line no-console + console.profile(this.name); + } } mark(eventName) { const duration = performance.now() - this.startTimestamp; - // eslint-disable-next-line no-console - console.log('%cmark %c%s:%s %c%dms', - 'font-weight: bold', - 'font-weight: normal; font-style: italic; color: blue', this.name, eventName, - 'font-style: normal; color: black', duration); + if (atom.config.get('github.performanceToConsole')) { + // eslint-disable-next-line no-console + console.log('%cmark %c%s:%s %c%dms', + 'font-weight: bold', + 'font-weight: normal; font-style: italic; color: blue', this.name, eventName, + 'font-style: normal; color: black', duration); + } + + if (atom.config.get('performanceToDirectory')) { + this.marks.push({eventName, duration}); + } + } + + finish() { + this.mark('finish'); - this.marks.push({eventName, duration}); + if (atom.config.get('github.performanceToProfile')) { + // eslint-disable-next-line no-console + console.profileEnd(this.name); + } } serialize() { @@ -38,26 +60,83 @@ class DurationSet { let durationSets = []; const activeSets = new Map(); +function shouldCapture(seriesName, eventName) { + const anyActive = ['Console', 'Directory', 'Profile'].some(kind => { + const value = atom.config.get(`github.performanceTo${kind}`); + return value !== '' && value !== false; + }); + if (!anyActive) { + return false; + } + + const mask = new RegExp(atom.config.get('github.performanceMask')); + if (!mask.test(`${seriesName}:${eventName}`)) { + return false; + } + + return true; +} + const yardstick = { - async saveTo(fileName) { + async save() { + const destDir = atom.config.get('github.performanceToDirectory'); + if (destDir === '') { + return; + } + const fileName = path.join(destDir, `performance-${Date.now()}.json`); + + await new Promise((resolve, reject) => { + fs.ensureDir(destDir, err => (err ? reject(err) : resolve())); + }); + const payload = JSON.stringify(durationSets.map(set => set.serialize())); await writeFile(fileName, payload); + + if (atom.config.get('github.performanceToConsole')) { + // eslint-disable-next-line no-console + console.log('%saved %c%d series to %s', + 'font-weight: bold', + 'font-weight: normal; color: black', durationSets.length, fileName); + } + durationSets = []; }, begin(seriesName) { + if (!shouldCapture(seriesName, 'begin')) { + return; + } + const ds = new DurationSet(seriesName); - durationSets.push(ds); activeSets.set(seriesName, ds); }, mark(seriesName, eventName) { + if (!shouldCapture(seriesName, eventName)) { + return; + } + const ds = activeSets.get(seriesName); if (ds === undefined) { return; } ds.mark(eventName); }, + + finish(seriesName) { + if (!shouldCapture(seriesName, 'finish')) { + return; + } + + const ds = activeSets.get(seriesName); + if (ds === undefined) { + return; + } + ds.finish(); + + durationSets.push(ds); + activeSets.delete(seriesName); + }, }; export default yardstick; From 22920db8c42fc104830da61a0cad0216e41abc26 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 08:58:51 -0400 Subject: [PATCH 13/20] Completing a render is a pretty common finish point --- lib/github-package.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/github-package.js b/lib/github-package.js index c971b6a318..1c1c612fef 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -58,7 +58,7 @@ export default class GithubPackage { yardstick.mark('stageLine', 'update-repository'); }), this.switchboard.onDidFinishRender(() => { - yardstick.mark('stageLine', 'finish-render'); + yardstick.finish('stageLine'); }), ); } From 75c86da6b329fa7f9bcc0768f9ce040aeee69a00 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 09:28:18 -0400 Subject: [PATCH 14/20] Flush recorded DurationSets to disk at regular intervals --- lib/yardstick.js | 36 +++++++++++++++++++++++++++++++++--- 1 file changed, 33 insertions(+), 3 deletions(-) diff --git a/lib/yardstick.js b/lib/yardstick.js index 3e6ca2d16d..a0dbfddf25 100644 --- a/lib/yardstick.js +++ b/lib/yardstick.js @@ -4,12 +4,20 @@ import fs from 'fs-extra'; import path from 'path'; import {writeFile} from './helpers'; -// A sequence of durations since a fixed beginning point +// The maximum number of marks within a single DurationSet. A DurationSet will be automatically finished if this many +// marks are recorded. +const MAXIMUM_MARKS = 100; + +// Flush all non-active DurationSets to disk each time that this many marks have been accumulated. +const PERSIST_INTERVAL = 1000; + +// A sequence of durations measured from a fixed beginning point. class DurationSet { constructor(name) { this.name = name; this.startTimestamp = performance.now(); this.marks = []; + this.markCount = 0; if (atom.config.get('github.performanceToConsole')) { // eslint-disable-next-line no-console @@ -35,9 +43,14 @@ class DurationSet { 'font-style: normal; color: black', duration); } - if (atom.config.get('performanceToDirectory')) { + if (atom.config.get('github.performanceToDirectory') !== '') { this.marks.push({eventName, duration}); } + + this.markCount++; + if (this.markCount >= MAXIMUM_MARKS) { + this.finish(); + } } finish() { @@ -55,9 +68,14 @@ class DurationSet { markers: this.marks, }; } + + getCount() { + return this.marks.length; + } } let durationSets = []; +let totalMarkCount = 0; const activeSets = new Map(); function shouldCapture(seriesName, eventName) { @@ -94,7 +112,7 @@ const yardstick = { if (atom.config.get('github.performanceToConsole')) { // eslint-disable-next-line no-console - console.log('%saved %c%d series to %s', + console.log('%csaved %c%d series to %s', 'font-weight: bold', 'font-weight: normal; color: black', durationSets.length, fileName); } @@ -136,6 +154,18 @@ const yardstick = { durationSets.push(ds); activeSets.delete(seriesName); + + totalMarkCount += ds.getCount(); + if (totalMarkCount >= PERSIST_INTERVAL) { + totalMarkCount = 0; + this.save(); + } + }, + + async flush() { + durationSets.push(...activeSets.values()); + activeSets.clear(); + await this.save(); }, }; From 38329fb53a6475fa645d199da3d7da0bca362a82 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 09:28:37 -0400 Subject: [PATCH 15/20] Flush final DurationSets to disk during package deactivation --- lib/github-package.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/github-package.js b/lib/github-package.js index 1c1c612fef..999158e676 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -178,7 +178,10 @@ export default class GithubPackage { async deactivate() { this.subscriptions.dispose(); if (this.destroyedRepositorySubscription) { this.destroyedRepositorySubscription.dispose(); } - await this.destroyModelsForPaths(Array.from(this.modelPromisesByProjectPath.keys())); + await Promise.all([ + this.destroyModelsForPaths(Array.from(this.modelPromisesByProjectPath.keys())), + yardstick.flush(), + ]); } consumeStatusBar(statusBar) { From 0221e22653d5d64415057c53a1e59a0ed045910f Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 09:39:16 -0400 Subject: [PATCH 16/20] Provide some context for didFinishRender events --- lib/controllers/root-controller.js | 4 ++-- lib/decorators/observe-model.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/controllers/root-controller.js b/lib/controllers/root-controller.js index 88194d394e..b3c8e1a3ab 100644 --- a/lib/controllers/root-controller.js +++ b/lib/controllers/root-controller.js @@ -320,12 +320,12 @@ export default class RootController extends React.Component { if (activate && this.filePatchControllerPane) { this.filePatchControllerPane.activate(); } - this.props.switchboard.didFinishRender(); + this.props.switchboard.didFinishRender('RootController.showFilePatchForPath'); resolve(); }); } else { this.setState({...nullFilePatchState}, () => { - this.props.switchboard.didFinishRender(); + this.props.switchboard.didFinishRender('RootController.showFilePatchForPath'); resolve(); }); } diff --git a/lib/decorators/observe-model.js b/lib/decorators/observe-model.js index af2f9f9832..54fd69e29f 100644 --- a/lib/decorators/observe-model.js +++ b/lib/decorators/observe-model.js @@ -48,7 +48,7 @@ export default function ObserveModelDecorator(spec) { this.setState({modelData: this.modelObserver.getActiveModelData()}, () => { /* eslint-disable react/prop-types */ if (this.props.switchboard) { - this.props.switchboard.didFinishRender(); + this.props.switchboard.didFinishRender('ObserveModel.didUpdate'); } /* eslint-enable react/prop-types */ this.resolve(); From ef109a7c8b90d6e47bb1232591f7e3371d006d55 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 09:47:59 -0400 Subject: [PATCH 17/20] Only subscribe to onDidUpdate with an actual Repository --- lib/github-package.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/github-package.js b/lib/github-package.js index 999158e676..e32b6d7569 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -305,9 +305,11 @@ export default class GithubPackage { setActiveModels(repository, resolutionProgress) { if (this.activeRepository !== repository) { if (this.changedRepositorySubscription) { this.changedRepositorySubscription.dispose(); } - this.changedRepositorySubscription = repository.onDidUpdate(() => { - this.switchboard.didUpdateRepository(); - }); + if (repository) { + this.changedRepositorySubscription = repository.onDidUpdate(() => { + this.switchboard.didUpdateRepository(); + }); + } } this.activeRepository = repository; From e921b385edd3e029d53a4d7001ed8c06a2d7a710 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 09:48:18 -0400 Subject: [PATCH 18/20] Apparently that can be `undefined` in tests? --- lib/yardstick.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/yardstick.js b/lib/yardstick.js index a0dbfddf25..f1c1fcbc23 100644 --- a/lib/yardstick.js +++ b/lib/yardstick.js @@ -98,7 +98,7 @@ function shouldCapture(seriesName, eventName) { const yardstick = { async save() { const destDir = atom.config.get('github.performanceToDirectory'); - if (destDir === '') { + if (destDir === '' || destDir === undefined || destDir === null) { return; } const fileName = path.join(destDir, `performance-${Date.now()}.json`); From 152f8bd422d4da199a0d6999472169b79d8c84b4 Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 10:14:18 -0400 Subject: [PATCH 19/20] Accept Arrays of series names in .mark() and .finish() --- lib/yardstick.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/yardstick.js b/lib/yardstick.js index f1c1fcbc23..dd84fee039 100644 --- a/lib/yardstick.js +++ b/lib/yardstick.js @@ -130,6 +130,13 @@ const yardstick = { }, mark(seriesName, eventName) { + if (seriesName instanceof Array) { + for (let i = 0; i < seriesName.length; i++) { + this.mark(seriesName[i], eventName); + } + return; + } + if (!shouldCapture(seriesName, eventName)) { return; } @@ -142,6 +149,13 @@ const yardstick = { }, finish(seriesName) { + if (seriesName instanceof Array) { + for (let i = 0; i < seriesName.length; i++) { + this.finish(seriesName[i]); + } + return; + } + if (!shouldCapture(seriesName, 'finish')) { return; } From 06d18d2e9536bad89e8e7c5430e1cc662dcbd60d Mon Sep 17 00:00:00 2001 From: Ash Wilson Date: Wed, 5 Apr 2017 10:15:06 -0400 Subject: [PATCH 20/20] Capture (un)?stage hunk operations and staging operations --- lib/github-package.js | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/lib/github-package.js b/lib/github-package.js index e32b6d7569..1b384e4631 100644 --- a/lib/github-package.js +++ b/lib/github-package.js @@ -48,17 +48,27 @@ export default class GithubPackage { } setupYardstick() { + const stagingSeries = ['stageLine', 'stageHunk', 'unstageLine', 'unstageHunk']; + this.subscriptions.add( this.switchboard.onDidBeginStageOperation(payload => { if (payload.stage && payload.line) { yardstick.begin('stageLine'); + } else if (payload.stage && payload.hunk) { + yardstick.begin('stageHunk'); + } else if (payload.unstage && payload.line) { + yardstick.begin('unstageLine'); + } else if (payload.unstage && payload.hunk) { + yardstick.begin('unstageHunk'); } }), this.switchboard.onDidUpdateRepository(() => { - yardstick.mark('stageLine', 'update-repository'); + yardstick.mark(stagingSeries, 'update-repository'); }), - this.switchboard.onDidFinishRender(() => { - yardstick.finish('stageLine'); + this.switchboard.onDidFinishRender(context => { + if (context === 'RootController.showFilePatchForPath') { + yardstick.finish(stagingSeries); + } }), ); }