From c632f5012f7db1af9d1ed2cfb58b7b88ea33b7a5 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 22 Nov 2016 17:31:21 -0800 Subject: [PATCH] Debounce showing FilePatch when navigating with keyboard Include option for debounce wait time --- lib/views/staging-view.js | 32 ++++++-- package.json | 7 ++ test/views/staging-view.test.js | 126 +++++++++++++++++++++++--------- 3 files changed, 122 insertions(+), 43 deletions(-) diff --git a/lib/views/staging-view.js b/lib/views/staging-view.js index 1e378bf2e9..21e602e4ad 100644 --- a/lib/views/staging-view.js +++ b/lib/views/staging-view.js @@ -9,9 +9,28 @@ import MergeConflictListItemView from './merge-conflict-list-item-view' import CompositeListSelection from './composite-list-selection' import {shortenSha} from '../helpers' +const debounce = (fn, wait) => { + let timeout + return (...args) => { + return new Promise(resolve => { + clearTimeout(timeout) + timeout = setTimeout(() => { + resolve(fn(...args)) + }, wait) + }) + } +} + export default class StagingView { constructor (props) { this.props = props + atom.config.observe("github.keyboardNavigationDelay", (value) => { + if (value === 0) { + this.debouncedDidChangeSelectedItem = this.didChangeSelectedItem.bind(this) + } else { + this.debouncedDidChangeSelectedItem = debounce(this.didChangeSelectedItem.bind(this), value) + } + }) this.mouseSelectionInProgress = false this.listElementsByItem = new WeakMap() this.registerItemElement = this.registerItemElement.bind(this) @@ -26,7 +45,6 @@ export default class StagingView { }, idForItem: item => item.filePath }) - this.onDidChangeSelectedItem() etch.initialize(this) this.subscriptions = new CompositeDisposable() @@ -81,35 +99,34 @@ export default class StagingView { selectPrevious (preserveTail = false) { this.selection.selectPreviousItem(preserveTail) this.selection.coalesce() - this.onDidChangeSelectedItem() + if (!preserveTail) this.debouncedDidChangeSelectedItem() return etch.update(this) } selectNext (preserveTail = false) { this.selection.selectNextItem(preserveTail) this.selection.coalesce() - this.onDidChangeSelectedItem() + if (!preserveTail) this.debouncedDidChangeSelectedItem() return etch.update(this) } selectAll () { this.selection.selectAllItems() this.selection.coalesce() - this.onDidChangeSelectedItem() return etch.update(this) } selectFirst (preserveTail = false) { this.selection.selectFirstItem(preserveTail) this.selection.coalesce() - this.onDidChangeSelectedItem() + if (!preserveTail) this.debouncedDidChangeSelectedItem() return etch.update(this) } selectLast (preserveTail = false) { this.selection.selectLastItem(preserveTail) this.selection.coalesce() - this.onDidChangeSelectedItem() + if (!preserveTail) this.debouncedDidChangeSelectedItem() return etch.update(this) } @@ -118,7 +135,7 @@ export default class StagingView { if (headItem) this.listElementsByItem.get(headItem).scrollIntoViewIfNeeded() } - onDidChangeSelectedItem () { + didChangeSelectedItem () { const selectedItem = this.selection.getHeadItem() if (!this.isFocused() || !selectedItem) return @@ -271,7 +288,6 @@ export default class StagingView { focus () { this.element.focus() - this.onDidChangeSelectedItem() } isFocused () { diff --git a/package.json b/package.json index 5c3007e7d4..7de379378e 100644 --- a/package.json +++ b/package.json @@ -53,5 +53,12 @@ "1.1.0": "consumeStatusBar_1_1" } } + }, + "configSchema": { + "keyboardNavigationDelay": { + "type": "number", + "default": 300, + "description": "How long to wait before showing a diff view after navigating to an entry with the keyboard" + } } } diff --git a/test/views/staging-view.test.js b/test/views/staging-view.test.js index 66c0bc6544..234ff7e9bb 100644 --- a/test/views/staging-view.test.js +++ b/test/views/staging-view.test.js @@ -68,45 +68,101 @@ describe('StagingView', () => { }) describe('when the selection changes', function () { - it('notifies the parent component via the appropriate callback', async function () { - const filePatches = [ - { filePath: 'a.txt', status: 'modified' }, - { filePath: 'b.txt', status: 'deleted' } - ] - const mergeConflicts = [{ - filePath: 'conflicted-path', - status: { - file: 'modified', - ours: 'deleted', - theirs: 'modified' - } - }] - - const didSelectFilePath = sinon.spy() - const didSelectMergeConflictFile = sinon.spy() - - const view = new StagingView({ - didSelectFilePath, didSelectMergeConflictFile, - unstagedChanges: filePatches, mergeConflicts, stagedChanges: [] + describe("when github.keyboardNavigationDelay is 0", () => { + beforeEach(() => { + atom.config.set("github.keyboardNavigationDelay", 0) }) - document.body.appendChild(view.element) - assert.equal(didSelectFilePath.callCount, 0) - view.focus() - assert.isTrue(didSelectFilePath.calledWith(filePatches[0].filePath)) - await view.selectNext() - assert.isTrue(didSelectFilePath.calledWith(filePatches[1].filePath)) - await view.selectNext() - assert.isTrue(didSelectMergeConflictFile.calledWith(mergeConflicts[0].filePath)) + it('synchronously notifies the parent component via the appropriate callback', async function () { + const filePatches = [ + { filePath: 'a.txt', status: 'modified' }, + { filePath: 'b.txt', status: 'deleted' } + ] + const mergeConflicts = [{ + filePath: 'conflicted-path', + status: { + file: 'modified', + ours: 'deleted', + theirs: 'modified' + } + }] + + const didSelectFilePath = sinon.spy() + const didSelectMergeConflictFile = sinon.spy() + + const view = new StagingView({ + didSelectFilePath, didSelectMergeConflictFile, + unstagedChanges: filePatches, mergeConflicts, stagedChanges: [] + }) + document.body.appendChild(view.element) + assert.equal(didSelectFilePath.callCount, 0) + + view.focus() + await view.selectNext() + assert.isTrue(didSelectFilePath.calledWith(filePatches[1].filePath)) + await view.selectNext() + assert.isTrue(didSelectMergeConflictFile.calledWith(mergeConflicts[0].filePath)) + + document.body.focus() + assert.isFalse(view.isFocused()) + didSelectFilePath.reset() + didSelectMergeConflictFile.reset() + await view.selectNext() + assert.equal(didSelectMergeConflictFile.callCount, 0) + + view.element.remove() + }) + }) - document.body.focus() - assert.isFalse(view.isFocused()) - didSelectFilePath.reset() - didSelectMergeConflictFile.reset() - await view.selectNext() - assert.equal(didSelectMergeConflictFile.callCount, 0) + describe("when github.keyboardNavigationDelay is greater than 0", () => { + beforeEach(() => { + atom.config.set("github.keyboardNavigationDelay", 50) + }) - view.element.remove() + it('asynchronously notifies the parent component via the appropriate callback', async function () { + const filePatches = [ + { filePath: 'a.txt', status: 'modified' }, + { filePath: 'b.txt', status: 'deleted' } + ] + const mergeConflicts = [{ + filePath: 'conflicted-path', + status: { + file: 'modified', + ours: 'deleted', + theirs: 'modified' + } + }] + + const didSelectFilePath = sinon.spy() + const didSelectMergeConflictFile = sinon.spy() + + const view = new StagingView({ + didSelectFilePath, didSelectMergeConflictFile, + unstagedChanges: filePatches, mergeConflicts, stagedChanges: [] + }) + document.body.appendChild(view.element) + assert.equal(didSelectFilePath.callCount, 0) + + view.focus() + await view.selectNext() + assert.isFalse(didSelectFilePath.calledWith(filePatches[1].filePath)) + await new Promise(resolve => setTimeout(resolve, 100)) + assert.isTrue(didSelectFilePath.calledWith(filePatches[1].filePath)) + await view.selectNext() + assert.isFalse(didSelectMergeConflictFile.calledWith(mergeConflicts[0].filePath)) + await new Promise(resolve => setTimeout(resolve, 100)) + assert.isTrue(didSelectMergeConflictFile.calledWith(mergeConflicts[0].filePath)) + + document.body.focus() + assert.isFalse(view.isFocused()) + didSelectFilePath.reset() + didSelectMergeConflictFile.reset() + await view.selectNext() + await new Promise(resolve => setTimeout(resolve, 100)) + assert.equal(didSelectMergeConflictFile.callCount, 0) + + view.element.remove() + }) }) it('autoscroll to the selected item if it is out of view', async function () {