From 61a52d49aae7401e303f1a54e85263f744fb14c8 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Wed, 31 May 2017 14:06:46 -0400 Subject: [PATCH 01/18] Allow text editors in the workspace center, but not in docks --- src/text-editor.coffee | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/text-editor.coffee b/src/text-editor.coffee index dae7e069885..302a2d88bf4 100644 --- a/src/text-editor.coffee +++ b/src/text-editor.coffee @@ -3627,6 +3627,9 @@ class TextEditor extends Model }) @component.element + getAllowedLocations: -> + ['center'] + # Essential: Retrieves the greyed out placeholder of a mini editor. # # Returns a {String}. From cf50625cc6673313952c12ec7b24f78974e10ffe Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Wed, 31 May 2017 14:32:20 -0400 Subject: [PATCH 02/18] Teach Workspace::getActiveTextEditor() to get item from center --- spec/workspace-spec.js | 36 ++++++++++++++++++++++++++++++++++++ src/workspace.js | 8 ++++---- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/spec/workspace-spec.js b/spec/workspace-spec.js index 9eaa17bcddd..98b448c2bdf 100644 --- a/spec/workspace-spec.js +++ b/spec/workspace-spec.js @@ -1419,6 +1419,42 @@ describe('Workspace', () => { }) }) + describe('::getActiveTextEditor()', () => { + describe("when the workspace center's active pane item is a text editor", () => { + describe('when the workspace center has focus', function () { + it('returns the text editor', () => { + const workspaceCenter = workspace.getCenter() + const editor = new TextEditor() + workspaceCenter.getActivePane().activateItem(editor) + workspaceCenter.activate() + + expect(workspace.getActiveTextEditor()).toBe(editor) + }) + }) + + describe('when a dock has focus', function () { + it('returns the text editor', () => { + const workspaceCenter = workspace.getCenter() + const editor = new TextEditor() + workspaceCenter.getActivePane().activateItem(editor) + workspace.getLeftDock().activate() + + expect(workspace.getActiveTextEditor()).toBe(editor) + }) + }) + }) + + describe("when the workspace center's active pane item is not a text editor", () => { + it('returns undefined', () => { + const workspaceCenter = workspace.getCenter() + const nonEditorItem = document.createElement('div') + workspaceCenter.getActivePane().activateItem(nonEditorItem) + + expect(workspace.getActiveTextEditor()).toBeUndefined() + }) + }) + }) + describe('::observeTextEditors()', () => { it('invokes the observer with current and future text editors', () => { const observed = [] diff --git a/src/workspace.js b/src/workspace.js index 414065203fd..08f0c5c7b76 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -1282,12 +1282,12 @@ module.exports = class Workspace extends Model { return this.getPaneItems().filter(item => item instanceof TextEditor) } - // Essential: Get the active item if it is an {TextEditor}. + // Essential: Get the workspace center's active item if it is a {TextEditor}. // - // Returns an {TextEditor} or `undefined` if the current active item is not an - // {TextEditor}. + // Returns a {TextEditor} or `undefined` if the workspace center's current + // active item is not a {TextEditor}. getActiveTextEditor () { - const activeItem = this.getActivePaneItem() + const activeItem = this.getCenter().getActivePaneItem() if (activeItem instanceof TextEditor) { return activeItem } } From 05efc143edc9bff3bccb054d500786b63d81dc4c Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Wed, 31 May 2017 16:08:24 -0400 Subject: [PATCH 03/18] Add Workspace::observeActiveTextEditor() --- spec/workspace-spec.js | 84 ++++++++++++++++++++++++++++++++++++++++++ src/workspace.js | 23 ++++++++++++ 2 files changed, 107 insertions(+) diff --git a/spec/workspace-spec.js b/spec/workspace-spec.js index 98b448c2bdf..ef7b11cb9fc 100644 --- a/spec/workspace-spec.js +++ b/spec/workspace-spec.js @@ -1471,6 +1471,90 @@ describe('Workspace', () => { }) }) + describe('::observeActiveTextEditor()', () => { + let center, pane, observed + + beforeEach(() => { + center = workspace.getCenter() + pane = center.getActivePane() + observed = [] + }) + + it('invokes the observer with current active text editor and each time a different text editor becomes active', () => { + const inactiveEditorBeforeRegisteringObserver = new TextEditor() + const activeEditorBeforeRegisteringObserver = new TextEditor() + pane.activateItem(inactiveEditorBeforeRegisteringObserver) + pane.activateItem(activeEditorBeforeRegisteringObserver) + + workspace.observeActiveTextEditor(editor => observed.push(editor)) + + const editorAddedAfterRegisteringObserver = new TextEditor() + const nonEditorItemAddedAfterRegisteringObserver = document.createElement('div') + pane.activateItem(editorAddedAfterRegisteringObserver) + + expect(observed).toEqual( + [activeEditorBeforeRegisteringObserver, editorAddedAfterRegisteringObserver] + ) + }) + + it('does not invoke the observer when there is no current active text editor', () => { + const editor = new TextEditor() + const nonEditorItem = document.createElement('div') + pane.activateItem(editor) + pane.activateItem(nonEditorItem) + + workspace.observeActiveTextEditor(editor => observed.push(editor)) + + expect(observed).toEqual([]) + }) + + it("invokes the observer when a text editor becomes the workspace center's active pane item while a dock has focus", () => { + workspace.observeActiveTextEditor(editor => observed.push(editor)) + + const dock = workspace.getLeftDock() + dock.activate() + expect(atom.workspace.getActivePaneContainer()).toBe(dock) + + const editor = new TextEditor() + center.getActivePane().activateItem(editor) + expect(atom.workspace.getActivePaneContainer()).toBe(dock) + + expect(observed).toEqual([editor]) + }) + + it('invokes the observer when the last text editor is closed', () => { + workspace.observeActiveTextEditor(editor => observed.push(editor)) + + const editor = new TextEditor() + pane.activateItem(editor) + pane.destroyItem(editor) + + expect(observed).toEqual([editor, undefined]) + }) + + it("invokes the observer when the workspace center's active pane item changes from an editor item to a non-editor item", () => { + workspace.observeActiveTextEditor(editor => observed.push(editor)) + + const editor = new TextEditor() + const nonEditorItem = document.createElement('div') + pane.activateItem(editor) + pane.activateItem(nonEditorItem) + + expect(observed).toEqual([editor, undefined]) + }) + + it("does not invoke the observer when the workspace center's active pane item changes from a non-editor item to another non-editor item", () => { + workspace.observeActiveTextEditor(editor => observed.push(editor)) + + const nonEditorItem1 = document.createElement('div') + const nonEditorItem2 = document.createElement('div') + pane.activateItem(nonEditorItem1) + pane.activateItem(nonEditorItem1) + + expect(observed).toEqual([]) + }) + }) + describe('when an editor is destroyed', () => { it('removes the editor', () => { let editor = null diff --git a/src/workspace.js b/src/workspace.js index 08f0c5c7b76..c0f6d4ef408 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -216,6 +216,7 @@ module.exports = class Workspace extends Model { bottom: this.createDock('bottom') } this.activePaneContainer = this.paneContainers.center + this.activeTextEditor = null this.panelContainers = { top: new PanelContainer({viewRegistry: this.viewRegistry, location: 'top'}), @@ -422,6 +423,17 @@ module.exports = class Workspace extends Model { this.didChangeActivePaneItem(item) this.emitter.emit('did-change-active-pane-item', item) } + + if (paneContainer === this.getCenter()) { + const activeTextEditorChanged = + (item instanceof TextEditor) || (this.activeTextEditor instanceof TextEditor) + + if (activeTextEditorChanged) { + this.activeTextEditor = (item instanceof TextEditor) ? item : null + const itemValue = (item instanceof TextEditor) ? item : undefined + this.emitter.emit('did-change-active-text-editor', itemValue) + } + } } didChangeActivePaneItem (item) { @@ -648,6 +660,10 @@ module.exports = class Workspace extends Model { return this.emitter.on('did-stop-changing-active-pane-item', callback) } + onDidChangeActiveTextEditor (callback) { + return this.emitter.on('did-change-active-text-editor', callback) + } + // Essential: Invoke the given callback with the current active pane item and // with all future active pane items in the workspace. // @@ -660,6 +676,13 @@ module.exports = class Workspace extends Model { return this.onDidChangeActivePaneItem(callback) } + observeActiveTextEditor (callback) { + const activeTextEditor = this.getActiveTextEditor() + if (activeTextEditor != null) { callback(activeTextEditor) } + + return this.onDidChangeActiveTextEditor(callback) + } + // Essential: Invoke the given callback whenever an item is opened. Unlike // {::onDidAddPaneItem}, observers will be notified for items that are already // present in the workspace when they are reopened. From 16e1ef917be66637a58bad5e09c7d546782ee6e8 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 09:34:56 -0400 Subject: [PATCH 04/18] Update Workspace specs regarding editors in docks --- spec/workspace-spec.js | 63 +++++------------------------------------- 1 file changed, 7 insertions(+), 56 deletions(-) diff --git a/spec/workspace-spec.js b/spec/workspace-spec.js index ef7b11cb9fc..ac4fdceb621 100644 --- a/spec/workspace-spec.js +++ b/spec/workspace-spec.js @@ -120,62 +120,6 @@ describe('Workspace', () => { expect(atom.workspace.getTextEditors().length).toBe(0) }) }) - - describe('where a dock contains an editor', () => { - afterEach(() => { - atom.workspace.getRightDock().paneContainer.destroy() - }) - - it('constructs the view with the same panes', () => { - const pane1 = atom.workspace.getRightDock().getActivePane() - const pane2 = pane1.splitRight({copyActiveItem: true}) - const pane3 = pane2.splitRight({copyActiveItem: true}) - let pane4 = null - - waitsForPromise(() => - atom.workspace.open(null, {location: 'right'}).then(editor => editor.setText('An untitled editor.')) - ) - - waitsForPromise(() => - atom.workspace.open('b', {location: 'right'}).then(editor => pane2.activateItem(editor.copy())) - ) - - waitsForPromise(() => - atom.workspace.open('../sample.js', {location: 'right'}).then(editor => pane3.activateItem(editor)) - ) - - runs(() => { - pane3.activeItem.setCursorScreenPosition([2, 4]) - pane4 = pane2.splitDown() - }) - - waitsForPromise(() => - atom.workspace.open('../sample.txt', {location: 'right'}).then(editor => pane4.activateItem(editor)) - ) - - runs(() => { - pane4.getActiveItem().setCursorScreenPosition([0, 2]) - pane2.activate() - - simulateReload() - - expect(atom.workspace.getTextEditors().length).toBe(5) - const [editor1, editor2, untitledEditor, editor3, editor4] = atom.workspace.getTextEditors() - const firstDirectory = atom.project.getDirectories()[0] - expect(firstDirectory).toBeDefined() - expect(editor1.getPath()).toBe(firstDirectory.resolve('b')) - expect(editor2.getPath()).toBe(firstDirectory.resolve('../sample.txt')) - expect(editor2.getCursorScreenPosition()).toEqual([0, 2]) - expect(editor3.getPath()).toBe(firstDirectory.resolve('b')) - expect(editor4.getPath()).toBe(firstDirectory.resolve('../sample.js')) - expect(editor4.getCursorScreenPosition()).toEqual([2, 4]) - expect(untitledEditor.getPath()).toBeUndefined() - expect(untitledEditor.getText()).toBe('An untitled editor.') - - expect(atom.workspace.getRightDock().getActiveTextEditor().getPath()).toBe(editor3.getPath()) - }) - }) - }) }) describe('::open(itemOrURI, options)', () => { @@ -429,6 +373,13 @@ describe('Workspace', () => { }) }) + describe('when attempting to open an editor in a dock', () => { + it('opens the editor in the workspace center', async () => { + await atom.workspace.open('sample.txt', {location: 'right'}) + expect(atom.workspace.getCenter().getActivePaneItem().getFileName()).toEqual('sample.txt') + }) + }) + describe('when called with an item rather than a URI', () => { it('adds the item itself to the workspace', async () => { const item = document.createElement('div') From e569f8fc4d82fd340b4752ae6764047fbbfe989a Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 12:08:09 -0400 Subject: [PATCH 05/18] :memo: Fix odd grammar in API docs --- src/dock.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/dock.js b/src/dock.js index d531e8627e2..cc6d0bcf2fe 100644 --- a/src/dock.js +++ b/src/dock.js @@ -590,9 +590,9 @@ module.exports = class Dock { return this.paneContainer.getTextEditors() } - // Essential: Get the active item if it is an {TextEditor}. + // Essential: Get the active item if it is a {TextEditor}. // - // Returns an {TextEditor} or `undefined` if the current active item is not an + // Returns a {TextEditor} or `undefined` if the current active item is not a // {TextEditor}. getActiveTextEditor () { const activeItem = this.getActivePaneItem() From f8ebd71200af4dcc5cf6fc2d85e68247833d9acc Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 15:24:50 -0400 Subject: [PATCH 06/18] Deprecate Dock::getActiveTextEditor() --- spec/dock-spec.js | 11 +++++++++++ src/dock.js | 5 ++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/spec/dock-spec.js b/spec/dock-spec.js index f9b8a53262f..e554094f2f4 100644 --- a/spec/dock-spec.js +++ b/spec/dock-spec.js @@ -1,5 +1,7 @@ /** @babel */ +const Grim = require('grim') + import {it, fit, ffit, fffit, beforeEach, afterEach} from './async-spec-helpers' describe('Dock', () => { @@ -329,4 +331,13 @@ describe('Dock', () => { expect(() => atom.workspace.getElement().handleDragStart(dragEvent)).not.toThrow() }) }) + + describe('::getActiveTextEditor()', () => { + it('is deprecated', () => { + spyOn(Grim, 'deprecate') + + atom.workspace.getLeftDock().getActiveTextEditor() + expect(Grim.deprecate.callCount).toBe(1) + }) + }) }) diff --git a/src/dock.js b/src/dock.js index cc6d0bcf2fe..f354f1b901c 100644 --- a/src/dock.js +++ b/src/dock.js @@ -4,6 +4,7 @@ const _ = require('underscore-plus') const {CompositeDisposable} = require('event-kit') const PaneContainer = require('./pane-container') const TextEditor = require('./text-editor') +const Grim = require('grim') const MINIMUM_SIZE = 100 const DEFAULT_INITIAL_SIZE = 300 @@ -590,11 +591,13 @@ module.exports = class Dock { return this.paneContainer.getTextEditors() } - // Essential: Get the active item if it is a {TextEditor}. + // Deprecated: Get the active item if it is a {TextEditor}. // // Returns a {TextEditor} or `undefined` if the current active item is not a // {TextEditor}. getActiveTextEditor () { + Grim.deprecate('Text editors are not allowed in docks. Use atom.workspace.getActiveTextEditor() instead.') + const activeItem = this.getActivePaneItem() if (activeItem instanceof TextEditor) { return activeItem } } From 0b314ac1a98ca06c82fc369fffe4f13fc772abe5 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 15:26:09 -0400 Subject: [PATCH 07/18] :fire: Remove broken Dock::observeTextEditors(callback) fn We're updating Atom to disallow editors in docks. As a result, we need to remove Dock::observeTextEditors(callback). Normally, we would deprecate a method before removing it, but this method is broken and has never worked: atom.workspace.getLeftDock().observeTextEditors(console.log) (unknown) Uncaught TypeError: this.paneContainer.getTextEditors is not a function at Dock.getTextEditors (/Applications/Atom.app/Contents/Resources/app/src/dock.js:590:37) at Dock.observeTextEditors (/Applications/Atom.app/Contents/Resources/app/src/dock.js:396:41) at :1:30 getTextEditors @ :29933 observeTextEditors @ :29739 (anonymous) @ VM1941:1 Since the method is broken, we know that nobody is relying on it. Instead of deprecating the method, we can just remove it. --- src/dock.js | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/src/dock.js b/src/dock.js index f354f1b901c..8f49528f14a 100644 --- a/src/dock.js +++ b/src/dock.js @@ -385,21 +385,6 @@ module.exports = class Dock { Section: Event Subscription */ - // Essential: Invoke the given callback with all current and future text - // editors in the dock. - // - // * `callback` {Function} to be called with current and future text editors. - // * `editor` An {TextEditor} that is present in {::getTextEditors} at the time - // of subscription or that is added at some later time. - // - // Returns a {Disposable} on which `.dispose()` can be called to unsubscribe. - observeTextEditors (callback) { - for (const textEditor of this.getTextEditors()) { - callback(textEditor) - } - return this.onDidAddTextEditor(({textEditor}) => callback(textEditor)) - } - // Essential: Invoke the given callback with all current and future panes items // in the dock. // From 09495dfc35b1294df0f4785e00d4eb1f8c7e27fc Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 15:30:43 -0400 Subject: [PATCH 08/18] :fire: Remove broken Dock::getTextEditors() function We're updating Atom to disallow editors in docks. As a result, we need to remove Dock::getTextEditors(). Normally, we would deprecate a method before removing it, but this method is broken and has never worked: atom.workspace.getLeftDock().getTextEditors() (unknown) Uncaught TypeError: this.paneContainer.getTextEditors is not a function at Dock.getTextEditors (/Applications/Atom.app/Contents/Resources/app/src/dock.js:590:37) at :1:30 Since the method is broken, we know that nobody is relying on it. Instead of deprecating the method, we can just remove it. --- src/dock.js | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/dock.js b/src/dock.js index 8f49528f14a..0023c52e543 100644 --- a/src/dock.js +++ b/src/dock.js @@ -569,13 +569,6 @@ module.exports = class Dock { return this.paneContainer.getActivePaneItem() } - // Essential: Get all text editors in the dock. - // - // Returns an {Array} of {TextEditor}s. - getTextEditors () { - return this.paneContainer.getTextEditors() - } - // Deprecated: Get the active item if it is a {TextEditor}. // // Returns a {TextEditor} or `undefined` if the current active item is not a From 5b52d8c77aca6e83926b50504a0e77efb8a5f366 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 15:43:11 -0400 Subject: [PATCH 09/18] :memo: Add API docs for onDidChangeActiveTextEditor(callback) --- src/workspace.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/workspace.js b/src/workspace.js index c0f6d4ef408..ee53d8b1273 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -660,6 +660,14 @@ module.exports = class Workspace extends Model { return this.emitter.on('did-stop-changing-active-pane-item', callback) } + // Essential: Invoke the given callback when a text editor becomes the active + // text editor and when there is no longer an active text editor. + // + // * `callback` {Function} to be called when the active text editor changes. + // * `editor` The active {TextEditor} or undefined if there is no longer an + // active text editor. + // + // Returns a {Disposable} on which `.dispose()` can be called to unsubscribe. onDidChangeActiveTextEditor (callback) { return this.emitter.on('did-change-active-text-editor', callback) } From 14d8eccc6e5c7adff28888c338dda1086af2a556 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 15:47:31 -0400 Subject: [PATCH 10/18] :memo: Add API docs for observeActiveTextEditor(callback) --- src/workspace.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/workspace.js b/src/workspace.js index ee53d8b1273..bb7f01cafcb 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -684,6 +684,15 @@ module.exports = class Workspace extends Model { return this.onDidChangeActivePaneItem(callback) } + // Essential: Invoke the given callback with the current active text editor + // (if any), with all future active text editors, and when there is no longer + // an active text editor. + // + // * `callback` {Function} to be called when the active text editor changes. + // * `editor` The active {TextEditor} or undefined if there is no longer an + // active text editor. + // + // Returns a {Disposable} on which `.dispose()` can be called to unsubscribe. observeActiveTextEditor (callback) { const activeTextEditor = this.getActiveTextEditor() if (activeTextEditor != null) { callback(activeTextEditor) } From fe550a1b039be63821e7ea43a852e7e00b3bb1d0 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 17:01:18 -0400 Subject: [PATCH 11/18] :art: Refactor: Introduce explaining variable xref: https://github.com/atom/atom/pull/14695#discussion_r119721719 --- src/workspace.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/workspace.js b/src/workspace.js index bb7f01cafcb..5489824c24c 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -425,12 +425,13 @@ module.exports = class Workspace extends Model { } if (paneContainer === this.getCenter()) { + const itemIsTextEditor = item instanceof TextEditor const activeTextEditorChanged = - (item instanceof TextEditor) || (this.activeTextEditor instanceof TextEditor) + itemIsTextEditor || (this.activeTextEditor instanceof TextEditor) if (activeTextEditorChanged) { - this.activeTextEditor = (item instanceof TextEditor) ? item : null - const itemValue = (item instanceof TextEditor) ? item : undefined + this.activeTextEditor = itemIsTextEditor ? item : null + const itemValue = itemIsTextEditor ? item : undefined this.emitter.emit('did-change-active-text-editor', itemValue) } } From 6a694f80f41730e9a32d208b5dc5547e3b286de9 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 17:04:43 -0400 Subject: [PATCH 12/18] :art: Replace instanceof check with nullness check xref: https://github.com/atom/atom/pull/14695#discussion_r119721719 --- src/workspace.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/workspace.js b/src/workspace.js index 5489824c24c..d09cc28622d 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -427,7 +427,7 @@ module.exports = class Workspace extends Model { if (paneContainer === this.getCenter()) { const itemIsTextEditor = item instanceof TextEditor const activeTextEditorChanged = - itemIsTextEditor || (this.activeTextEditor instanceof TextEditor) + itemIsTextEditor || (this.activeTextEditor != null) if (activeTextEditorChanged) { this.activeTextEditor = itemIsTextEditor ? item : null From 5dfbb65b0e4f92eac8bbc2f624aba50b089a9c03 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Thu, 1 Jun 2017 17:09:38 -0400 Subject: [PATCH 13/18] :art: Reduce ternaries --- src/workspace.js | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/workspace.js b/src/workspace.js index d09cc28622d..24ec4dbbf4d 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -425,13 +425,12 @@ module.exports = class Workspace extends Model { } if (paneContainer === this.getCenter()) { - const itemIsTextEditor = item instanceof TextEditor - const activeTextEditorChanged = - itemIsTextEditor || (this.activeTextEditor != null) + const newActiveTextEditor = (item instanceof TextEditor) ? item : null + const oldActiveTextEditor = this.activeTextEditor - if (activeTextEditorChanged) { - this.activeTextEditor = itemIsTextEditor ? item : null - const itemValue = itemIsTextEditor ? item : undefined + if (newActiveTextEditor || oldActiveTextEditor) { + this.activeTextEditor = newActiveTextEditor + const itemValue = (this.activeTextEditor || undefined) this.emitter.emit('did-change-active-text-editor', itemValue) } } From 5b61c0a949e70d258b5ce2657dc9a1f5507d1878 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Fri, 2 Jun 2017 07:39:52 -0400 Subject: [PATCH 14/18] :bug: Initialize active editor state correctly after reload Fixes the following bug: 1. Open Atom 2. Open a file 3. Observe the file's encoding in the status bar 4. Reload Atom 5. Close the file 6. Observe that the closed file's encoding is still present in the status bar This bug occured because the reload did not deserialize/serialze the workspace's active text editor state. As a result, when closing the text editor in step 5, we failed to notify observers that there is no longer an active text editor. --- spec/workspace-spec.js | 14 ++++++++++++++ src/workspace.js | 18 +++++++++++------- 2 files changed, 25 insertions(+), 7 deletions(-) diff --git a/spec/workspace-spec.js b/spec/workspace-spec.js index ac4fdceb621..7fd27329c97 100644 --- a/spec/workspace-spec.js +++ b/spec/workspace-spec.js @@ -120,6 +120,20 @@ describe('Workspace', () => { expect(atom.workspace.getTextEditors().length).toBe(0) }) }) + + it('remembers whether the workspace had an active text editor', async () => { + await atom.workspace.open('../sample.txt') + expect(atom.workspace.hasActiveTextEditor).toBe(true) + + simulateReload() + expect(atom.workspace.hasActiveTextEditor).toBe(true) + + atom.workspace.getActivePane().destroy() + expect(atom.workspace.hasActiveTextEditor).toBe(false) + + simulateReload() + expect(atom.workspace.hasActiveTextEditor).toBe(false) + }) }) describe('::open(itemOrURI, options)', () => { diff --git a/src/workspace.js b/src/workspace.js index 24ec4dbbf4d..6f990969405 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -216,7 +216,7 @@ module.exports = class Workspace extends Model { bottom: this.createDock('bottom') } this.activePaneContainer = this.paneContainers.center - this.activeTextEditor = null + this.hasActiveTextEditor = false this.panelContainers = { top: new PanelContainer({viewRegistry: this.viewRegistry, location: 'top'}), @@ -345,7 +345,8 @@ module.exports = class Workspace extends Model { left: this.paneContainers.left.serialize(), right: this.paneContainers.right.serialize(), bottom: this.paneContainers.bottom.serialize() - } + }, + hasActiveTextEditor: this.hasActiveTextEditor } } @@ -372,6 +373,10 @@ module.exports = class Workspace extends Model { this.paneContainers.center.deserialize(state.paneContainer, deserializerManager) } + if (state.hasActiveTextEditor != null) { + this.hasActiveTextEditor = state.hasActiveTextEditor + } + this.updateWindowTitle() } @@ -425,12 +430,11 @@ module.exports = class Workspace extends Model { } if (paneContainer === this.getCenter()) { - const newActiveTextEditor = (item instanceof TextEditor) ? item : null - const oldActiveTextEditor = this.activeTextEditor + const hadActiveTextEditor = this.hasActiveTextEditor + this.hasActiveTextEditor = item instanceof TextEditor - if (newActiveTextEditor || oldActiveTextEditor) { - this.activeTextEditor = newActiveTextEditor - const itemValue = (this.activeTextEditor || undefined) + if (this.hasActiveTextEditor || hadActiveTextEditor) { + const itemValue = this.hasActiveTextEditor ? item : undefined this.emitter.emit('did-change-active-text-editor', itemValue) } } From 9629caefb7e6423020d035b0b213a1d266917fc2 Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Fri, 2 Jun 2017 15:09:20 -0400 Subject: [PATCH 15/18] Remove unnecessary serialization --- src/workspace.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/workspace.js b/src/workspace.js index 6f990969405..991f74a71be 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -345,8 +345,7 @@ module.exports = class Workspace extends Model { left: this.paneContainers.left.serialize(), right: this.paneContainers.right.serialize(), bottom: this.paneContainers.bottom.serialize() - }, - hasActiveTextEditor: this.hasActiveTextEditor + } } } @@ -373,9 +372,7 @@ module.exports = class Workspace extends Model { this.paneContainers.center.deserialize(state.paneContainer, deserializerManager) } - if (state.hasActiveTextEditor != null) { - this.hasActiveTextEditor = state.hasActiveTextEditor - } + this.hasActiveTextEditor = this.getActiveTextEditor() != null this.updateWindowTitle() } From 44a2be7c9dc99a091fb3fb8c207b8c4bce7a609b Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Fri, 2 Jun 2017 15:27:08 -0400 Subject: [PATCH 16/18] Test deserialization in terms of user-observable functionality - Rework serialization/deserialization test - Move simulateReload function so that it can be used in multiple describe blocks --- spec/workspace-spec.js | 81 ++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/spec/workspace-spec.js b/spec/workspace-spec.js index 7fd27329c97..e0279336997 100644 --- a/spec/workspace-spec.js +++ b/spec/workspace-spec.js @@ -31,35 +31,35 @@ describe('Workspace', () => { afterEach(() => temp.cleanupSync()) - describe('serialization', () => { - const simulateReload = () => { - const workspaceState = atom.workspace.serialize() - const projectState = atom.project.serialize({isUnloading: true}) - atom.workspace.destroy() - atom.project.destroy() - atom.project = new Project({ - notificationManager: atom.notifications, - packageManager: atom.packages, - confirm: atom.confirm.bind(atom), - applicationDelegate: atom.applicationDelegate - }) - atom.project.deserialize(projectState) - atom.workspace = new Workspace({ - config: atom.config, - project: atom.project, - packageManager: atom.packages, - grammarRegistry: atom.grammars, - styleManager: atom.styles, - deserializerManager: atom.deserializers, - notificationManager: atom.notifications, - applicationDelegate: atom.applicationDelegate, - viewRegistry: atom.views, - assert: atom.assert.bind(atom), - textEditorRegistry: atom.textEditors - }) - return atom.workspace.deserialize(workspaceState, atom.deserializers) - } + const simulateReload = () => { + const workspaceState = workspace.serialize() + const projectState = atom.project.serialize({isUnloading: true}) + workspace.destroy() + atom.project.destroy() + atom.project = new Project({ + notificationManager: atom.notifications, + packageManager: atom.packages, + confirm: atom.confirm.bind(atom), + applicationDelegate: atom.applicationDelegate + }) + atom.project.deserialize(projectState) + workspace = atom.workspace = new Workspace({ + config: atom.config, + project: atom.project, + packageManager: atom.packages, + grammarRegistry: atom.grammars, + styleManager: atom.styles, + deserializerManager: atom.deserializers, + notificationManager: atom.notifications, + applicationDelegate: atom.applicationDelegate, + viewRegistry: atom.views, + assert: atom.assert.bind(atom), + textEditorRegistry: atom.textEditors + }) + return workspace.deserialize(workspaceState, atom.deserializers) + } + describe('serialization', () => { describe('when the workspace contains text editors', () => { it('constructs the view with the same panes', () => { const pane1 = atom.workspace.getActivePane() @@ -120,20 +120,6 @@ describe('Workspace', () => { expect(atom.workspace.getTextEditors().length).toBe(0) }) }) - - it('remembers whether the workspace had an active text editor', async () => { - await atom.workspace.open('../sample.txt') - expect(atom.workspace.hasActiveTextEditor).toBe(true) - - simulateReload() - expect(atom.workspace.hasActiveTextEditor).toBe(true) - - atom.workspace.getActivePane().destroy() - expect(atom.workspace.hasActiveTextEditor).toBe(false) - - simulateReload() - expect(atom.workspace.hasActiveTextEditor).toBe(false) - }) }) describe('::open(itemOrURI, options)', () => { @@ -1518,6 +1504,17 @@ describe('Workspace', () => { expect(observed).toEqual([]) }) + + it('invokes the observer when closing the one and only text editor after deserialization', async () => { + pane.activateItem(new TextEditor()) + + simulateReload() + + const editor = workspace.getActiveTextEditor() + workspace.observeActiveTextEditor(editor => observed.push(editor)) + workspace.closeActivePaneItemOrEmptyPaneOrWindow() + expect(observed).toEqual([editor, undefined]) + }) }) describe('when an editor is destroyed', () => { From 2347c9bdaa7ba1de577a5850412ad1c5c42560fa Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Fri, 2 Jun 2017 15:55:00 -0400 Subject: [PATCH 17/18] Always invoke callback with current active text editor This makes observeActiveTextEditor consistent with observers like observeActivePaneItem, which always invoke the callback with the current value, regardless of whether that value is undefined or not. --- spec/workspace-spec.js | 47 +++++++++++++++++------------------------- src/workspace.js | 5 ++--- 2 files changed, 21 insertions(+), 31 deletions(-) diff --git a/spec/workspace-spec.js b/spec/workspace-spec.js index e0279336997..7fd8aa01918 100644 --- a/spec/workspace-spec.js +++ b/spec/workspace-spec.js @@ -1423,15 +1423,10 @@ describe('Workspace', () => { }) describe('::observeActiveTextEditor()', () => { - let center, pane, observed - - beforeEach(() => { - center = workspace.getCenter() - pane = center.getActivePane() + it('invokes the observer with current active text editor and each time a different text editor becomes active', () => { + const pane = workspace.getCenter().getActivePane() observed = [] - }) - it('invokes the observer with current active text editor and each time a different text editor becomes active', () => { const inactiveEditorBeforeRegisteringObserver = new TextEditor() const activeEditorBeforeRegisteringObserver = new TextEditor() pane.activateItem(inactiveEditorBeforeRegisteringObserver) @@ -1447,20 +1442,19 @@ describe('Workspace', () => { [activeEditorBeforeRegisteringObserver, editorAddedAfterRegisteringObserver] ) }) + }) - it('does not invoke the observer when there is no current active text editor', () => { - const editor = new TextEditor() - const nonEditorItem = document.createElement('div') - pane.activateItem(editor) - pane.activateItem(nonEditorItem) - - workspace.observeActiveTextEditor(editor => observed.push(editor)) + describe('::onDidChangeActiveTextEditor()', () => { + let center, pane, observed - expect(observed).toEqual([]) + beforeEach(() => { + center = workspace.getCenter() + pane = center.getActivePane() + observed = [] }) it("invokes the observer when a text editor becomes the workspace center's active pane item while a dock has focus", () => { - workspace.observeActiveTextEditor(editor => observed.push(editor)) + workspace.onDidChangeActiveTextEditor(editor => observed.push(editor)) const dock = workspace.getLeftDock() dock.activate() @@ -1474,28 +1468,26 @@ describe('Workspace', () => { }) it('invokes the observer when the last text editor is closed', () => { - workspace.observeActiveTextEditor(editor => observed.push(editor)) - const editor = new TextEditor() pane.activateItem(editor) - pane.destroyItem(editor) - expect(observed).toEqual([editor, undefined]) + workspace.onDidChangeActiveTextEditor(editor => observed.push(editor)) + pane.destroyItem(editor) + expect(observed).toEqual([undefined]) }) it("invokes the observer when the workspace center's active pane item changes from an editor item to a non-editor item", () => { - workspace.observeActiveTextEditor(editor => observed.push(editor)) - const editor = new TextEditor() const nonEditorItem = document.createElement('div') pane.activateItem(editor) - pane.activateItem(nonEditorItem) - expect(observed).toEqual([editor, undefined]) + workspace.onDidChangeActiveTextEditor(editor => observed.push(editor)) + pane.activateItem(nonEditorItem) + expect(observed).toEqual([undefined]) }) it("does not invoke the observer when the workspace center's active pane item changes from a non-editor item to another non-editor item", () => { - workspace.observeActiveTextEditor(editor => observed.push(editor)) + workspace.onDidChangeActiveTextEditor(editor => observed.push(editor)) const nonEditorItem1 = document.createElement('div') const nonEditorItem2 = document.createElement('div') @@ -1510,10 +1502,9 @@ describe('Workspace', () => { simulateReload() - const editor = workspace.getActiveTextEditor() - workspace.observeActiveTextEditor(editor => observed.push(editor)) + workspace.onDidChangeActiveTextEditor(editor => observed.push(editor)) workspace.closeActivePaneItemOrEmptyPaneOrWindow() - expect(observed).toEqual([editor, undefined]) + expect(observed).toEqual([undefined]) }) }) diff --git a/src/workspace.js b/src/workspace.js index 991f74a71be..60080592f48 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -690,13 +690,12 @@ module.exports = class Workspace extends Model { // an active text editor. // // * `callback` {Function} to be called when the active text editor changes. - // * `editor` The active {TextEditor} or undefined if there is no longer an + // * `editor` The active {TextEditor} or undefined if there is not an // active text editor. // // Returns a {Disposable} on which `.dispose()` can be called to unsubscribe. observeActiveTextEditor (callback) { - const activeTextEditor = this.getActiveTextEditor() - if (activeTextEditor != null) { callback(activeTextEditor) } + callback(this.getActiveTextEditor()) return this.onDidChangeActiveTextEditor(callback) } From e1719a8923ea1bd80fd66c1df99dc0da29ac209b Mon Sep 17 00:00:00 2001 From: Jason Rudolph Date: Fri, 2 Jun 2017 15:56:13 -0400 Subject: [PATCH 18/18] Teach Workspace::reset() to properly reset hasActiveTextEditor --- src/workspace.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/workspace.js b/src/workspace.js index 60080592f48..53f7f5c624a 100644 --- a/src/workspace.js +++ b/src/workspace.js @@ -297,6 +297,7 @@ module.exports = class Workspace extends Model { bottom: this.createDock('bottom') } this.activePaneContainer = this.paneContainers.center + this.hasActiveTextEditor = false this.panelContainers = { top: new PanelContainer({viewRegistry: this.viewRegistry, location: 'top'}),