From cfcd149285359173cfb09cdf606cafe418f3307b Mon Sep 17 00:00:00 2001 From: Michelle Tilley Date: Mon, 1 May 2017 17:20:14 -0700 Subject: [PATCH 1/7] Change Repository#observeFilesystemChange to take paths instead of events --- lib/models/repository-states/present.js | 8 +++----- lib/models/workdir-context.js | 7 ++++++- test/models/repository.test.js | 3 ++- test/models/workdir-context.test.js | 7 ++++--- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/lib/models/repository-states/present.js b/lib/models/repository-states/present.js index b6fbd3aebb..a7953bba43 100644 --- a/lib/models/repository-states/present.js +++ b/lib/models/repository-states/present.js @@ -69,12 +69,10 @@ export default class Present extends State { this.didUpdate(); } - observeFilesystemChange(events) { + observeFilesystemChange(paths) { const keys = new Set(); - for (let i = 0; i < events.length; i++) { - const event = events[i]; - - const fullPath = path.join(event.directory, event.file || event.newFile); + for (let i = 0; i < paths.length; i++) { + const fullPath = paths[i]; const endsWith = (...segments) => fullPath.endsWith(path.join(...segments)); const includes = (...segments) => fullPath.includes(path.join(...segments)); diff --git a/lib/models/workdir-context.js b/lib/models/workdir-context.js index 22dda9d829..efbbfba1dd 100644 --- a/lib/models/workdir-context.js +++ b/lib/models/workdir-context.js @@ -1,3 +1,5 @@ +import path from 'path'; + import {Emitter, CompositeDisposable} from 'event-kit'; import {autobind} from 'core-decorators'; @@ -45,7 +47,10 @@ export default class WorkdirContext { // Wire up event forwarding among models this.subs.add(this.repository.onDidChangeState(this.repositoryChangedState)); - this.subs.add(this.observer.onDidChange(payload => this.repository.observeFilesystemChange(payload))); + this.subs.add(this.observer.onDidChange(events => { + const paths = events.map(e => path.join(e.directory, e.file || e.newFile)); + this.repository.observeFilesystemChange(paths); + })); this.subs.add(this.observer.onDidChangeWorkdirOrHead(() => this.emitter.emit('did-change-workdir-or-head'))); // If a pre-loaded Repository was provided, broadcast an initial state change event. diff --git a/test/models/repository.test.js b/test/models/repository.test.js index decc4f3c4b..1bf44272db 100644 --- a/test/models/repository.test.js +++ b/test/models/repository.test.js @@ -1263,7 +1263,8 @@ describe('Repository', function() { ); sub.add(observer.onDidChange(events => { - repository.observeFilesystemChange(events); + const paths = events.map(e => path.join(e.directory, e.file || e.newFile)); + repository.observeFilesystemChange(paths); observedEvents.push(...events); eventCallback(); })); diff --git a/test/models/workdir-context.test.js b/test/models/workdir-context.test.js index 2042bfe548..5275d4cb98 100644 --- a/test/models/workdir-context.test.js +++ b/test/models/workdir-context.test.js @@ -1,3 +1,5 @@ +import path from 'path'; + import {CompositeDisposable, Disposable} from 'event-kit'; import {cloneRepository} from '../helpers'; @@ -62,10 +64,9 @@ describe('WorkdirContext', function() { sinon.spy(repo, 'observeFilesystemChange'); - const payload = {}; - context.getChangeObserver().didChange(payload); + context.getChangeObserver().didChange([{directory: '/a/b', file: 'c'}]); - assert.isTrue(repo.observeFilesystemChange.calledWith(payload)); + assert.isTrue(repo.observeFilesystemChange.calledWith([path.join('/a/b', 'c')])); }); it('re-emits an event on workdir or head change', async function() { From fcaf032c3e5525547a8990032625c23b0544fceb Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 2 May 2017 15:50:43 -0700 Subject: [PATCH 2/7] Implement DeferredCallbackQueue ...to minimize index lock contention while Atom updates in the background --- lib/deferred-callback-queue.js | 72 ++++++++++++++++++++++++++++ lib/models/workdir-context.js | 9 +++- test/deferred-callback-queue.test.js | 54 +++++++++++++++++++++ 3 files changed, 133 insertions(+), 2 deletions(-) create mode 100644 lib/deferred-callback-queue.js create mode 100644 test/deferred-callback-queue.test.js diff --git a/lib/deferred-callback-queue.js b/lib/deferred-callback-queue.js new file mode 100644 index 0000000000..5ce49aa069 --- /dev/null +++ b/lib/deferred-callback-queue.js @@ -0,0 +1,72 @@ +import {remote} from 'electron'; +import {CompositeDisposable, Disposable} from 'event-kit'; +import {autobind} from 'core-decorators'; + +export default class DeferredCallbackQueue { + constructor(wait, callback, options = {}) { + this.wait = wait; + const currentWindow = remote.getCurrentWindow(); + + const onDidFocus = options.onDidFocus || function(cb) { + currentWindow.on('focus', cb); + return new Disposable(() => currentWindow.removeListener('focus', cb)); + }; + const onDidBlur = options.onDidBlur || function(cb) { + currentWindow.on('blur', cb); + return new Disposable(() => currentWindow.removeListener('blur', cb)); + }; + + this.callback = callback; + this.subscriptions = new CompositeDisposable(); + this.items = new Set(); + + this.paused = !currentWindow.isFocused(); + this.subscriptions.add(onDidFocus(this.resume)); + this.subscriptions.add(onDidBlur(this.pause)); + } + + @autobind + pause() { + if (this.paused) { return; } + this.paused = true; + } + + @autobind + resume() { + if (!this.paused) { return; } + this.paused = false; + if (this.timer) { + clearTimeout(this.timer); + } + this.flush(); + } + + resetTimer() { + if (this.timer) { + clearTimeout(this.timer); + } + this.timer = setTimeout(this.flush, this.wait); + } + + @autobind + flush() { + delete this.timer; + if (this.items.size) { + this.callback([...this.items]); + this.items.clear(); + } + } + + push(...items) { + if (this.paused) { + items.forEach(item => this.items.add(item)); + this.resetTimer(); + } else { + this.callback(items); + } + } + + destroy() { + this.subscriptions.dispose(); + } +} diff --git a/lib/models/workdir-context.js b/lib/models/workdir-context.js index efbbfba1dd..9b7ee00222 100644 --- a/lib/models/workdir-context.js +++ b/lib/models/workdir-context.js @@ -1,8 +1,9 @@ import path from 'path'; -import {Emitter, CompositeDisposable} from 'event-kit'; +import {Emitter, CompositeDisposable, Disposable} from 'event-kit'; import {autobind} from 'core-decorators'; +import DeferredCallbackQueue from '../deferred-callback-queue'; import Repository from './repository'; import ResolutionProgress from './conflicts/resolution-progress'; import FileSystemChangeObserver from './file-system-change-observer'; @@ -40,6 +41,9 @@ export default class WorkdirContext { ? new WorkspaceChangeObserver(theWindow, workspace, this.repository) : new FileSystemChangeObserver(this.repository); this.resolutionProgress = new ResolutionProgress(); + this.deferredCallbackQueue = new DeferredCallbackQueue(3000, collection => { + this.repository.observeFilesystemChange(collection); + }); if (promptCallback) { this.repository.setPromptCallback(promptCallback); @@ -49,9 +53,10 @@ export default class WorkdirContext { this.subs.add(this.repository.onDidChangeState(this.repositoryChangedState)); this.subs.add(this.observer.onDidChange(events => { const paths = events.map(e => path.join(e.directory, e.file || e.newFile)); - this.repository.observeFilesystemChange(paths); + this.deferredCallbackQueue.push(...paths); })); this.subs.add(this.observer.onDidChangeWorkdirOrHead(() => this.emitter.emit('did-change-workdir-or-head'))); + this.subs.add(new Disposable(() => this.deferredCallbackQueue.destroy())); // If a pre-loaded Repository was provided, broadcast an initial state change event. this.repositoryChangedState({from: null, to: this.repository.state}); diff --git a/test/deferred-callback-queue.test.js b/test/deferred-callback-queue.test.js new file mode 100644 index 0000000000..e127feb0e6 --- /dev/null +++ b/test/deferred-callback-queue.test.js @@ -0,0 +1,54 @@ +import DeferredCallbackQueue from '../lib/deferred-callback-queue'; +import {Emitter} from 'event-kit'; + +describe('DeferredCallbackQueue', function() { + let callbackSpy, emitter, queue; + beforeEach(() => { + callbackSpy = sinon.spy(); + emitter = new Emitter(); + queue = new DeferredCallbackQueue(20, callbackSpy, { + onDidFocus: cb => { + return emitter.on('focus', cb); + }, + onDidBlur: cb => { + return emitter.on('blur', cb); + }, + }); + }); + + describe('when focused', function() { + it('calls the specified callback immediately', function() { + emitter.emit('focus'); + queue.push('a', 'b', 'c'); + assert.deepEqual(callbackSpy.args[0][0], ['a', 'b', 'c']); + }); + }); + + describe('when blurred', function() { + it('calls the specified callback after a specified quiet time', async function() { + emitter.emit('blur'); + queue.push(1); + await new Promise(res => setTimeout(res, 10)); + queue.push(2); + await new Promise(res => setTimeout(res, 10)); + queue.push(3); + await new Promise(res => setTimeout(res, 10)); + assert.isFalse(callbackSpy.called); + await new Promise(res => setTimeout(res, 20)); + assert.deepEqual(callbackSpy.args[0][0], [1, 2, 3]); + }); + + it('calls the specified callback immediately when focus is regained', async function() { + emitter.emit('blur'); + queue.push(4); + await new Promise(res => setTimeout(res, 10)); + queue.push(5); + await new Promise(res => setTimeout(res, 10)); + queue.push(6); + await new Promise(res => setTimeout(res, 10)); + assert.isFalse(callbackSpy.called); + emitter.emit('focus'); + assert.deepEqual(callbackSpy.args[0][0], [4, 5, 6]); + }); + }); +}); From 068df94fd020e4b53e148dee9e5a72e594c10697 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 2 May 2017 17:01:43 -0700 Subject: [PATCH 3/7] Don't pause in spec mode as default --- lib/deferred-callback-queue.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/deferred-callback-queue.js b/lib/deferred-callback-queue.js index 5ce49aa069..44bd3b02d3 100644 --- a/lib/deferred-callback-queue.js +++ b/lib/deferred-callback-queue.js @@ -4,6 +4,7 @@ import {autobind} from 'core-decorators'; export default class DeferredCallbackQueue { constructor(wait, callback, options = {}) { + console.log('create DeferredCallbackQueue'); this.wait = wait; const currentWindow = remote.getCurrentWindow(); @@ -12,6 +13,9 @@ export default class DeferredCallbackQueue { return new Disposable(() => currentWindow.removeListener('focus', cb)); }; const onDidBlur = options.onDidBlur || function(cb) { + if (atom.inSpecMode()) { + return new Disposable(); + } currentWindow.on('blur', cb); return new Disposable(() => currentWindow.removeListener('blur', cb)); }; @@ -20,7 +24,7 @@ export default class DeferredCallbackQueue { this.subscriptions = new CompositeDisposable(); this.items = new Set(); - this.paused = !currentWindow.isFocused(); + this.paused = !currentWindow.isFocused() || !atom.inSpecMode(); this.subscriptions.add(onDidFocus(this.resume)); this.subscriptions.add(onDidBlur(this.pause)); } @@ -28,6 +32,7 @@ export default class DeferredCallbackQueue { @autobind pause() { if (this.paused) { return; } + console.log('paused'); this.paused = true; } @@ -50,6 +55,7 @@ export default class DeferredCallbackQueue { @autobind flush() { + console.warn('FLUSH'); delete this.timer; if (this.items.size) { this.callback([...this.items]); From a3c0b2235fce8f614912cdfce882803fcd41d909 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 2 May 2017 18:22:43 -0700 Subject: [PATCH 4/7] Fallback to sha when describing HEAD in GSOS#getCurrentBranch --- lib/git-shell-out-strategy.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/git-shell-out-strategy.js b/lib/git-shell-out-strategy.js index 9e4b342d89..914bb46061 100644 --- a/lib/git-shell-out-strategy.js +++ b/lib/git-shell-out-strategy.js @@ -523,7 +523,7 @@ export default class GitShellOutStrategy { // Detached HEAD return { - name: (await this.exec(['describe', '--contains', '--all', 'HEAD'])).trim(), + name: (await this.exec(['describe', '--contains', '--all', '--always', 'HEAD'])).trim(), isDetached: true, }; } From ac4ac1a8d0c7d792d985656c5e89b16eeb440b3a Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 2 May 2017 18:40:43 -0700 Subject: [PATCH 5/7] :fire: console.logs --- lib/deferred-callback-queue.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/deferred-callback-queue.js b/lib/deferred-callback-queue.js index 44bd3b02d3..145b5a86ee 100644 --- a/lib/deferred-callback-queue.js +++ b/lib/deferred-callback-queue.js @@ -4,7 +4,6 @@ import {autobind} from 'core-decorators'; export default class DeferredCallbackQueue { constructor(wait, callback, options = {}) { - console.log('create DeferredCallbackQueue'); this.wait = wait; const currentWindow = remote.getCurrentWindow(); @@ -32,13 +31,13 @@ export default class DeferredCallbackQueue { @autobind pause() { if (this.paused) { return; } - console.log('paused'); this.paused = true; } @autobind resume() { if (!this.paused) { return; } + console.log('resume'); this.paused = false; if (this.timer) { clearTimeout(this.timer); From e4aa4d77974d47c62ada8577784ac1b840c720c4 Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 2 May 2017 18:51:26 -0700 Subject: [PATCH 6/7] :fire: logs --- lib/deferred-callback-queue.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/lib/deferred-callback-queue.js b/lib/deferred-callback-queue.js index 145b5a86ee..b59d107738 100644 --- a/lib/deferred-callback-queue.js +++ b/lib/deferred-callback-queue.js @@ -37,7 +37,6 @@ export default class DeferredCallbackQueue { @autobind resume() { if (!this.paused) { return; } - console.log('resume'); this.paused = false; if (this.timer) { clearTimeout(this.timer); @@ -54,7 +53,6 @@ export default class DeferredCallbackQueue { @autobind flush() { - console.warn('FLUSH'); delete this.timer; if (this.items.size) { this.callback([...this.items]); From 72fa76c73559422be69c1f86654cf20c2f23f71d Mon Sep 17 00:00:00 2001 From: Katrina Uychaco Date: Tue, 2 May 2017 18:53:24 -0700 Subject: [PATCH 7/7] Oops fix boolean logic --- lib/deferred-callback-queue.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/deferred-callback-queue.js b/lib/deferred-callback-queue.js index b59d107738..c96c8d4f70 100644 --- a/lib/deferred-callback-queue.js +++ b/lib/deferred-callback-queue.js @@ -23,7 +23,7 @@ export default class DeferredCallbackQueue { this.subscriptions = new CompositeDisposable(); this.items = new Set(); - this.paused = !currentWindow.isFocused() || !atom.inSpecMode(); + this.paused = !currentWindow.isFocused() && !atom.inSpecMode(); this.subscriptions.add(onDidFocus(this.resume)); this.subscriptions.add(onDidBlur(this.pause)); }