From b8cbbf295d2df505cadd6df610ccf31ecf75a6dd Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Tue, 13 Dec 2016 22:21:00 -0800 Subject: [PATCH 01/16] WIP: move state out of CommitView --- lib/controllers/commit-view-controller.js | 97 +++++++++++++++++++++ lib/controllers/git-controller.js | 6 +- lib/controllers/git-panel-controller.js | 55 ++++++------ lib/views/commit-view.js | 100 +++++----------------- lib/views/git-panel-view.js | 11 +-- 5 files changed, 159 insertions(+), 110 deletions(-) create mode 100644 lib/controllers/commit-view-controller.js diff --git a/lib/controllers/commit-view-controller.js b/lib/controllers/commit-view-controller.js new file mode 100644 index 0000000000..c018032ecb --- /dev/null +++ b/lib/controllers/commit-view-controller.js @@ -0,0 +1,97 @@ +/** @babel */ +/** @jsx etch.dom */ +/* eslint react/no-unknown-property: "off" */ + +import etch from 'etch'; + +import CommitView from '../views/commit-view'; + +const viewStateByRepository = new WeakMap(); +export default class CommitViewController { + constructor(props) { + this.props = props; + this.loadViewState(props.repository); + + this.commit = this.commit.bind(this); + this.handleMessageChange = this.handleMessageChange.bind(this); + + etch.initialize(this); + } + + update(props) { + if (this.props.repository !== props.repository) { + this.saveViewState(this.props.repository); + this.loadViewState(props.repository); + } + if (this.props.lastCommit !== props.lastCommit) { + this.amendingCommitMessage = props.lastCommit && props.lastCommit.message; + } + this.props = props; + etch.update(this); + } + + saveViewState(repository) { + if (repository) { + const viewState = { + regularCommitMessage: this.regularCommitMessage, + amendingCommitMessage: this.amendingCommitMessage, + }; + viewStateByRepository.set(repository, viewState); + } + } + + loadViewState(repository) { + let viewState = {}; + if (repository && viewStateByRepository.has(repository)) { + viewState = viewStateByRepository.get(repository); + } + this.regularCommitMessage = viewState.regularCommitMessage || ''; + this.amendingCommitMessage = viewState.amendingCommitMessage || ''; + } + + render() { + let message = this.props.isAmending ? this.amendingCommitMessage : this.regularCommitMessage; + if (this.props.mergeMessage && message === '') { + message = this.props.mergeMessage; + } + return ( + + ); + } + + async commit(message) { + await this.props.commit(message); + this.regularCommitMessage = ''; + etch.update(this); + } + + handleMessageChange(newMessage) { + if (this.props.isAmending) { + this.amendingCommitMessage = newMessage; + } else { + this.regularCommitMessage = newMessage; + } + etch.update(this); + } + + destroy() { + this.saveViewState(this.props.repository); + return etch.destroy(this); + } +} diff --git a/lib/controllers/git-controller.js b/lib/controllers/git-controller.js index 14822c4686..a5a9fe1799 100644 --- a/lib/controllers/git-controller.js +++ b/lib/controllers/git-controller.js @@ -19,7 +19,6 @@ const nullFilePatchState = { filePath: null, filePatch: null, stagingStatus: null, - amending: null, }; export default class GitController extends React.Component { @@ -46,6 +45,7 @@ export default class GitController extends React.Component { super(props, context); this.state = { ...nullFilePatchState, + amending: false, gitPanelActive: !!props.savedState.gitPanelActive, }; @@ -116,6 +116,7 @@ export default class GitController extends React.Component { commandRegistry={this.props.commandRegistry} notificationManager={this.props.notificationManager} repository={this.props.repository} + isAmending={this.state.amending} didSelectFilePath={this.showFilePatchForPath} didSelectMergeConflictFile={this.showMergeConflictFileForPath} didChangeAmending={this.didChangeAmending} @@ -162,7 +163,7 @@ export default class GitController extends React.Component { const filePatch = await repository.getFilePatchForPath(filePath, {staged: stagingStatus === 'staged', amending}); if (filePatch) { - this.setState({filePath, filePatch, stagingStatus, amending}, () => { + this.setState({filePath, filePatch, stagingStatus}, () => { // TODO: can be better done w/ a prop? if (activate && this.filePatchControllerPane) { this.filePatchControllerPane.activate(); @@ -188,6 +189,7 @@ export default class GitController extends React.Component { } didChangeAmending(isAmending) { + this.setState({amending: isAmending}); return this.showFilePatchForPath(this.state.filePath, this.state.stagingStatus, {amending: isAmending}); } diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index 7b300aa22e..f343db28c6 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -17,7 +17,6 @@ export default class GitPanelController { this.setAmending = this.setAmending.bind(this); this.checkout = this.checkout.bind(this); this.abortMerge = this.abortMerge.bind(this); - this.viewStateByRepository = new WeakMap(); this.repositoryObserver = new ModelObserver({ fetchData: this.fetchRepositoryData.bind(this), didUpdate: () => etch.update(this), @@ -26,26 +25,13 @@ export default class GitPanelController { etch.initialize(this); } - viewStateForRepository(repository) { - if (repository) { - if (!this.viewStateByRepository.has(repository)) { - this.viewStateByRepository.set(repository, {}); - } - return this.viewStateByRepository.get(repository); - } else { - return null; - } - } - render() { - const viewState = this.viewStateForRepository(this.getActiveRepository()); const modelData = this.repositoryObserver.getActiveModelData() || {fetchInProgress: true}; return ( { etch.update(this); }), - this.editor.getBuffer().onDidChangeText(() => { etch.update(this); }), + this.editor.onDidChange(() => this.props.onChangeMessage(this.editor.getText())), props.commandRegistry.add(this.element, {'git:commit': () => this.commit()}), ); - this.setMessageAndAmendStatus(); - this.updateStateForRepository(); const grammar = atom.grammars.grammarForScopeName(COMMIT_GRAMMAR_SCOPE); if (grammar) { @@ -38,8 +38,12 @@ export default class CommitView { } update(props) { + const previousMessage = this.props.message; + const newMessage = props.message; + if (this.editor && previousMessage !== newMessage && this.editor.getText() !== newMessage) { + this.editor.setText(newMessage); + } this.props = {...this.props, ...props}; - this.setMessageAndAmendStatus(); return etch.update(this); } @@ -52,33 +56,6 @@ export default class CommitView { this.grammarSubscription.dispose(); } - setMessageAndAmendStatus() { - const viewState = this.props.viewState || {}; - const message = viewState.message || ''; - if (this.props.message && message === '') { - this.editor.setText(this.props.message); - this.editor.setCursorBufferPosition([0, 0]); - } else { - this.editor.setText(message || ''); - if (viewState.cursorPosition) { this.editor.setCursorBufferPosition(viewState.cursorPosition); } - } - this.refs.amend.checked = Boolean(viewState.amendInProgress); - } - - updateStateForRepository() { - if (this.props.viewState) { - Object.assign(this.props.viewState, { - message: this.editor.getText(), - cursorPosition: this.editor.getCursorBufferPosition(), - amendInProgress: this.refs.amend.checked, - }); - } - } - - readAfterUpdate() { - this.updateStateForRepository(); - } - render() { let remainingCharsClassName = ''; if (this.getRemainingCharacters() < 0) { @@ -104,10 +81,16 @@ export default class CommitView { onclick={this.abortMerge} style={{display: this.props.isMerging ? '' : 'none'}}>Abort Merge
{this.getRemainingCharacters()} @@ -117,55 +100,18 @@ export default class CommitView { ); } - async abortMerge() { - const choice = atom.confirm({ - message: 'Abort merge', - detailedMessage: 'Are you sure?', - buttons: ['Abort', 'Cancel'], - }); - if (choice !== 0) { return null; } - - try { - await this.props.abortMerge(); - this.editor.setText(''); - } catch (e) { - if (e.code === 'EDIRTYSTAGED') { - this.props.notificationManager.addError(`Cannot abort because ${e.path} is both dirty and staged.`); - } - } - return etch.update(this); + abortMerge() { + this.props.abortMerge(); } - async handleAmendBoxClick() { - const checked = this.refs.amend.checked; - const viewState = this.props.viewState || {}; - viewState.amendInProgress = checked; - if (checked) { - viewState.messagePriorToAmending = this.editor.getText(); - const lastCommitMessage = this.props.lastCommit ? this.props.lastCommit.message : ''; - this.editor.setText(lastCommitMessage); - } else { - this.editor.setText(viewState.messagePriorToAmending || ''); - } - this.editor.setCursorBufferPosition([0, 0]); - if (this.props.setAmending) { await this.props.setAmending(checked); } - return etch.update(this); + handleAmendBoxClick() { + this.props.setAmending(this.refs.amend.checked); } - async commit() { + commit() { if (this.isCommitButtonEnabled()) { - try { - await this.props.commit(this.editor.getText(), {amend: this.refs.amend.checked}); - this.editor.setText(''); - this.editor.getBuffer().clearUndoStack(); - } catch (e) { - if (e.code === 'ECONFLICT') { - this.props.notificationManager.addError('Cannot commit without resolving all the merge conflicts first.'); - } - } + this.props.commit(this.editor.getText()); } - this.refs.amend.checked = false; - return etch.update(this); } getRemainingCharacters() { diff --git a/lib/views/git-panel-view.js b/lib/views/git-panel-view.js index a6cd90394b..bd26f98864 100644 --- a/lib/views/git-panel-view.js +++ b/lib/views/git-panel-view.js @@ -5,7 +5,7 @@ import etch from 'etch'; import StagingView from './staging-view'; -import CommitView from './commit-view'; +import CommitViewController from '../controllers/commit-view-controller'; export default class GitPanelView { constructor(props) { @@ -47,25 +47,22 @@ export default class GitPanelView { lastCommit={this.props.lastCommit} isAmending={this.props.isAmending} /> - 0} mergeConflictsExist={this.props.mergeConflicts.length > 0} commit={this.props.commit} + amending={this.props.amending} setAmending={this.props.setAmending} disableCommitButton={this} abortMerge={this.props.abortMerge} branchName={this.props.branchName} commandRegistry={this.props.commandRegistry} - notificationManager={this.props.notificationManager} - maximumCharacterLimit={72} message={this.props.mergeMessage} isMerging={this.props.isMerging} isAmending={this.props.isAmending} lastCommit={this.props.lastCommit} repository={this.props.repository} - viewState={this.props.viewState} />
); From 1a423080c7e822f981dd98eaa44ccfcc49b37d7d Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Tue, 13 Dec 2016 23:43:36 -0800 Subject: [PATCH 02/16] :fire: unused disableCommitButton --- lib/controllers/commit-view-controller.js | 1 - lib/views/git-panel-view.js | 1 - 2 files changed, 2 deletions(-) diff --git a/lib/controllers/commit-view-controller.js b/lib/controllers/commit-view-controller.js index c018032ecb..220f98dc97 100644 --- a/lib/controllers/commit-view-controller.js +++ b/lib/controllers/commit-view-controller.js @@ -61,7 +61,6 @@ export default class CommitViewController { mergeConflictsExist={this.props.mergeConflictsExist} commit={this.commit} setAmending={this.props.setAmending} - disableCommitButton={this} abortMerge={this.props.abortMerge} branchName={this.props.branchName} commandRegistry={this.props.commandRegistry} diff --git a/lib/views/git-panel-view.js b/lib/views/git-panel-view.js index bd26f98864..369e186044 100644 --- a/lib/views/git-panel-view.js +++ b/lib/views/git-panel-view.js @@ -54,7 +54,6 @@ export default class GitPanelView { commit={this.props.commit} amending={this.props.amending} setAmending={this.props.setAmending} - disableCommitButton={this} abortMerge={this.props.abortMerge} branchName={this.props.branchName} commandRegistry={this.props.commandRegistry} From 1664d1dbd2544bb9bef73e84713497299a4d50bc Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 13 Dec 2016 23:53:45 -0800 Subject: [PATCH 03/16] Fix merge message prop name --- lib/views/git-panel-view.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/views/git-panel-view.js b/lib/views/git-panel-view.js index 369e186044..7d2e87e95e 100644 --- a/lib/views/git-panel-view.js +++ b/lib/views/git-panel-view.js @@ -57,7 +57,7 @@ export default class GitPanelView { abortMerge={this.props.abortMerge} branchName={this.props.branchName} commandRegistry={this.props.commandRegistry} - message={this.props.mergeMessage} + mergeMessage={this.props.mergeMessage} isMerging={this.props.isMerging} isAmending={this.props.isAmending} lastCommit={this.props.lastCommit} From 14e5e474423fb12c29336f9d27720e81d58c78be Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 14 Dec 2016 00:38:43 -0800 Subject: [PATCH 04/16] WIP fix tests --- lib/views/commit-view.js | 7 +-- test/views/commit-view.test.js | 87 ++++++++++++---------------------- 2 files changed, 35 insertions(+), 59 deletions(-) diff --git a/lib/views/commit-view.js b/lib/views/commit-view.js index 4c60695628..a1fb1df818 100644 --- a/lib/views/commit-view.js +++ b/lib/views/commit-view.js @@ -19,7 +19,8 @@ export default class CommitView { this.editor = this.refs.editor; this.editor.setText(this.props.message || ''); this.subscriptions = new CompositeDisposable( - this.editor.onDidChange(() => this.props.onChangeMessage(this.editor.getText())), + this.editor.onDidChange(() => this.props.onChangeMessage && this.props.onChangeMessage(this.editor.getText())), + this.editor.onDidChangeCursorPosition(() => { etch.update(this); }), props.commandRegistry.add(this.element, {'git:commit': () => this.commit()}), ); @@ -39,11 +40,11 @@ export default class CommitView { update(props) { const previousMessage = this.props.message; - const newMessage = props.message; + this.props = {...this.props, ...props}; + const newMessage = this.props.message; if (this.editor && previousMessage !== newMessage && this.editor.getText() !== newMessage) { this.editor.setText(newMessage); } - this.props = {...this.props, ...props}; return etch.update(this); } diff --git a/test/views/commit-view.test.js b/test/views/commit-view.test.js index c0caaf0c88..e18310ebdb 100644 --- a/test/views/commit-view.test.js +++ b/test/views/commit-view.test.js @@ -25,50 +25,41 @@ describe('CommitView', () => { }); it('displays the remaining characters limit based on which line is being edited', async () => { - const workdirPath = await cloneRepository('three-files'); - const repository = await buildRepository(workdirPath); - const viewState = {}; - const view = new CommitView({workspace, repository, commandRegistry, stagedChangesExist: true, maximumCharacterLimit: 72, viewState}); - const {editor} = view.refs; + const view = new CommitView({commandRegistry, stagedChangesExist: true, maximumCharacterLimit: 72, message: ''}); assert.equal(view.refs.remainingCharacters.textContent, '72'); - editor.insertText('abcde fghij'); - await etch.getScheduler().getNextUpdatePromise(); + await view.update({message: 'abcde fghij'}); assert.equal(view.refs.remainingCharacters.textContent, '61'); assert(!view.refs.remainingCharacters.classList.contains('is-error')); assert(!view.refs.remainingCharacters.classList.contains('is-warning')); - editor.insertText('\nklmno'); - await etch.getScheduler().getNextUpdatePromise(); + await view.update({message: '\nklmno'}); assert.equal(view.refs.remainingCharacters.textContent, '∞'); assert(!view.refs.remainingCharacters.classList.contains('is-error')); assert(!view.refs.remainingCharacters.classList.contains('is-warning')); - editor.insertText('\npqrst'); - await etch.getScheduler().getNextUpdatePromise(); + await view.update({message: 'abcde\npqrst'}); assert.equal(view.refs.remainingCharacters.textContent, '∞'); assert(!view.refs.remainingCharacters.classList.contains('is-error')); assert(!view.refs.remainingCharacters.classList.contains('is-warning')); - editor.setCursorBufferPosition([0, 3]); + view.editor.setCursorBufferPosition([0, 3]); await etch.getScheduler().getNextUpdatePromise(); - assert.equal(view.refs.remainingCharacters.textContent, '61'); + assert.equal(view.refs.remainingCharacters.textContent, '67'); assert(!view.refs.remainingCharacters.classList.contains('is-error')); assert(!view.refs.remainingCharacters.classList.contains('is-warning')); await view.update({stagedChangesExist: true, maximumCharacterLimit: 50}); - assert.equal(view.refs.remainingCharacters.textContent, '39'); + assert.equal(view.refs.remainingCharacters.textContent, '45'); assert(!view.refs.remainingCharacters.classList.contains('is-error')); assert(!view.refs.remainingCharacters.classList.contains('is-warning')); - editor.insertText('abcde fghij klmno pqrst uvwxyz'); - await etch.getScheduler().getNextUpdatePromise(); + await view.update({message: 'a'.repeat(41)}); assert.equal(view.refs.remainingCharacters.textContent, '9'); assert(!view.refs.remainingCharacters.classList.contains('is-error')); assert(view.refs.remainingCharacters.classList.contains('is-warning')); - editor.insertText('ABCDE FGHIJ KLMNO'); - await etch.getScheduler().getNextUpdatePromise(); + await view.update({message: 'a'.repeat(58)}); assert.equal(view.refs.remainingCharacters.textContent, '-8'); assert(view.refs.remainingCharacters.classList.contains('is-error')); assert(!view.refs.remainingCharacters.classList.contains('is-warning')); @@ -119,20 +110,14 @@ describe('CommitView', () => { }); it('calls props.commit(message) when the commit button is clicked or git:commit is dispatched', async () => { - const workdirPath = await cloneRepository('three-files'); - const repository = await buildRepository(workdirPath); const commit = sinon.spy(); - const view = new CommitView({workspace, commandRegistry, stagedChangesExist: false, commit}); + const view = new CommitView({workspace, commandRegistry, stagedChangesExist: false, commit, message: ''}); const {editor, commitButton} = view.refs; // commit by clicking the commit button - await view.update({repository, stagedChangesExist: true}); - editor.setText('Commit 1'); - await etch.getScheduler().getNextUpdatePromise(); + await view.update({stagedChangesExist: true, message: 'Commit 1'}); commitButton.dispatchEvent(new MouseEvent('click')); - await etch.getScheduler().getNextUpdatePromise(); assert.equal(commit.args[0][0], 'Commit 1'); - assert.equal(editor.getText(), ''); // undo history is cleared commandRegistry.dispatch(editor.element, 'core:undo'); @@ -140,50 +125,40 @@ describe('CommitView', () => { // commit via the git:commit command commit.reset(); - await view.update({repository, stagedChangesExist: true}); - editor.setText('Commit 2'); - await etch.getScheduler().getNextUpdatePromise(); + await view.update({stagedChangesExist: true, message: 'Commit 2'}); commandRegistry.dispatch(editor.element, 'git:commit'); - await etch.getScheduler().getNextUpdatePromise(); assert.equal(commit.args[0][0], 'Commit 2'); - assert.equal(editor.getText(), ''); // disable git:commit when there are no staged changes... commit.reset(); - await view.update({repository, stagedChangesExist: false}); - editor.setText('Commit 4'); - await etch.getScheduler().getNextUpdatePromise(); + await view.update({stagedChangesExist: false, message: 'Commit 4'}); commandRegistry.dispatch(editor.element, 'git:commit'); - await etch.getScheduler().getNextUpdatePromise(); assert.equal(commit.callCount, 0); - assert.equal(editor.getText(), 'Commit 4'); // ...or the commit message is empty commit.reset(); - editor.setText(''); - await etch.getScheduler().getNextUpdatePromise(); - await view.update({repository, stagedChangesExist: true}); + await view.update({stagedChangesExist: true, message: ''}); commandRegistry.dispatch(editor.element, 'git:commit'); - await etch.getScheduler().getNextUpdatePromise(); assert.equal(commit.callCount, 0); }); - it('shows an error notification when props.commit() throws an ECONFLICT exception', async () => { - const commit = sinon.spy(async () => { - await Promise.resolve(); - throw new CommitError('ECONFLICT'); - }); - const view = new CommitView({workspace, commandRegistry, notificationManager, stagedChangesExist: true, commit}); - const {editor, commitButton} = view.refs; - editor.setText('A message.'); - await etch.getScheduler().getNextUpdatePromise(); - assert.equal(notificationManager.getNotifications().length, 0); - commitButton.dispatchEvent(new MouseEvent('click')); - await etch.getScheduler().getNextUpdatePromise(); - assert(commit.calledOnce); - assert.equal(editor.getText(), 'A message.'); - assert.equal(notificationManager.getNotifications().length, 1); - }); + // // move to git panel controller + // it('shows an error notification when props.commit() throws an ECONFLICT exception', async () => { + // const commit = sinon.spy(async () => { + // await Promise.resolve(); + // throw new CommitError('ECONFLICT'); + // }); + // const view = new CommitView({workspace, commandRegistry, notificationManager, stagedChangesExist: true, commit}); + // const {editor, commitButton} = view.refs; + // editor.setText('A message.'); + // await etch.getScheduler().getNextUpdatePromise(); + // assert.equal(notificationManager.getNotifications().length, 0); + // commitButton.dispatchEvent(new MouseEvent('click')); + // await etch.getScheduler().getNextUpdatePromise(); + // assert(commit.calledOnce); + // assert.equal(editor.getText(), 'A message.'); + // assert.equal(notificationManager.getNotifications().length, 1); + // }); it('replaces the contents of the commit message when it is empty and a message is supplied from the outside', async () => { const workdirPath = await cloneRepository('three-files'); From 8596256c331ae1d9d584bcede6ae3632be11006b Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Wed, 14 Dec 2016 11:44:32 -0800 Subject: [PATCH 05/16] Fix tests (squash previous WIP commits) --- lib/controllers/git-panel-controller.js | 3 +- test/controllers/git-panel-controller.test.js | 10 +- test/views/commit-view.test.js | 98 +++++++++---------- 3 files changed, 55 insertions(+), 56 deletions(-) diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index f343db28c6..2211d1268c 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -59,6 +59,8 @@ export default class GitPanelController { this.props = {...this.props, ...props}; if (this.props.repository !== oldProps.repository) { await this.repositoryObserver.setActiveModel(props.repository); + } else if (this.props.isAmending !== oldProps.isAmending) { + await this.repositoryObserver.refreshModelData(this.getActiveRepository()); } return etch.update(this); } @@ -158,7 +160,6 @@ export default class GitPanelController { setAmending(isAmending) { this.props.didChangeAmending(isAmending); - return this.repositoryObserver.refreshModelData(this.getActiveRepository()); } async abortMerge() { diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index 0bbac7de91..2e398585d6 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -76,21 +76,21 @@ describe('GitPanelController', () => { const didChangeAmending = sinon.spy(); const workdirPath = await cloneRepository('multiple-commits'); const repository = await buildRepository(workdirPath); - const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending}); + const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending, isAmending: false}); await controller.getLastModelDataRefreshPromise(); assert.deepEqual(controller.refs.gitPanel.props.stagedChanges, []); assert.equal(didChangeAmending.callCount, 0); await controller.setAmending(true); assert.equal(didChangeAmending.callCount, 1); + await controller.update({isAmending: true}); assert.deepEqual( controller.refs.gitPanel.props.stagedChanges, await controller.getActiveRepository().getStagedChangesSinceParentCommit(), ); await controller.commit('Delete most of the code', {amend: true}); - await controller.getLastModelDataRefreshPromise(); - assert(!controller.refs.gitPanel.props.isAmending); + assert.equal(didChangeAmending.callCount, 2); }); describe('integration tests', () => { @@ -99,10 +99,10 @@ describe('GitPanelController', () => { const repository = await buildRepository(workdirPath); fs.writeFileSync(path.join(workdirPath, 'a.txt'), 'a change\n'); fs.unlinkSync(path.join(workdirPath, 'b.txt')); - const controller = new GitPanelController({workspace, commandRegistry, repository}); + const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending: sinon.stub()}); await controller.getLastModelDataRefreshPromise(); const stagingView = controller.refs.gitPanel.refs.stagingView; - const commitView = controller.refs.gitPanel.refs.commitView; + const commitView = controller.refs.gitPanel.refs.commitViewController.refs.commitView; assert.equal(stagingView.props.unstagedChanges.length, 2); assert.equal(stagingView.props.stagedChanges.length, 0); diff --git a/test/views/commit-view.test.js b/test/views/commit-view.test.js index e18310ebdb..fa28de6635 100644 --- a/test/views/commit-view.test.js +++ b/test/views/commit-view.test.js @@ -8,20 +8,15 @@ import CommitView from '../../lib/views/commit-view'; import {AbortMergeError, CommitError} from '../../lib/models/repository'; describe('CommitView', () => { - let atomEnv, workspace, commandRegistry, notificationManager, confirmChoice; + let atomEnv, commandRegistry, notificationManager; beforeEach(() => { atomEnv = global.buildAtomEnvironment(); - workspace = atomEnv.workspace; commandRegistry = atomEnv.commands; - notificationManager = atomEnv.notifications; - confirmChoice = 0; - sinon.stub(atom, 'confirm', () => confirmChoice); }); afterEach(() => { atomEnv.destroy(); - atom.confirm.restore(); }); it('displays the remaining characters limit based on which line is being edited', async () => { @@ -68,14 +63,14 @@ describe('CommitView', () => { it('uses the git commit message grammar when the grammar is loaded', async () => { await atom.packages.activatePackage('language-git'); - const view = new CommitView({workspace, commandRegistry}); + const view = new CommitView({commandRegistry}); assert.equal(view.editor.getGrammar().scopeName, 'text.git-commit'); }); it('uses the git commit message grammar when the grammar has not been loaded', async () => { atom.packages.deactivatePackage('language-git'); - const view = new CommitView({workspace, commandRegistry}); + const view = new CommitView({commandRegistry}); assert(view.editor.getGrammar().scopeName.startsWith('text.plain')); await atom.packages.activatePackage('language-git'); @@ -87,7 +82,7 @@ describe('CommitView', () => { const workdirPath = await cloneRepository('three-files'); const repository = await buildRepository(workdirPath); const viewState = {}; - const view = new CommitView({workspace, repository, commandRegistry, stagedChangesExist: false, viewState}); + const view = new CommitView({repository, commandRegistry, stagedChangesExist: false, viewState}); const {editor, commitButton} = view.refs; assert.isTrue(commitButton.disabled); @@ -111,7 +106,7 @@ describe('CommitView', () => { it('calls props.commit(message) when the commit button is clicked or git:commit is dispatched', async () => { const commit = sinon.spy(); - const view = new CommitView({workspace, commandRegistry, stagedChangesExist: false, commit, message: ''}); + const view = new CommitView({commandRegistry, stagedChangesExist: false, commit, message: ''}); const {editor, commitButton} = view.refs; // commit by clicking the commit button @@ -142,29 +137,30 @@ describe('CommitView', () => { assert.equal(commit.callCount, 0); }); - // // move to git panel controller - // it('shows an error notification when props.commit() throws an ECONFLICT exception', async () => { - // const commit = sinon.spy(async () => { - // await Promise.resolve(); - // throw new CommitError('ECONFLICT'); - // }); - // const view = new CommitView({workspace, commandRegistry, notificationManager, stagedChangesExist: true, commit}); - // const {editor, commitButton} = view.refs; - // editor.setText('A message.'); - // await etch.getScheduler().getNextUpdatePromise(); - // assert.equal(notificationManager.getNotifications().length, 0); - // commitButton.dispatchEvent(new MouseEvent('click')); - // await etch.getScheduler().getNextUpdatePromise(); - // assert(commit.calledOnce); - // assert.equal(editor.getText(), 'A message.'); - // assert.equal(notificationManager.getNotifications().length, 1); - // }); - - it('replaces the contents of the commit message when it is empty and a message is supplied from the outside', async () => { + // FIXME: move to git panel controller + xit('shows an error notification when props.commit() throws an ECONFLICT exception', async () => { + const commit = sinon.spy(async () => { + await Promise.resolve(); + throw new CommitError('ECONFLICT'); + }); + const view = new CommitView({commandRegistry, notificationManager, stagedChangesExist: true, commit}); + const {editor, commitButton} = view.refs; + editor.setText('A message.'); + await etch.getScheduler().getNextUpdatePromise(); + assert.equal(notificationManager.getNotifications().length, 0); + commitButton.dispatchEvent(new MouseEvent('click')); + await etch.getScheduler().getNextUpdatePromise(); + assert(commit.calledOnce); + assert.equal(editor.getText(), 'A message.'); + assert.equal(notificationManager.getNotifications().length, 1); + }); + + // FIXME: this only makes sense in the context of a commitViewController test + xit('replaces the contents of the commit message when it is empty and a message is supplied from the outside', async () => { const workdirPath = await cloneRepository('three-files'); const repository = await buildRepository(workdirPath); const viewState = {}; - const view = new CommitView({workspace, repository, commandRegistry, stagedChangesExist: true, maximumCharacterLimit: 72, viewState}); + const view = new CommitView({repository, commandRegistry, stagedChangesExist: true, maximumCharacterLimit: 72, viewState}); const {editor} = view.refs; editor.setText('message 1'); await etch.getScheduler().getNextUpdatePromise(); @@ -180,7 +176,7 @@ describe('CommitView', () => { }); it('shows the "Abort Merge" button when props.isMerging is true', async () => { - const view = new CommitView({workspace, commandRegistry, stagedChangesExist: true, isMerging: false}); + const view = new CommitView({commandRegistry, stagedChangesExist: true, isMerging: false}); const {abortMergeButton} = view.refs; assert.equal(abortMergeButton.style.display, 'none'); @@ -191,23 +187,22 @@ describe('CommitView', () => { assert.equal(abortMergeButton.style.display, 'none'); }); - it('calls props.abortMerge() when the "Abort Merge" button is clicked and then clears the commit message', async () => { + // FIXME: we should test elsewhere that this clears the mergeMessage prop to commitViewController + it('calls props.abortMerge() when the "Abort Merge" button is clicked', () => { // and then clears the commit message', async () => { const abortMerge = sinon.spy(() => Promise.resolve()); - const view = new CommitView({workspace, commandRegistry, stagedChangesExist: true, isMerging: true, abortMerge}); - const {editor, abortMergeButton} = view.refs; - editor.setText('A message.'); + const view = new CommitView({commandRegistry, stagedChangesExist: true, isMerging: true, abortMerge}); + const {abortMergeButton} = view.refs; abortMergeButton.dispatchEvent(new MouseEvent('click')); - await etch.getScheduler().getNextUpdatePromise(); assert(abortMerge.calledOnce); - assert.equal(editor.getText(), ''); }); - it('shows an error notification when props.abortMerge() throws an EDIRTYSTAGED exception', async () => { + // FIXME: this needs to go elsewhere, e.g. gitPanelController + xit('shows an error notification when props.abortMerge() throws an EDIRTYSTAGED exception', async () => { const abortMerge = sinon.spy(async () => { await Promise.resolve(); throw new AbortMergeError('EDIRTYSTAGED', 'a.txt'); }); - const view = new CommitView({workspace, commandRegistry, notificationManager, stagedChangesExist: true, isMerging: true, abortMerge}); + const view = new CommitView({commandRegistry, notificationManager, stagedChangesExist: true, isMerging: true, abortMerge}); const {editor, abortMergeButton} = view.refs; editor.setText('A message.'); assert.equal(notificationManager.getNotifications().length, 0); @@ -219,11 +214,12 @@ describe('CommitView', () => { }); describe('amending', () => { - it('displays the appropriate commit message and sets the cursor to the beginning of the text', async () => { + // FIXME: move somewhere else + xit('displays the appropriate commit message and sets the cursor to the beginning of the text', async () => { const workdirPath = await cloneRepository('three-files'); const repository = await buildRepository(workdirPath); const viewState = {}; - const view = new CommitView({workspace, repository, commandRegistry, stagedChangesExist: false, lastCommit: {message: 'previous commit\'s message'}, viewState}); + const view = new CommitView({repository, commandRegistry, stagedChangesExist: false, lastCommit: {message: 'previous commit\'s message'}, viewState}); const {editor, amend} = view.refs; editor.setText('some commit message'); @@ -243,10 +239,11 @@ describe('CommitView', () => { assert.deepEqual(editor.getCursorBufferPosition().serialize(), [0, 0]); }); - it('clears the amend checkbox after committing', async () => { + // FIXME: move + xit('clears the amend checkbox after committing', async () => { const workdirPath = await cloneRepository('three-files'); const repository = await buildRepository(workdirPath); - const view = new CommitView({workspace, commandRegistry, stagedChangesExist: false}); + const view = new CommitView({commandRegistry, stagedChangesExist: false}); const {amend} = view.refs; await view.update({repository, stagedChangesExist: true}); assert.isFalse(amend.checked); @@ -258,7 +255,7 @@ describe('CommitView', () => { it('calls props.setAmending() when the box is checked or unchecked', () => { const setAmending = sinon.spy(); - const view = new CommitView({workspace, commandRegistry, stagedChangesExist: false, lastCommit: {message: 'previous commit\'s message'}, setAmending}); + const view = new CommitView({commandRegistry, stagedChangesExist: false, lastCommit: {message: 'previous commit\'s message'}, setAmending}); const {amend} = view.refs; amend.click(); @@ -269,7 +266,8 @@ describe('CommitView', () => { }); }); - describe('when switching between repositories', () => { + // FIXME: move to commit view controller + xdescribe('when switching between repositories', () => { it('retains the commit message and cursor location', async () => { const workdirPath1 = await cloneRepository('multiple-commits'); const repository1 = await buildRepository(workdirPath1); @@ -279,7 +277,7 @@ describe('CommitView', () => { const viewStateForRepo1 = {}; const viewStateForRepo2 = {}; - let viewForRepo1 = new CommitView({workspace, repository: repository1, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo1}); + let viewForRepo1 = new CommitView({repository: repository1, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo1}); let editor = viewForRepo1.refs.editor; const repository1Message = 'commit message for first repo\nsome details about the commit\nmore details'; @@ -290,7 +288,7 @@ describe('CommitView', () => { assert.equal(editor.getText(), repository1Message); assert.deepEqual(editor.getCursorBufferPosition().serialize(), repository1CursorPosition); - let viewForRepo2 = new CommitView({workspace, repository: repository2, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo2}); + let viewForRepo2 = new CommitView({repository: repository2, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo2}); editor = viewForRepo2.refs.editor; assert.equal(editor.getText(), ''); @@ -303,13 +301,13 @@ describe('CommitView', () => { assert.deepEqual(editor.getCursorBufferPosition().serialize(), repository2CursorPosition); // when repository1 is selected, restore its state - viewForRepo1 = new CommitView({workspace, repository: repository1, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo1}); + viewForRepo1 = new CommitView({repository: repository1, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo1}); editor = viewForRepo1.refs.editor; assert.equal(editor.getText(), repository1Message); assert.deepEqual(editor.getCursorBufferPosition().serialize(), repository1CursorPosition); // when repository2 is selected, restore its state - viewForRepo2 = new CommitView({workspace, repository: repository2, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo2}); + viewForRepo2 = new CommitView({repository: repository2, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo2}); editor = viewForRepo2.refs.editor; assert.equal(editor.getText(), repository2Message); assert.deepEqual(editor.getCursorBufferPosition().serialize(), repository2CursorPosition); @@ -326,7 +324,7 @@ describe('CommitView', () => { const viewStateForRepo1 = {}; const viewStateForRepo2 = {}; - const view = new CommitView({workspace, repository: repository1, lastCommit: repository1LastCommit, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo1}); + const view = new CommitView({repository: repository1, lastCommit: repository1LastCommit, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo1}); const {editor, amend} = view.refs; // create message for repository1 From 09f8264f18ac621327cc643bc1768f1c47293b0f Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 14 Dec 2016 13:06:46 -0800 Subject: [PATCH 06/16] moar --- lib/controllers/git-panel-controller.js | 2 +- lib/models/model-state-tracker.js | 1 + test/controllers/commit-view-controller.js | 7 ++++ test/models/model-state-tracker.test.js | 44 ++++++++++++++++++++++ 4 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 lib/models/model-state-tracker.js create mode 100644 test/controllers/commit-view-controller.js create mode 100644 test/models/model-state-tracker.test.js diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index 2211d1268c..8836a8cf09 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -153,7 +153,7 @@ export default class GitPanelController { if (e.code === 'ECONFLICT') { this.props.notificationManager.addError('Cannot commit without resolving all the merge conflicts first.'); } else { - console.error(e); + console.error(e); // eslint-disable-line no-console } } } diff --git a/lib/models/model-state-tracker.js b/lib/models/model-state-tracker.js new file mode 100644 index 0000000000..a1a0174069 --- /dev/null +++ b/lib/models/model-state-tracker.js @@ -0,0 +1 @@ +/** @babel */ diff --git a/test/controllers/commit-view-controller.js b/test/controllers/commit-view-controller.js new file mode 100644 index 0000000000..5cd1772aaa --- /dev/null +++ b/test/controllers/commit-view-controller.js @@ -0,0 +1,7 @@ +/** @babel */ + +/* +manages state for commit view +- per repo commit messages + +*/ diff --git a/test/models/model-state-tracker.test.js b/test/models/model-state-tracker.test.js new file mode 100644 index 0000000000..e54a961422 --- /dev/null +++ b/test/models/model-state-tracker.test.js @@ -0,0 +1,44 @@ +/** @babel */ + +import ModelStateTracker from '../../lib/models/model-state-tracker'; + +describe.only('ModelStateTracker', () => { + it('serializes and deserializes data per model', () => { + const model1 = Symbol('model1'); + const model2 = Symbol('model2'); + let data; + const tracker = new ModelStateTracker(model1, { + serialize: () => { + return data; + }, + deserialize: (state = {}) => { + data = {...state}; + }, + }); + + assert.deepEqual(data, {}); + data = { + hubber: 'kuychaco', + start: [2015, 12, 15], + }; + + tracker.setModel(model2); + assert.deepEqual(data, {}); + data = { + hubber: 'BinaryMuse', + start: [2016, 2, 16], + }; + + tracker.setModel(model1); + assert.deepEqual(data, { + hubber: 'kuychaco', + start: [2015, 12, 15], + }); + + tracker.setModel(model2); + assert.deepEqual(data, { + hubber: 'BinaryMuse', + start: [2015, 2, 16], + }); + }); +}); From 121f23447cba0276929d6ec89a282a4bd84b6d11 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 14 Dec 2016 13:59:56 -0800 Subject: [PATCH 07/16] Differenter state tracking --- lib/controllers/commit-view-controller.js | 56 +++++++++++------------ lib/models/model-state-tracker.js | 47 +++++++++++++++++++ test/models/model-state-tracker.test.js | 42 +++++++++++++---- 3 files changed, 107 insertions(+), 38 deletions(-) diff --git a/lib/controllers/commit-view-controller.js b/lib/controllers/commit-view-controller.js index 220f98dc97..2a70580f02 100644 --- a/lib/controllers/commit-view-controller.js +++ b/lib/controllers/commit-view-controller.js @@ -5,53 +5,50 @@ import etch from 'etch'; import CommitView from '../views/commit-view'; +import ModelStateTracker from '../models/model-state-tracker'; -const viewStateByRepository = new WeakMap(); +const viewStateByRepository = new ModelStateTracker({ + serialize: (_model, controller) => { + return { + regularCommitMessage: controller.regularCommitMessage, + amendingCommitMessage: controller.amendingCommitMessage, + }; + }, + deserialize: (state = {}, repo, controller) => { + if (controller) { + controller.regularCommitMessage = state.regularCommitMessage || ''; + controller.amendingCommitMessage = state.amendingCommitMessage || ''; + } + }, +}); export default class CommitViewController { constructor(props) { this.props = props; - this.loadViewState(props.repository); this.commit = this.commit.bind(this); this.handleMessageChange = this.handleMessageChange.bind(this); + viewStateByRepository.setContext(this); + viewStateByRepository.setModel(props.repository); etch.initialize(this); } update(props) { if (this.props.repository !== props.repository) { - this.saveViewState(this.props.repository); - this.loadViewState(props.repository); - } - if (this.props.lastCommit !== props.lastCommit) { - this.amendingCommitMessage = props.lastCommit && props.lastCommit.message; + viewStateByRepository.setModel(props.repository); } this.props = props; etch.update(this); } - saveViewState(repository) { - if (repository) { - const viewState = { - regularCommitMessage: this.regularCommitMessage, - amendingCommitMessage: this.amendingCommitMessage, - }; - viewStateByRepository.set(repository, viewState); - } - } - - loadViewState(repository) { - let viewState = {}; - if (repository && viewStateByRepository.has(repository)) { - viewState = viewStateByRepository.get(repository); - } - this.regularCommitMessage = viewState.regularCommitMessage || ''; - this.amendingCommitMessage = viewState.amendingCommitMessage || ''; - } - render() { - let message = this.props.isAmending ? this.amendingCommitMessage : this.regularCommitMessage; - if (this.props.mergeMessage && message === '') { + let message = this.regularCommitMessage; + if (this.props.isAmending) { + message = this.amendingCommitMessage; + if (!message.length && this.props.lastCommit) { + message = this.props.lastCommit.message; + } + } else if (!message.length && this.props.mergeMessage) { message = this.props.mergeMessage; } return ( @@ -77,6 +74,7 @@ export default class CommitViewController { async commit(message) { await this.props.commit(message); this.regularCommitMessage = ''; + this.amendingCommitMessage = ''; etch.update(this); } @@ -90,7 +88,7 @@ export default class CommitViewController { } destroy() { - this.saveViewState(this.props.repository); + viewStateByRepository.setModel(null); return etch.destroy(this); } } diff --git a/lib/models/model-state-tracker.js b/lib/models/model-state-tracker.js index a1a0174069..233ecd8cf4 100644 --- a/lib/models/model-state-tracker.js +++ b/lib/models/model-state-tracker.js @@ -1 +1,48 @@ /** @babel */ + +export default class ModelStateTracker { + constructor({initialModel, initialContext, serialize, deserialize} = {}) { + this.statePerModel = new WeakMap(); + this.serializeMethod = serialize; + this.deserializeMethod = deserialize; + this.currentModel = null; + this.context = null; + + this.setContext(initialContext || null); + this.setModel(initialModel || null); + } + + setModel(model) { + if (model === this.currentModel) { return; } + + try { + this.saveState(this.currentModel); + this.loadState(model); + } finally { + this.currentModel = model; + } + } + + setContext(context) { + this.context = context; + } + + saveState(model) { + if (!this.serializeMethod) { throw new Error('No serialize method defined'); } + + if (model) { + const state = this.serializeMethod(model, this.context); + this.statePerModel.set(model, state); + } + } + + loadState(model) { + if (!this.deserializeMethod) { throw new Error('No deserialize method defined'); } + + let state; + if (model && this.statePerModel.has(model)) { + state = this.statePerModel.get(model); + } + this.deserializeMethod(state, model, this.context); + } +} diff --git a/test/models/model-state-tracker.test.js b/test/models/model-state-tracker.test.js index e54a961422..e13572c371 100644 --- a/test/models/model-state-tracker.test.js +++ b/test/models/model-state-tracker.test.js @@ -2,20 +2,31 @@ import ModelStateTracker from '../../lib/models/model-state-tracker'; -describe.only('ModelStateTracker', () => { +describe('ModelStateTracker', () => { it('serializes and deserializes data per model', () => { - const model1 = Symbol('model1'); - const model2 = Symbol('model2'); - let data; - const tracker = new ModelStateTracker(model1, { - serialize: () => { - return data; + const model1 = {model: 1}; + const model2 = {model: 2}; + const context1 = {context: 2}; + const context2 = {context: 2}; + let data, lastSerializeModel, lastSerializeContext, lastDeserializeModel, lastDeserializeContext; + const tracker = new ModelStateTracker({ + initialModel: model1, + initialContext: context1, + serialize: (model, context) => { + lastSerializeModel = model; + lastSerializeContext = context; + return {...data}; }, - deserialize: (state = {}) => { + deserialize: (state = {}, model, context) => { + lastDeserializeModel = model; + lastDeserializeContext = context; data = {...state}; }, }); + tracker.setContext(context1); + assert.equal(lastDeserializeModel, model1); + assert.equal(lastDeserializeContext, context1); assert.deepEqual(data, {}); data = { hubber: 'kuychaco', @@ -23,22 +34,35 @@ describe.only('ModelStateTracker', () => { }; tracker.setModel(model2); + assert.equal(lastSerializeModel, model1); + assert.equal(lastSerializeContext, context1); + assert.equal(lastDeserializeModel, model2); + assert.equal(lastDeserializeContext, context1); assert.deepEqual(data, {}); data = { hubber: 'BinaryMuse', start: [2016, 2, 16], }; + tracker.setContext(context2); tracker.setModel(model1); + assert.equal(lastSerializeModel, model2); + assert.equal(lastSerializeContext, context2); + assert.equal(lastDeserializeModel, model1); + assert.equal(lastDeserializeContext, context2); assert.deepEqual(data, { hubber: 'kuychaco', start: [2015, 12, 15], }); tracker.setModel(model2); + assert.equal(lastSerializeModel, model1); + assert.equal(lastSerializeContext, context2); + assert.equal(lastDeserializeModel, model2); + assert.equal(lastDeserializeContext, context2); assert.deepEqual(data, { hubber: 'BinaryMuse', - start: [2015, 2, 16], + start: [2016, 2, 16], }); }); }); From 8af17c41ccadd7855a7b88f4a9816ef1d2f516d1 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 14 Dec 2016 18:16:42 -0800 Subject: [PATCH 08/16] Create ModelStateRegistry for tracking state per model and type --- lib/models/model-state-registry.js | 29 ++++++ test/models/model-state-registry.test.js | 122 +++++++++++++++++++++++ 2 files changed, 151 insertions(+) create mode 100644 lib/models/model-state-registry.js create mode 100644 test/models/model-state-registry.test.js diff --git a/lib/models/model-state-registry.js b/lib/models/model-state-registry.js new file mode 100644 index 0000000000..3cc2b1842d --- /dev/null +++ b/lib/models/model-state-registry.js @@ -0,0 +1,29 @@ +/** @babel */ + +let statePerType = new WeakMap(); + +export default class { + static clearSavedState() { + statePerType = new WeakMap(); + } + + constructor(type, {initialModel, save, restore}) { + this.save = save; + this.restore = restore; + if (statePerType.has(type)) { + this.statePerModel = statePerType.get(type); + } else { + this.statePerModel = new WeakMap(); + statePerType.set(type, this.statePerModel); + } + this.setModel(initialModel); + } + + setModel(model) { + if (model !== this.model) { + this.model && this.statePerModel.set(this.model, this.save()); + model && this.restore(this.statePerModel.get(model)); + this.model = model; + } + } +} diff --git a/test/models/model-state-registry.test.js b/test/models/model-state-registry.test.js new file mode 100644 index 0000000000..9956afdedf --- /dev/null +++ b/test/models/model-state-registry.test.js @@ -0,0 +1,122 @@ +/** @babel */ + +import sinon from 'sinon'; + +import ModelStateRegistry from '../../lib/models/model-state-registry'; + +const Type1 = {type: 1}; +const Type2 = {type: 2}; + +const model1 = {model: 1}; +const model2 = {model: 2}; + +describe.only('ModelStateRegistry', () => { + beforeEach(() => { + ModelStateRegistry.clearSavedState(); + }); + + describe('#setModel', () => { + it('saves the previous data to be restored later', () => { + let data; + const registry = new ModelStateRegistry(Type1, { + initialModel: model1, + save: () => data, + restore: (saved = {}) => { data = saved; }, + }); + assert.deepEqual(data, {}); + data = {some: 'data'}; + registry.setModel(model2); + assert.deepEqual(data, {}); + registry.setModel(model1); + assert.deepEqual(data, {some: 'data'}); + }); + + it('does not call save or restore if the model has not changed', () => { + let data; + const save = sinon.spy(() => data); + const restore = sinon.spy((saved = {}) => { data = saved; }); + const registry = new ModelStateRegistry(Type1, { + initialModel: model1, + save, + restore, + }); + save.reset(); + restore.reset(); + registry.setModel(model1); + assert.equal(save.callCount, 0); + assert.equal(restore.callCount, 0); + }); + + it('does not call save or restore for a model that does not exist', () => { + const save = sinon.stub(); + const restore = sinon.stub(); + const registry = new ModelStateRegistry(Type1, { + initialModel: model1, + save, restore, + }); + + save.reset(); + restore.reset(); + registry.setModel(null); + assert.equal(save.callCount, 1); + assert.equal(restore.callCount, 0); + + save.reset(); + restore.reset(); + registry.setModel(model1); + assert.equal(save.callCount, 0); + assert.equal(restore.callCount, 1); + }); + }); + + it('shares data across multiple instances given the same type and model', () => { + let data; + const registry1 = new ModelStateRegistry(Type1, { + initialModel: model1, + save: () => data, + restore: (saved = {}) => { data = saved; }, + }); + data = {some: 'data'}; + registry1.setModel(model2); + assert.deepEqual(data, {}); + data = {more: 'datas'}; + registry1.setModel(model1); + + let data2; + const registry2 = new ModelStateRegistry(Type1, { + initialModel: model1, + save: () => data2, + restore: (saved = {}) => { data2 = saved; }, + }); + assert.deepEqual(data2, {some: 'data'}); + registry2.setModel(model2); + assert.deepEqual(data2, {more: 'datas'}); + }); + + it('does not share data across multiple instances given the same model but a different type', () => { + let data; + const registry1 = new ModelStateRegistry(Type1, { + initialModel: model1, + save: () => data, + restore: (saved = {}) => { data = saved; }, + }); + data = {some: 'data'}; + registry1.setModel(model2); + assert.deepEqual(data, {}); + data = {more: 'datas'}; + registry1.setModel(model1); + + let data2; + const registry2 = new ModelStateRegistry(Type2, { + initialModel: model1, + save: () => data2, + restore: (saved = {}) => { data2 = saved; }, + }); + assert.deepEqual(data2, {}); + data2 = {evenMore: 'data'}; + registry2.setModel(model2); + assert.deepEqual(data2, {}); + registry2.setModel(model1); + assert.deepEqual(data2, {evenMore: 'data'}); + }); +}); From dd1dbeb24dd8b0ceb8af2f6f0a108f1feb4273fa Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 14 Dec 2016 19:35:07 -0800 Subject: [PATCH 09/16] :fire: superseded ModelStateTracker --- lib/models/model-state-tracker.js | 48 ----------------- test/models/model-state-tracker.test.js | 68 ------------------------- 2 files changed, 116 deletions(-) delete mode 100644 lib/models/model-state-tracker.js delete mode 100644 test/models/model-state-tracker.test.js diff --git a/lib/models/model-state-tracker.js b/lib/models/model-state-tracker.js deleted file mode 100644 index 233ecd8cf4..0000000000 --- a/lib/models/model-state-tracker.js +++ /dev/null @@ -1,48 +0,0 @@ -/** @babel */ - -export default class ModelStateTracker { - constructor({initialModel, initialContext, serialize, deserialize} = {}) { - this.statePerModel = new WeakMap(); - this.serializeMethod = serialize; - this.deserializeMethod = deserialize; - this.currentModel = null; - this.context = null; - - this.setContext(initialContext || null); - this.setModel(initialModel || null); - } - - setModel(model) { - if (model === this.currentModel) { return; } - - try { - this.saveState(this.currentModel); - this.loadState(model); - } finally { - this.currentModel = model; - } - } - - setContext(context) { - this.context = context; - } - - saveState(model) { - if (!this.serializeMethod) { throw new Error('No serialize method defined'); } - - if (model) { - const state = this.serializeMethod(model, this.context); - this.statePerModel.set(model, state); - } - } - - loadState(model) { - if (!this.deserializeMethod) { throw new Error('No deserialize method defined'); } - - let state; - if (model && this.statePerModel.has(model)) { - state = this.statePerModel.get(model); - } - this.deserializeMethod(state, model, this.context); - } -} diff --git a/test/models/model-state-tracker.test.js b/test/models/model-state-tracker.test.js deleted file mode 100644 index e13572c371..0000000000 --- a/test/models/model-state-tracker.test.js +++ /dev/null @@ -1,68 +0,0 @@ -/** @babel */ - -import ModelStateTracker from '../../lib/models/model-state-tracker'; - -describe('ModelStateTracker', () => { - it('serializes and deserializes data per model', () => { - const model1 = {model: 1}; - const model2 = {model: 2}; - const context1 = {context: 2}; - const context2 = {context: 2}; - let data, lastSerializeModel, lastSerializeContext, lastDeserializeModel, lastDeserializeContext; - const tracker = new ModelStateTracker({ - initialModel: model1, - initialContext: context1, - serialize: (model, context) => { - lastSerializeModel = model; - lastSerializeContext = context; - return {...data}; - }, - deserialize: (state = {}, model, context) => { - lastDeserializeModel = model; - lastDeserializeContext = context; - data = {...state}; - }, - }); - tracker.setContext(context1); - - assert.equal(lastDeserializeModel, model1); - assert.equal(lastDeserializeContext, context1); - assert.deepEqual(data, {}); - data = { - hubber: 'kuychaco', - start: [2015, 12, 15], - }; - - tracker.setModel(model2); - assert.equal(lastSerializeModel, model1); - assert.equal(lastSerializeContext, context1); - assert.equal(lastDeserializeModel, model2); - assert.equal(lastDeserializeContext, context1); - assert.deepEqual(data, {}); - data = { - hubber: 'BinaryMuse', - start: [2016, 2, 16], - }; - - tracker.setContext(context2); - tracker.setModel(model1); - assert.equal(lastSerializeModel, model2); - assert.equal(lastSerializeContext, context2); - assert.equal(lastDeserializeModel, model1); - assert.equal(lastDeserializeContext, context2); - assert.deepEqual(data, { - hubber: 'kuychaco', - start: [2015, 12, 15], - }); - - tracker.setModel(model2); - assert.equal(lastSerializeModel, model1); - assert.equal(lastSerializeContext, context2); - assert.equal(lastDeserializeModel, model2); - assert.equal(lastDeserializeContext, context2); - assert.deepEqual(data, { - hubber: 'BinaryMuse', - start: [2016, 2, 16], - }); - }); -}); From 1dd66628decce0a34812fc7df8f8cebc42d122d3 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 14 Dec 2016 19:36:05 -0800 Subject: [PATCH 10/16] Use ModelStateRegistry to track state per repo in CommitViewController --- lib/controllers/commit-view-controller.js | 37 ++++++++++------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/lib/controllers/commit-view-controller.js b/lib/controllers/commit-view-controller.js index 2a70580f02..9b45482b45 100644 --- a/lib/controllers/commit-view-controller.js +++ b/lib/controllers/commit-view-controller.js @@ -5,38 +5,33 @@ import etch from 'etch'; import CommitView from '../views/commit-view'; -import ModelStateTracker from '../models/model-state-tracker'; +import ModelStateRegistry from '../models/model-state-registry'; -const viewStateByRepository = new ModelStateTracker({ - serialize: (_model, controller) => { - return { - regularCommitMessage: controller.regularCommitMessage, - amendingCommitMessage: controller.amendingCommitMessage, - }; - }, - deserialize: (state = {}, repo, controller) => { - if (controller) { - controller.regularCommitMessage = state.regularCommitMessage || ''; - controller.amendingCommitMessage = state.amendingCommitMessage || ''; - } - }, -}); export default class CommitViewController { constructor(props) { this.props = props; this.commit = this.commit.bind(this); this.handleMessageChange = this.handleMessageChange.bind(this); - viewStateByRepository.setContext(this); - viewStateByRepository.setModel(props.repository); + this.repoStateRegistry = new ModelStateRegistry(CommitViewController, { + initialModel: props.repository, + save: () => { + return { + regularCommitMessage: this.regularCommitMessage, + amendingCommitMessage: this.amendingCommitMessage, + }; + }, + restore: (state = {}) => { + this.regularCommitMessage = state.regularCommitMessage || ''; + this.amendingCommitMessage = state.amendingCommitMessage || ''; + }, + }); etch.initialize(this); } update(props) { - if (this.props.repository !== props.repository) { - viewStateByRepository.setModel(props.repository); - } + this.repoStateRegistry.setModel(props.repository); this.props = props; etch.update(this); } @@ -88,7 +83,7 @@ export default class CommitViewController { } destroy() { - viewStateByRepository.setModel(null); + this.repoStateRegistry.setModel(null); return etch.destroy(this); } } From 88b8a63ed32ead9396468a66b132884ac94ef021 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Wed, 14 Dec 2016 19:36:22 -0800 Subject: [PATCH 11/16] Use ModelStateRegistry to track state per repo in GitController --- lib/controllers/git-controller.js | 13 +++++++++++++ test/models/model-state-registry.test.js | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/controllers/git-controller.js b/lib/controllers/git-controller.js index a5a9fe1799..879a5358d7 100644 --- a/lib/controllers/git-controller.js +++ b/lib/controllers/git-controller.js @@ -14,6 +14,7 @@ import FilePatchController from './file-patch-controller'; import GitPanelController from './git-panel-controller'; import StatusBarTileController from './status-bar-tile-controller'; import ModelObserver from '../models/model-observer'; +import ModelStateRegistry from '../models/model-state-registry'; const nullFilePatchState = { filePath: null, @@ -49,6 +50,16 @@ export default class GitController extends React.Component { gitPanelActive: !!props.savedState.gitPanelActive, }; + this.repositoryStateRegistry = new ModelStateRegistry(GitController, { + initialModel: props.repository, + save: () => { + return {amending: this.state.amending}; + }, + restore: (state = {}) => { + this.setState({amending: state.amending}); + }, + }); + this.showFilePatchForPath = this.showFilePatchForPath.bind(this); this.showMergeConflictFileForPath = this.showMergeConflictFileForPath.bind(this); this.didChangeAmending = this.didChangeAmending.bind(this); @@ -76,6 +87,7 @@ export default class GitController extends React.Component { componentWillReceiveProps(newProps) { this.repositoryObserver.setActiveModel(newProps.repository); + this.repositoryStateRegistry.setModel(newProps.repository); } render() { @@ -147,6 +159,7 @@ export default class GitController extends React.Component { } componentWillUnmount() { + this.repositoryStateRegistry.setModel(null); this.subscriptions.dispose(); } diff --git a/test/models/model-state-registry.test.js b/test/models/model-state-registry.test.js index 9956afdedf..e4eb11034c 100644 --- a/test/models/model-state-registry.test.js +++ b/test/models/model-state-registry.test.js @@ -10,7 +10,7 @@ const Type2 = {type: 2}; const model1 = {model: 1}; const model2 = {model: 2}; -describe.only('ModelStateRegistry', () => { +describe('ModelStateRegistry', () => { beforeEach(() => { ModelStateRegistry.clearSavedState(); }); From 4de44ab228aa9756f40ce3ce01c6ba9cd4138085 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 15 Dec 2016 13:39:22 -0800 Subject: [PATCH 12/16] :white_check_mark: Add tests for CommitViewController --- lib/controllers/commit-view-controller.js | 6 +- test/controllers/commit-view-controller.js | 7 - .../commit-view-controller.test.js | 84 +++++++++ test/views/commit-view.test.js | 159 +----------------- 4 files changed, 88 insertions(+), 168 deletions(-) delete mode 100644 test/controllers/commit-view-controller.js create mode 100644 test/controllers/commit-view-controller.test.js diff --git a/lib/controllers/commit-view-controller.js b/lib/controllers/commit-view-controller.js index 9b45482b45..4baedc3333 100644 --- a/lib/controllers/commit-view-controller.js +++ b/lib/controllers/commit-view-controller.js @@ -31,9 +31,9 @@ export default class CommitViewController { } update(props) { - this.repoStateRegistry.setModel(props.repository); - this.props = props; - etch.update(this); + this.props = {...this.props, ...props}; + this.repoStateRegistry.setModel(this.props.repository); + return etch.update(this); } render() { diff --git a/test/controllers/commit-view-controller.js b/test/controllers/commit-view-controller.js deleted file mode 100644 index 5cd1772aaa..0000000000 --- a/test/controllers/commit-view-controller.js +++ /dev/null @@ -1,7 +0,0 @@ -/** @babel */ - -/* -manages state for commit view -- per repo commit messages - -*/ diff --git a/test/controllers/commit-view-controller.test.js b/test/controllers/commit-view-controller.test.js new file mode 100644 index 0000000000..9ed68c2ded --- /dev/null +++ b/test/controllers/commit-view-controller.test.js @@ -0,0 +1,84 @@ +/** @babel */ + +import CommitViewController from '../../lib/controllers/commit-view-controller'; +import {cloneRepository, buildRepository} from '../helpers'; + +describe('CommitViewController', () => { + let atomEnvironment, commandRegistry; + + beforeEach(() => { + atomEnvironment = global.buildAtomEnvironment(); + commandRegistry = atomEnvironment.commands; + }); + + afterEach(() => { + atomEnvironment.destroy(); + }); + + it('correctly updates state when switching repos', async () => { + const workdirPath1 = await cloneRepository('three-files'); + const repository1 = await buildRepository(workdirPath1); + const workdirPath2 = await cloneRepository('three-files'); + const repository2 = await buildRepository(workdirPath2); + const controller = new CommitViewController({commandRegistry, repository: repository1}); + + assert.equal(controller.regularCommitMessage, ''); + assert.equal(controller.amendingCommitMessage, ''); + + controller.regularCommitMessage = 'regular message 1'; + controller.amendingCommitMessage = 'amending message 1'; + + await controller.update({repository: repository2}); + assert.equal(controller.regularCommitMessage, ''); + assert.equal(controller.amendingCommitMessage, ''); + + await controller.update({repository: repository1}); + assert.equal(controller.regularCommitMessage, 'regular message 1'); + assert.equal(controller.amendingCommitMessage, 'amending message 1'); + }); + + describe('the passed commit message', () => { + let controller, commitView, lastCommit; + beforeEach(async () => { + const workdirPath = await cloneRepository('three-files'); + const repository = await buildRepository(workdirPath); + controller = new CommitViewController({commandRegistry, repository}); + commitView = controller.refs.commitView; + lastCommit = {sha: 'a1e23fd45', message: 'last commit message'}; + }); + + it('is set to the regularCommitMessage in the default case', async () => { + controller.regularCommitMessage = 'regular message'; + await controller.update(); + assert.equal(commitView.props.message, 'regular message'); + }); + + describe('when isAmending is true', () => { + it('is set to the last commits message if amendingCommitMessage is blank', async () => { + controller.amendingCommitMessage = 'amending commit message'; + await controller.update({isAmending: true, lastCommit}); + assert.equal(commitView.props.message, 'amending commit message'); + }); + + it('is set to amendingCommitMessage if it is set', async () => { + controller.amendingCommitMessage = 'amending commit message'; + await controller.update({isAmending: true, lastCommit}); + assert.equal(commitView.props.message, 'amending commit message'); + }); + }); + + describe('when a merge message is defined', () => { + it('is set to the merge message if regularCommitMessage is blank', async () => { + controller.regularCommitMessage = ''; + await controller.update({mergeMessage: 'merge conflict!'}); + assert.equal(commitView.props.message, 'merge conflict!'); + }); + + it('is set to regularCommitMessage if it is set', async () => { + controller.regularCommitMessage = 'regular commit message'; + await controller.update({mergeMessage: 'merge conflict!'}); + assert.equal(commitView.props.message, 'regular commit message'); + }); + }); + }); +}); diff --git a/test/views/commit-view.test.js b/test/views/commit-view.test.js index fa28de6635..24f3688df6 100644 --- a/test/views/commit-view.test.js +++ b/test/views/commit-view.test.js @@ -155,26 +155,6 @@ describe('CommitView', () => { assert.equal(notificationManager.getNotifications().length, 1); }); - // FIXME: this only makes sense in the context of a commitViewController test - xit('replaces the contents of the commit message when it is empty and a message is supplied from the outside', async () => { - const workdirPath = await cloneRepository('three-files'); - const repository = await buildRepository(workdirPath); - const viewState = {}; - const view = new CommitView({repository, commandRegistry, stagedChangesExist: true, maximumCharacterLimit: 72, viewState}); - const {editor} = view.refs; - editor.setText('message 1'); - await etch.getScheduler().getNextUpdatePromise(); - assert.equal(editor.getText(), 'message 1'); - - await view.update({message: 'Merge conflict!'}); - assert.equal(editor.getText(), 'message 1'); - - editor.setText(''); - await etch.getScheduler().getNextUpdatePromise(); - await view.update({message: 'Merge conflict!'}); - assert.equal(editor.getText(), 'Merge conflict!'); - }); - it('shows the "Abort Merge" button when props.isMerging is true', async () => { const view = new CommitView({commandRegistry, stagedChangesExist: true, isMerging: false}); const {abortMergeButton} = view.refs; @@ -188,7 +168,7 @@ describe('CommitView', () => { }); // FIXME: we should test elsewhere that this clears the mergeMessage prop to commitViewController - it('calls props.abortMerge() when the "Abort Merge" button is clicked', () => { // and then clears the commit message', async () => { + it('calls props.abortMerge() when the "Abort Merge" button is clicked', () => { const abortMerge = sinon.spy(() => Promise.resolve()); const view = new CommitView({commandRegistry, stagedChangesExist: true, isMerging: true, abortMerge}); const {abortMergeButton} = view.refs; @@ -214,31 +194,6 @@ describe('CommitView', () => { }); describe('amending', () => { - // FIXME: move somewhere else - xit('displays the appropriate commit message and sets the cursor to the beginning of the text', async () => { - const workdirPath = await cloneRepository('three-files'); - const repository = await buildRepository(workdirPath); - const viewState = {}; - const view = new CommitView({repository, commandRegistry, stagedChangesExist: false, lastCommit: {message: 'previous commit\'s message'}, viewState}); - const {editor, amend} = view.refs; - - editor.setText('some commit message'); - assert.isFalse(amend.checked); - assert.equal(editor.getText(), 'some commit message'); - - // displays message for last commit - amend.click(); - assert.isTrue(amend.checked); - assert.equal(editor.getText(), 'previous commit\'s message'); - assert.deepEqual(editor.getCursorBufferPosition().serialize(), [0, 0]); - - // restores original message - amend.click(); - assert.isFalse(amend.checked); - assert.equal(editor.getText(), 'some commit message'); - assert.deepEqual(editor.getCursorBufferPosition().serialize(), [0, 0]); - }); - // FIXME: move xit('clears the amend checkbox after committing', async () => { const workdirPath = await cloneRepository('three-files'); @@ -265,116 +220,4 @@ describe('CommitView', () => { assert.deepEqual(setAmending.args, [[true], [false]]); }); }); - - // FIXME: move to commit view controller - xdescribe('when switching between repositories', () => { - it('retains the commit message and cursor location', async () => { - const workdirPath1 = await cloneRepository('multiple-commits'); - const repository1 = await buildRepository(workdirPath1); - const workdirPath2 = await cloneRepository('three-files'); - const repository2 = await buildRepository(workdirPath2); - - const viewStateForRepo1 = {}; - const viewStateForRepo2 = {}; - - let viewForRepo1 = new CommitView({repository: repository1, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo1}); - let editor = viewForRepo1.refs.editor; - - const repository1Message = 'commit message for first repo\nsome details about the commit\nmore details'; - editor.setText(repository1Message); - const repository1CursorPosition = [1, 3]; - editor.setCursorBufferPosition(repository1CursorPosition); - await etch.getScheduler().getNextUpdatePromise(); - assert.equal(editor.getText(), repository1Message); - assert.deepEqual(editor.getCursorBufferPosition().serialize(), repository1CursorPosition); - - let viewForRepo2 = new CommitView({repository: repository2, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo2}); - editor = viewForRepo2.refs.editor; - assert.equal(editor.getText(), ''); - - const repository2Message = 'commit message for second repo'; - editor.setText(repository2Message); - const repository2CursorPosition = [0, 10]; - editor.setCursorBufferPosition(repository2CursorPosition); - await etch.getScheduler().getNextUpdatePromise(); - assert.equal(editor.getText(), repository2Message); - assert.deepEqual(editor.getCursorBufferPosition().serialize(), repository2CursorPosition); - - // when repository1 is selected, restore its state - viewForRepo1 = new CommitView({repository: repository1, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo1}); - editor = viewForRepo1.refs.editor; - assert.equal(editor.getText(), repository1Message); - assert.deepEqual(editor.getCursorBufferPosition().serialize(), repository1CursorPosition); - - // when repository2 is selected, restore its state - viewForRepo2 = new CommitView({repository: repository2, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo2}); - editor = viewForRepo2.refs.editor; - assert.equal(editor.getText(), repository2Message); - assert.deepEqual(editor.getCursorBufferPosition().serialize(), repository2CursorPosition); - }); - - it('retains the amend status and restores the correct commit message when amend state is exited', async () => { - const workdirPath1 = await cloneRepository('multiple-commits'); - const repository1 = await buildRepository(workdirPath1); - const workdirPath2 = await cloneRepository('three-files'); - const repository2 = await buildRepository(workdirPath2); - - const repository1LastCommit = {message: 'first repository\'s previous commit\'s message'}; - const repository2LastCommit = {message: 'second repository\'s previous commit\'s message'}; - const viewStateForRepo1 = {}; - const viewStateForRepo2 = {}; - - const view = new CommitView({repository: repository1, lastCommit: repository1LastCommit, commandRegistry, stagedChangesExist: true, viewState: viewStateForRepo1}); - const {editor, amend} = view.refs; - - // create message for repository1 - const repository1Message = 'commit message for first repo\nsome details about the commit\nmore details'; - editor.setText(repository1Message); - await etch.getScheduler().getNextUpdatePromise(); - - // put repository1 in amend state, commit message changes to that of the last commit - amend.click(); - await etch.getScheduler().getNextUpdatePromise(); - assert.isTrue(amend.checked); - assert.equal(editor.getText(), repository1LastCommit.message); - - // when repository2 is selected, restore to initial state of unchecked amend box and empty commit message - await view.update({repository: repository2, lastCommit: repository2LastCommit, viewState: viewStateForRepo2}); - assert.isFalse(amend.checked); - assert.equal(editor.getText(), ''); - - // create commit message for repository2 - const repository2Message = 'commit message for second repo'; - editor.setText(repository2Message); - await etch.getScheduler().getNextUpdatePromise(); - assert.isFalse(amend.checked); - assert.equal(editor.getText(), repository2Message); - - // put repository2 in amend state, commit message changes to that of the last commit - amend.click(); - await etch.getScheduler().getNextUpdatePromise(); - assert.isTrue(amend.checked); - assert.equal(editor.getText(), repository2LastCommit.message); - - // when repository1 is selected, restore its state - await view.update({repository: repository1, viewState: viewStateForRepo1}); - assert.isTrue(amend.checked); - assert.equal(editor.getText(), repository1LastCommit.message); - - // exit amend state and restore original message for repository1 - amend.click(); - assert.isFalse(amend.checked); - assert.equal(editor.getText(), repository1Message); - - // when repository2 is selected, restore its state - await view.update({repository: repository2, viewState: viewStateForRepo2}); - assert.isTrue(amend.checked); - assert.equal(editor.getText(), repository2LastCommit.message); - - // exit amend state and restore original message for repository2 - amend.click(); - assert.isFalse(amend.checked); - assert.equal(editor.getText(), repository2Message); - }); - }); }); From 7c3edc57c9661d9e01188b22ae755ef1356cd2e3 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 15 Dec 2016 14:58:42 -0800 Subject: [PATCH 13/16] Create global sinon spy sandbox --- test/controllers/file-patch-controller.test.js | 1 - test/controllers/git-controller.test.js | 1 - test/controllers/git-panel-controller.test.js | 1 - test/controllers/status-bar-tile-controller.test.js | 1 - test/github-package.test.js | 1 - test/helpers.js | 7 +++++++ test/models/file-system-change-observer.test.js | 1 - test/models/model-state-registry.test.js | 2 -- test/models/workspace-change-observer.test.js | 1 - test/multi-list.test.js | 1 - test/views/commit-view.test.js | 1 - test/views/file-patch-view.test.js | 1 - test/views/hunk-view.test.js | 1 - test/views/list-view.test.js | 1 - test/views/pane-item.test.js | 1 - test/views/panel.test.js | 1 - test/views/staging-view.test.js | 1 - 17 files changed, 7 insertions(+), 17 deletions(-) diff --git a/test/controllers/file-patch-controller.test.js b/test/controllers/file-patch-controller.test.js index 6b3e18b119..b1b2019ab1 100644 --- a/test/controllers/file-patch-controller.test.js +++ b/test/controllers/file-patch-controller.test.js @@ -2,7 +2,6 @@ import fs from 'fs'; import path from 'path'; -import sinon from 'sinon'; import {cloneRepository, buildRepository} from '../helpers'; import FilePatch from '../../lib/models/file-patch'; diff --git a/test/controllers/git-controller.test.js b/test/controllers/git-controller.test.js index 52e771dae9..d3ba720094 100644 --- a/test/controllers/git-controller.test.js +++ b/test/controllers/git-controller.test.js @@ -4,7 +4,6 @@ import path from 'path'; import fs from 'fs'; import React from 'react'; -import sinon from 'sinon'; import {shallow} from 'enzyme'; import {cloneRepository, buildRepository} from '../helpers'; diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index 2e398585d6..1fbf931840 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -3,7 +3,6 @@ import fs from 'fs'; import path from 'path'; -import sinon from 'sinon'; import dedent from 'dedent-js'; import {cloneRepository, buildRepository} from '../helpers'; diff --git a/test/controllers/status-bar-tile-controller.test.js b/test/controllers/status-bar-tile-controller.test.js index 18e4482db0..7df9cc18a5 100644 --- a/test/controllers/status-bar-tile-controller.test.js +++ b/test/controllers/status-bar-tile-controller.test.js @@ -4,7 +4,6 @@ import fs from 'fs'; import path from 'path'; import etch from 'etch'; -import sinon from 'sinon'; import {cloneRepository, buildRepository, setUpLocalAndRemoteRepositories} from '../helpers'; import StatusBarTileController from '../../lib/controllers/status-bar-tile-controller'; diff --git a/test/github-package.test.js b/test/github-package.test.js index 034ff089e3..0b430fd622 100644 --- a/test/github-package.test.js +++ b/test/github-package.test.js @@ -5,7 +5,6 @@ import {Directory} from 'atom'; import fs from 'fs'; import path from 'path'; import temp from 'temp'; -import sinon from 'sinon'; import {cloneRepository} from './helpers'; import GithubPackage from '../lib/github-package'; diff --git a/test/helpers.js b/test/helpers.js index b4e0f91bb7..f845dd7e8e 100644 --- a/test/helpers.js +++ b/test/helpers.js @@ -7,6 +7,7 @@ import temp from 'temp'; import {Directory} from 'atom'; import React from 'react'; import ReactDom from 'react-dom'; +import sinon from 'sinon'; import Repository from '../lib/models/repository'; import GitShellOutStrategy from '../lib/git-shell-out-strategy'; @@ -195,8 +196,14 @@ export function createRenderer() { return renderer; } +// eslint-disable-next-line jasmine/no-global-setup +beforeEach(() => { + global.sinon = sinon.sandbox.create(); +}); + // eslint-disable-next-line jasmine/no-global-setup afterEach(() => { activeRenderers.forEach(r => r.unmount()); activeRenderers = []; + global.sinon.restore(); }); diff --git a/test/models/file-system-change-observer.test.js b/test/models/file-system-change-observer.test.js index 11fc83f004..33b3ac5223 100644 --- a/test/models/file-system-change-observer.test.js +++ b/test/models/file-system-change-observer.test.js @@ -2,7 +2,6 @@ import fs from 'fs'; import path from 'path'; -import sinon from 'sinon'; import {cloneRepository, buildRepository, setUpLocalAndRemoteRepositories} from '../helpers'; diff --git a/test/models/model-state-registry.test.js b/test/models/model-state-registry.test.js index e4eb11034c..4d116829c7 100644 --- a/test/models/model-state-registry.test.js +++ b/test/models/model-state-registry.test.js @@ -1,7 +1,5 @@ /** @babel */ -import sinon from 'sinon'; - import ModelStateRegistry from '../../lib/models/model-state-registry'; const Type1 = {type: 1}; diff --git a/test/models/workspace-change-observer.test.js b/test/models/workspace-change-observer.test.js index 8e54dbf062..6b0edccc37 100644 --- a/test/models/workspace-change-observer.test.js +++ b/test/models/workspace-change-observer.test.js @@ -1,7 +1,6 @@ /** @babel */ import path from 'path'; -import sinon from 'sinon'; import {cloneRepository, buildRepository} from '../helpers'; diff --git a/test/multi-list.test.js b/test/multi-list.test.js index 2cd7de8494..fd70ce4d06 100644 --- a/test/multi-list.test.js +++ b/test/multi-list.test.js @@ -1,6 +1,5 @@ /** @babel */ -import sinon from 'sinon'; import MultiList from '../lib/multi-list'; describe('MultiList', () => { diff --git a/test/views/commit-view.test.js b/test/views/commit-view.test.js index 24f3688df6..a3605a4a78 100644 --- a/test/views/commit-view.test.js +++ b/test/views/commit-view.test.js @@ -2,7 +2,6 @@ import {cloneRepository, buildRepository} from '../helpers'; import etch from 'etch'; -import sinon from 'sinon'; import CommitView from '../../lib/views/commit-view'; import {AbortMergeError, CommitError} from '../../lib/models/repository'; diff --git a/test/views/file-patch-view.test.js b/test/views/file-patch-view.test.js index 2b17b67e14..869407fec6 100644 --- a/test/views/file-patch-view.test.js +++ b/test/views/file-patch-view.test.js @@ -1,6 +1,5 @@ /** @babel */ -import sinon from 'sinon'; import FilePatchView from '../../lib/views/file-patch-view'; import Hunk from '../../lib/models/hunk'; diff --git a/test/views/hunk-view.test.js b/test/views/hunk-view.test.js index c8fdbdd441..29139c57f3 100644 --- a/test/views/hunk-view.test.js +++ b/test/views/hunk-view.test.js @@ -1,6 +1,5 @@ /** @babel */ -import sinon from 'sinon'; import Hunk from '../../lib/models/hunk'; import HunkLine from '../../lib/models/hunk-line'; diff --git a/test/views/list-view.test.js b/test/views/list-view.test.js index 7a1cceaeeb..cb71c4ea81 100644 --- a/test/views/list-view.test.js +++ b/test/views/list-view.test.js @@ -3,7 +3,6 @@ /* eslint react/no-unknown-property: "off" */ import etch from 'etch'; -import sinon from 'sinon'; import simulant from 'simulant'; import ListView from '../../lib/views/list-view'; diff --git a/test/views/pane-item.test.js b/test/views/pane-item.test.js index 54e136eb66..05ecbb0dc0 100644 --- a/test/views/pane-item.test.js +++ b/test/views/pane-item.test.js @@ -1,7 +1,6 @@ /** @babel */ import React from 'react'; -import sinon from 'sinon'; import PaneItem from '../../lib/views/pane-item'; diff --git a/test/views/panel.test.js b/test/views/panel.test.js index 338ce898c7..5b5bc7def7 100644 --- a/test/views/panel.test.js +++ b/test/views/panel.test.js @@ -1,7 +1,6 @@ /** @babel */ import React from 'react'; -import sinon from 'sinon'; import Panel from '../../lib/views/panel'; diff --git a/test/views/staging-view.test.js b/test/views/staging-view.test.js index b756d8c98e..7444b1d293 100644 --- a/test/views/staging-view.test.js +++ b/test/views/staging-view.test.js @@ -1,6 +1,5 @@ /** @babel */ -import sinon from 'sinon'; import StagingView from '../../lib/views/staging-view'; import {assertEqualSets} from '../helpers'; From 3db1de20ed47d37fb7db4491ab2a9f24d0d780eb Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 15 Dec 2016 15:02:18 -0800 Subject: [PATCH 14/16] Add 'sinon' global to .eslintrc --- .eslintrc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.eslintrc b/.eslintrc index b720df006e..7da33e7bc0 100644 --- a/.eslintrc +++ b/.eslintrc @@ -34,6 +34,7 @@ "jasmine": true, "expect": true, "spyOn": true, - "waitsFor": true + "waitsFor": true, + "sinon": true, } } From 5cb9c33bda5e08facd58f133bc48e06addf1e05a Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 15 Dec 2016 15:11:13 -0800 Subject: [PATCH 15/16] :white_check_mark: Add tests for GitPanelController and GitController --- lib/controllers/git-controller.js | 2 +- lib/controllers/git-panel-controller.js | 1 + test/controllers/git-controller.test.js | 19 +++++ test/controllers/git-panel-controller.test.js | 80 ++++++++++++++++++- test/views/commit-view.test.js | 53 +----------- 5 files changed, 99 insertions(+), 56 deletions(-) diff --git a/lib/controllers/git-controller.js b/lib/controllers/git-controller.js index 879a5358d7..cd4c6daf82 100644 --- a/lib/controllers/git-controller.js +++ b/lib/controllers/git-controller.js @@ -56,7 +56,7 @@ export default class GitController extends React.Component { return {amending: this.state.amending}; }, restore: (state = {}) => { - this.setState({amending: state.amending}); + this.setState({amending: !!state.amending}); }, }); diff --git a/lib/controllers/git-panel-controller.js b/lib/controllers/git-panel-controller.js index 8836a8cf09..71bc64b45f 100644 --- a/lib/controllers/git-panel-controller.js +++ b/lib/controllers/git-panel-controller.js @@ -90,6 +90,7 @@ export default class GitPanelController { isMerging: await repository.isMerging(), branchName: await repository.getCurrentBranch(), branches: await repository.getBranches(), + mergeMessage: null, aheadCount: null, behindCount: null, remoteName: await repository.getRemoteForBranch(this.branchName), diff --git a/test/controllers/git-controller.test.js b/test/controllers/git-controller.test.js index d3ba720094..3da688d5e5 100644 --- a/test/controllers/git-controller.test.js +++ b/test/controllers/git-controller.test.js @@ -244,4 +244,23 @@ describe('GitController', () => { assert.equal(wrapper.instance().focusGitPanel.callCount, 1); }); }); + + it('correctly updates state when switching repos', async () => { + const workdirPath1 = await cloneRepository('three-files'); + const repository1 = await buildRepository(workdirPath1); + const workdirPath2 = await cloneRepository('three-files'); + const repository2 = await buildRepository(workdirPath2); + + app = React.cloneElement(app, {repository: repository1}); + const wrapper = shallow(app); + + assert.equal(wrapper.state('amending'), false); + + wrapper.setState({amending: true}); + wrapper.setProps({repository: repository2}); + assert.equal(wrapper.state('amending'), false); + + wrapper.setProps({repository: repository1}); + assert.equal(wrapper.state('amending'), true); + }); }); diff --git a/test/controllers/git-panel-controller.test.js b/test/controllers/git-panel-controller.test.js index 1fbf931840..aae00a328f 100644 --- a/test/controllers/git-panel-controller.test.js +++ b/test/controllers/git-panel-controller.test.js @@ -5,16 +5,19 @@ import path from 'path'; import dedent from 'dedent-js'; -import {cloneRepository, buildRepository} from '../helpers'; import GitPanelController from '../../lib/controllers/git-panel-controller'; +import {cloneRepository, buildRepository} from '../helpers'; +import {AbortMergeError, CommitError} from '../../lib/models/repository'; + describe('GitPanelController', () => { - let atomEnvironment, workspace, commandRegistry; + let atomEnvironment, workspace, commandRegistry, notificationManager; beforeEach(() => { atomEnvironment = global.buildAtomEnvironment(); workspace = atomEnvironment.workspace; commandRegistry = atomEnvironment.commands; + notificationManager = atomEnvironment.notifications; }); afterEach(() => { @@ -39,7 +42,7 @@ describe('GitPanelController', () => { assert.isDefined(controller.refs.gitPanel.refs.repoInfo); }); - it('keeps the state of the GitPanelView in sync with the assigned repository', async done => { + it('keeps the state of the GitPanelView in sync with the assigned repository', async () => { const workdirPath1 = await cloneRepository('three-files'); const repository1 = await buildRepository(workdirPath1); const workdirPath2 = await cloneRepository('three-files'); @@ -92,6 +95,77 @@ describe('GitPanelController', () => { assert.equal(didChangeAmending.callCount, 2); }); + describe('abortMerge()', () => { + it('shows an error notification when abortMerge() throws an EDIRTYSTAGED exception', async () => { + const workdirPath = await cloneRepository('three-files'); + const repository = await buildRepository(workdirPath); + sinon.stub(repository, 'abortMerge', async () => { + await Promise.resolve(); + throw new AbortMergeError('EDIRTYSTAGED', 'a.txt'); + }); + + const controller = new GitPanelController({workspace, commandRegistry, notificationManager, repository}); + assert.equal(notificationManager.getNotifications().length, 0); + sinon.stub(atom, 'confirm').returns(0); + await controller.abortMerge(); + assert.equal(notificationManager.getNotifications().length, 1); + }); + + it('resets merge related state', async () => { + const workdirPath = await cloneRepository('merge-conflict'); + const repository = await buildRepository(workdirPath); + + await repository.git.merge('origin/branch') + .then(() => { throw new Error('Expected merge to throw an error'); }) + .catch(() => true); + + const controller = new GitPanelController({workspace, commandRegistry, repository}); + await controller.getLastModelDataRefreshPromise(); + let modelData = controller.repositoryObserver.getActiveModelData(); + + assert.notEqual(modelData.mergeConflicts.length, 0); + assert.isTrue(modelData.isMerging); + assert.isOk(modelData.mergeMessage); + + sinon.stub(atom, 'confirm').returns(0); + await controller.abortMerge(); + await controller.getLastModelDataRefreshPromise(); + modelData = controller.repositoryObserver.getActiveModelData(); + + assert.equal(modelData.mergeConflicts.length, 0); + assert.isFalse(modelData.isMerging); + assert.isNull(modelData.mergeMessage); + }); + }); + + describe('commit(message)', () => { + it('shows an error notification when committing throws an ECONFLICT exception', async () => { + const workdirPath = await cloneRepository('three-files'); + const repository = await buildRepository(workdirPath); + sinon.stub(repository, 'commit', async () => { + await Promise.resolve(); + throw new CommitError('ECONFLICT'); + }); + + const controller = new GitPanelController({workspace, commandRegistry, notificationManager, repository}); + assert.equal(notificationManager.getNotifications().length, 0); + await controller.commit(); + assert.equal(notificationManager.getNotifications().length, 1); + }); + + it('sets amending to false', async () => { + const workdirPath = await cloneRepository('three-files'); + const repository = await buildRepository(workdirPath); + sinon.stub(repository, 'commit', () => Promise.resolve()); + const didChangeAmending = sinon.stub(); + const controller = new GitPanelController({workspace, commandRegistry, repository, didChangeAmending}); + + await controller.commit('message'); + assert.equal(didChangeAmending.callCount, 1); + }); + }); + + describe('integration tests', () => { it('can stage and unstage files and commit', async () => { const workdirPath = await cloneRepository('three-files'); diff --git a/test/views/commit-view.test.js b/test/views/commit-view.test.js index a3605a4a78..1ec0269740 100644 --- a/test/views/commit-view.test.js +++ b/test/views/commit-view.test.js @@ -4,10 +4,9 @@ import {cloneRepository, buildRepository} from '../helpers'; import etch from 'etch'; import CommitView from '../../lib/views/commit-view'; -import {AbortMergeError, CommitError} from '../../lib/models/repository'; describe('CommitView', () => { - let atomEnv, commandRegistry, notificationManager; + let atomEnv, commandRegistry; beforeEach(() => { atomEnv = global.buildAtomEnvironment(); @@ -136,24 +135,6 @@ describe('CommitView', () => { assert.equal(commit.callCount, 0); }); - // FIXME: move to git panel controller - xit('shows an error notification when props.commit() throws an ECONFLICT exception', async () => { - const commit = sinon.spy(async () => { - await Promise.resolve(); - throw new CommitError('ECONFLICT'); - }); - const view = new CommitView({commandRegistry, notificationManager, stagedChangesExist: true, commit}); - const {editor, commitButton} = view.refs; - editor.setText('A message.'); - await etch.getScheduler().getNextUpdatePromise(); - assert.equal(notificationManager.getNotifications().length, 0); - commitButton.dispatchEvent(new MouseEvent('click')); - await etch.getScheduler().getNextUpdatePromise(); - assert(commit.calledOnce); - assert.equal(editor.getText(), 'A message.'); - assert.equal(notificationManager.getNotifications().length, 1); - }); - it('shows the "Abort Merge" button when props.isMerging is true', async () => { const view = new CommitView({commandRegistry, stagedChangesExist: true, isMerging: false}); const {abortMergeButton} = view.refs; @@ -166,7 +147,6 @@ describe('CommitView', () => { assert.equal(abortMergeButton.style.display, 'none'); }); - // FIXME: we should test elsewhere that this clears the mergeMessage prop to commitViewController it('calls props.abortMerge() when the "Abort Merge" button is clicked', () => { const abortMerge = sinon.spy(() => Promise.resolve()); const view = new CommitView({commandRegistry, stagedChangesExist: true, isMerging: true, abortMerge}); @@ -175,38 +155,7 @@ describe('CommitView', () => { assert(abortMerge.calledOnce); }); - // FIXME: this needs to go elsewhere, e.g. gitPanelController - xit('shows an error notification when props.abortMerge() throws an EDIRTYSTAGED exception', async () => { - const abortMerge = sinon.spy(async () => { - await Promise.resolve(); - throw new AbortMergeError('EDIRTYSTAGED', 'a.txt'); - }); - const view = new CommitView({commandRegistry, notificationManager, stagedChangesExist: true, isMerging: true, abortMerge}); - const {editor, abortMergeButton} = view.refs; - editor.setText('A message.'); - assert.equal(notificationManager.getNotifications().length, 0); - abortMergeButton.dispatchEvent(new MouseEvent('click')); - await etch.getScheduler().getNextUpdatePromise(); - assert(abortMerge.calledOnce); - assert.equal(editor.getText(), 'A message.'); - assert.equal(notificationManager.getNotifications().length, 1); - }); - describe('amending', () => { - // FIXME: move - xit('clears the amend checkbox after committing', async () => { - const workdirPath = await cloneRepository('three-files'); - const repository = await buildRepository(workdirPath); - const view = new CommitView({commandRegistry, stagedChangesExist: false}); - const {amend} = view.refs; - await view.update({repository, stagedChangesExist: true}); - assert.isFalse(amend.checked); - amend.click(); - assert.isTrue(amend.checked); - await view.commit(); - assert.isFalse(amend.checked); - }); - it('calls props.setAmending() when the box is checked or unchecked', () => { const setAmending = sinon.spy(); const view = new CommitView({commandRegistry, stagedChangesExist: false, lastCommit: {message: 'previous commit\'s message'}, setAmending}); From e5f33f41e9a67360c9cd910fe3c4fbc5b4fed857 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Thu, 15 Dec 2016 15:29:27 -0800 Subject: [PATCH 16/16] Fix edge cases around ModelStateRegistry when mounting/unmounting --- lib/controllers/commit-view-controller.js | 2 +- lib/controllers/git-controller.js | 7 +++++-- lib/models/model-state-registry.js | 18 +++++++++++++----- test/models/model-state-registry.test.js | 18 ++++++++++++++++++ 4 files changed, 37 insertions(+), 8 deletions(-) diff --git a/lib/controllers/commit-view-controller.js b/lib/controllers/commit-view-controller.js index 4baedc3333..3265c4772b 100644 --- a/lib/controllers/commit-view-controller.js +++ b/lib/controllers/commit-view-controller.js @@ -83,7 +83,7 @@ export default class CommitViewController { } destroy() { - this.repoStateRegistry.setModel(null); + this.repoStateRegistry.save(); return etch.destroy(this); } } diff --git a/lib/controllers/git-controller.js b/lib/controllers/git-controller.js index cd4c6daf82..6495ac4b89 100644 --- a/lib/controllers/git-controller.js +++ b/lib/controllers/git-controller.js @@ -51,7 +51,6 @@ export default class GitController extends React.Component { }; this.repositoryStateRegistry = new ModelStateRegistry(GitController, { - initialModel: props.repository, save: () => { return {amending: this.state.amending}; }, @@ -85,6 +84,10 @@ export default class GitController extends React.Component { ); } + componentWillMount() { + this.repositoryStateRegistry.setModel(this.props.repository); + } + componentWillReceiveProps(newProps) { this.repositoryObserver.setActiveModel(newProps.repository); this.repositoryStateRegistry.setModel(newProps.repository); @@ -159,7 +162,7 @@ export default class GitController extends React.Component { } componentWillUnmount() { - this.repositoryStateRegistry.setModel(null); + this.repositoryStateRegistry.save(); this.subscriptions.dispose(); } diff --git a/lib/models/model-state-registry.js b/lib/models/model-state-registry.js index 3cc2b1842d..1698d0a84c 100644 --- a/lib/models/model-state-registry.js +++ b/lib/models/model-state-registry.js @@ -8,8 +8,8 @@ export default class { } constructor(type, {initialModel, save, restore}) { - this.save = save; - this.restore = restore; + this.saveData = save; + this.restoreData = restore; if (statePerType.has(type)) { this.statePerModel = statePerType.get(type); } else { @@ -21,9 +21,17 @@ export default class { setModel(model) { if (model !== this.model) { - this.model && this.statePerModel.set(this.model, this.save()); - model && this.restore(this.statePerModel.get(model)); - this.model = model; + this.save(); + this.restore(model); } } + + save() { + this.model && this.statePerModel.set(this.model, this.saveData()); + } + + restore(model) { + model && this.restoreData(this.statePerModel.get(model)); + this.model = model; + } } diff --git a/test/models/model-state-registry.test.js b/test/models/model-state-registry.test.js index 4d116829c7..0d83ca3f8d 100644 --- a/test/models/model-state-registry.test.js +++ b/test/models/model-state-registry.test.js @@ -117,4 +117,22 @@ describe('ModelStateRegistry', () => { registry2.setModel(model1); assert.deepEqual(data2, {evenMore: 'data'}); }); + + describe('#save and #restore', () => { + it('manually saves and restores data', () => { + let data; + const registry = new ModelStateRegistry(Type1, { + initialModel: model1, + save: () => data, + restore: (saved = {}) => { data = saved; }, + }); + data = {some: 'data'}; + registry.save(); + data = {}; + registry.restore(model2); + assert.deepEqual(data, {}); + registry.restore(model1); + assert.deepEqual(data, {some: 'data'}); + }); + }); });