From b0d93accf7871c09b9d14bedb887bb25ef47d142 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 17 Jun 2015 12:24:35 +0200 Subject: [PATCH 1/3] :bug: Remove Gutter from ViewRegistry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #7306 We started noticing that when a `TextEditor` pane got split, the same view for `Gutter` was being shared amongst several models, thereby making the same DOM element accessible simultaneously by more than one object. This made us experience *orphaned line numbers*, caused by two instances of `LineNumberGutterComponent` mutating `.line-numbers` at the same time. This is a typical race condition which I would normally address by understanding and possibly locking the correct order in which operations should happen. However, I believe in this situation we shouldn’t actually care about ordering at all, since I think views should be kept “local”, thus avoiding to expose them to the world and/or reusing them across other views (either accidentally as in this case or on purpose). --- spec/custom-gutter-component-spec.coffee | 3 -- spec/gutter-container-component-spec.coffee | 38 ++++++++++++--------- src/custom-gutter-component.coffee | 6 ++-- src/line-number-gutter-component.coffee | 5 ++- src/pane-container.coffee | 3 -- 5 files changed, 25 insertions(+), 30 deletions(-) diff --git a/spec/custom-gutter-component-spec.coffee b/spec/custom-gutter-component-spec.coffee index 4b7d81fba..37da68807 100644 --- a/spec/custom-gutter-component-spec.coffee +++ b/spec/custom-gutter-component-spec.coffee @@ -16,9 +16,6 @@ describe "CustomGutterComponent", -> decorationsWrapperNode = gutterComponent.getDomNode().children.item(0) expect(decorationsWrapperNode.classList.contains('custom-decorations')).toBe true - it "makes its view accessible from the view registry", -> - expect(gutterComponent.getDomNode()).toBe atom.views.getView(gutter) - it "hides its DOM node when ::hideNode is called, and shows its DOM node when ::showNode is called", -> gutterComponent.hideNode() expect(gutterComponent.getDomNode().style.display).toBe 'none' diff --git a/spec/gutter-container-component-spec.coffee b/spec/gutter-container-component-spec.coffee index 5d815fea8..6048655f1 100644 --- a/spec/gutter-container-component-spec.coffee +++ b/spec/gutter-container-component-spec.coffee @@ -107,22 +107,24 @@ describe "GutterContainerComponent", -> gutterContainerComponent.updateSync(testState) expect(gutterContainerComponent.getDomNode().children.length).toBe 2 - expectedCustomGutterNode = gutterContainerComponent.getDomNode().children.item(0) - expect(expectedCustomGutterNode).toBe atom.views.getView(customGutter1) - expectedLineNumbersNode = gutterContainerComponent.getDomNode().children.item(1) - expect(expectedLineNumbersNode).toBe atom.views.getView(lineNumberGutter) + + initialCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(0) + initialLineNumbersNode = gutterContainerComponent.getDomNode().children.item(1) # Add a gutter. customGutter2 = new Gutter(mockGutterContainer, {name: 'custom2', priority: -10}) testState = buildTestState([customGutter1, customGutter2, lineNumberGutter]) gutterContainerComponent.updateSync(testState) + expect(gutterContainerComponent.getDomNode().children.length).toBe 3 - expectedCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(0) - expect(expectedCustomGutterNode1).toBe atom.views.getView(customGutter1) - expectedCustomGutterNode2 = gutterContainerComponent.getDomNode().children.item(1) - expect(expectedCustomGutterNode2).toBe atom.views.getView(customGutter2) - expectedLineNumbersNode = gutterContainerComponent.getDomNode().children.item(2) - expect(expectedLineNumbersNode).toBe atom.views.getView(lineNumberGutter) + + unchangedCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(0) + insertedCustomGutterNode2 = gutterContainerComponent.getDomNode().children.item(1) + repositionedLineNumbersNode = gutterContainerComponent.getDomNode().children.item(2) + + expect(initialCustomGutterNode1).toBe(unchangedCustomGutterNode1) + expect(initialLineNumbersNode).toBe(repositionedLineNumbersNode) + expect(insertedCustomGutterNode2).toBeDefined() # Hide one gutter, reposition one gutter, remove one gutter; and add a new gutter. customGutter2.hide() @@ -130,10 +132,12 @@ describe "GutterContainerComponent", -> testState = buildTestState([customGutter2, customGutter1, customGutter3]) gutterContainerComponent.updateSync(testState) expect(gutterContainerComponent.getDomNode().children.length).toBe 3 - expectedCustomGutterNode2 = gutterContainerComponent.getDomNode().children.item(0) - expect(expectedCustomGutterNode2).toBe atom.views.getView(customGutter2) - expect(expectedCustomGutterNode2.style.display).toBe 'none' - expectedCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(1) - expect(expectedCustomGutterNode1).toBe atom.views.getView(customGutter1) - expectedCustomGutterNode3 = gutterContainerComponent.getDomNode().children.item(2) - expect(expectedCustomGutterNode3).toBe atom.views.getView(customGutter3) + + repositionedCustomGutterNode2 = gutterContainerComponent.getDomNode().children.item(0) + repositionedCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(1) + insertedCustomGutterNode3 = gutterContainerComponent.getDomNode().children.item(2) + + expect(initialCustomGutterNode1).toBe(repositionedCustomGutterNode1) + expect(repositionedCustomGutterNode2).toBe(insertedCustomGutterNode2) + expect(repositionedCustomGutterNode2.style.display).toBe('none') + expect(insertedCustomGutterNode3).toBeDefined() diff --git a/src/custom-gutter-component.coffee b/src/custom-gutter-component.coffee index 39f5a80a1..5694f6182 100644 --- a/src/custom-gutter-component.coffee +++ b/src/custom-gutter-component.coffee @@ -1,4 +1,4 @@ -{setDimensionsAndBackground} = require './gutter-component-helpers' +{createGutterView, setDimensionsAndBackground} = require './gutter-component-helpers' # This class represents a gutter other than the 'line-numbers' gutter. # The contents of this gutter may be specified by Decorations. @@ -11,10 +11,8 @@ class CustomGutterComponent @decorationItemsById = {} @visible = true - @domNode = atom.views.getView(@gutter) + @domNode = createGutterView(@gutter) @decorationsNode = @domNode.firstChild - # Clear the contents in case the domNode is being reused. - @decorationsNode.innerHTML = '' getDomNode: -> @domNode diff --git a/src/line-number-gutter-component.coffee b/src/line-number-gutter-component.coffee index 351275f63..4efcf8dd7 100644 --- a/src/line-number-gutter-component.coffee +++ b/src/line-number-gutter-component.coffee @@ -1,5 +1,5 @@ _ = require 'underscore-plus' -{setDimensionsAndBackground} = require './gutter-component-helpers' +{createGutterView, setDimensionsAndBackground} = require './gutter-component-helpers' WrapperDiv = document.createElement('div') @@ -11,9 +11,8 @@ class LineNumberGutterComponent @lineNumberNodesById = {} @visible = true - @domNode = atom.views.getView(@gutter) + @domNode = createGutterView(@gutter) @lineNumbersNode = @domNode.firstChild - @lineNumbersNode.innerHTML = '' @domNode.addEventListener 'click', @onClick @domNode.addEventListener 'mousedown', @onMouseDown diff --git a/src/pane-container.coffee b/src/pane-container.coffee index 26ef22cac..e6eb97075 100644 --- a/src/pane-container.coffee +++ b/src/pane-container.coffee @@ -2,8 +2,6 @@ Grim = require 'grim' {Emitter, CompositeDisposable} = require 'event-kit' Serializable = require 'serializable' -{createGutterView} = require './gutter-component-helpers' -Gutter = require './gutter' Model = require './model' Pane = require './pane' PaneElement = require './pane-element' @@ -62,7 +60,6 @@ class PaneContainer extends Model new PaneElement().initialize(model) atom.views.addViewProvider TextEditor, (model) -> new TextEditorElement().initialize(model) - atom.views.addViewProvider(Gutter, createGutterView) onDidChangeRoot: (fn) -> @emitter.on 'did-change-root', fn From b04b0a8dc45ca9d437ab00bf50f3527be05023e1 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 17 Jun 2015 13:48:31 +0200 Subject: [PATCH 2/3] :bug: Fix race condition as well --- spec/text-editor-component-spec.coffee | 9 +++++++++ src/text-editor-component.coffee | 2 +- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/spec/text-editor-component-spec.coffee b/spec/text-editor-component-spec.coffee index f9effd047..31467bed4 100644 --- a/spec/text-editor-component-spec.coffee +++ b/spec/text-editor-component-spec.coffee @@ -67,6 +67,15 @@ describe "TextEditorComponent", -> expect(nextAnimationFrame).not.toThrow() + it "doesn't update when an animation frame was requested but the component got destroyed before its delivery", -> + editor.setText("You shouldn't see this update.") + expect(nextAnimationFrame).not.toBe(noAnimationFrame) + + component.destroy() + nextAnimationFrame() + + expect(component.lineNodeForScreenRow(0).textContent).not.toBe("You shouldn't see this update.") + describe "line rendering", -> expectTileContainsRow = (tileNode, screenRow, {top}) -> lineNode = tileNode.querySelector("[data-screen-row='#{screenRow}']") diff --git a/src/text-editor-component.coffee b/src/text-editor-component.coffee index e0ab143c5..66996a996 100644 --- a/src/text-editor-component.coffee +++ b/src/text-editor-component.coffee @@ -195,7 +195,7 @@ class TextEditorComponent @updateRequested = true atom.views.updateDocument => @updateRequested = false - @updateSync() if @editor.isAlive() + @updateSync() if @canUpdate() atom.views.readDocument(@readAfterUpdateSync) canUpdate: -> From c8f24d2358603a623754fdfa4c777e8825c784c9 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Thu, 18 Jun 2015 19:54:37 +0200 Subject: [PATCH 3/3] Revert ":bug: Remove Gutter from ViewRegistry" This reverts commit b0d93accf7871c09b9d14bedb887bb25ef47d142. --- spec/custom-gutter-component-spec.coffee | 3 ++ spec/gutter-container-component-spec.coffee | 38 +++++++++------------ src/custom-gutter-component.coffee | 6 ++-- src/line-number-gutter-component.coffee | 5 +-- src/pane-container.coffee | 3 ++ 5 files changed, 30 insertions(+), 25 deletions(-) diff --git a/spec/custom-gutter-component-spec.coffee b/spec/custom-gutter-component-spec.coffee index 37da68807..4b7d81fba 100644 --- a/spec/custom-gutter-component-spec.coffee +++ b/spec/custom-gutter-component-spec.coffee @@ -16,6 +16,9 @@ describe "CustomGutterComponent", -> decorationsWrapperNode = gutterComponent.getDomNode().children.item(0) expect(decorationsWrapperNode.classList.contains('custom-decorations')).toBe true + it "makes its view accessible from the view registry", -> + expect(gutterComponent.getDomNode()).toBe atom.views.getView(gutter) + it "hides its DOM node when ::hideNode is called, and shows its DOM node when ::showNode is called", -> gutterComponent.hideNode() expect(gutterComponent.getDomNode().style.display).toBe 'none' diff --git a/spec/gutter-container-component-spec.coffee b/spec/gutter-container-component-spec.coffee index 6048655f1..5d815fea8 100644 --- a/spec/gutter-container-component-spec.coffee +++ b/spec/gutter-container-component-spec.coffee @@ -107,24 +107,22 @@ describe "GutterContainerComponent", -> gutterContainerComponent.updateSync(testState) expect(gutterContainerComponent.getDomNode().children.length).toBe 2 - - initialCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(0) - initialLineNumbersNode = gutterContainerComponent.getDomNode().children.item(1) + expectedCustomGutterNode = gutterContainerComponent.getDomNode().children.item(0) + expect(expectedCustomGutterNode).toBe atom.views.getView(customGutter1) + expectedLineNumbersNode = gutterContainerComponent.getDomNode().children.item(1) + expect(expectedLineNumbersNode).toBe atom.views.getView(lineNumberGutter) # Add a gutter. customGutter2 = new Gutter(mockGutterContainer, {name: 'custom2', priority: -10}) testState = buildTestState([customGutter1, customGutter2, lineNumberGutter]) gutterContainerComponent.updateSync(testState) - expect(gutterContainerComponent.getDomNode().children.length).toBe 3 - - unchangedCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(0) - insertedCustomGutterNode2 = gutterContainerComponent.getDomNode().children.item(1) - repositionedLineNumbersNode = gutterContainerComponent.getDomNode().children.item(2) - - expect(initialCustomGutterNode1).toBe(unchangedCustomGutterNode1) - expect(initialLineNumbersNode).toBe(repositionedLineNumbersNode) - expect(insertedCustomGutterNode2).toBeDefined() + expectedCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(0) + expect(expectedCustomGutterNode1).toBe atom.views.getView(customGutter1) + expectedCustomGutterNode2 = gutterContainerComponent.getDomNode().children.item(1) + expect(expectedCustomGutterNode2).toBe atom.views.getView(customGutter2) + expectedLineNumbersNode = gutterContainerComponent.getDomNode().children.item(2) + expect(expectedLineNumbersNode).toBe atom.views.getView(lineNumberGutter) # Hide one gutter, reposition one gutter, remove one gutter; and add a new gutter. customGutter2.hide() @@ -132,12 +130,10 @@ describe "GutterContainerComponent", -> testState = buildTestState([customGutter2, customGutter1, customGutter3]) gutterContainerComponent.updateSync(testState) expect(gutterContainerComponent.getDomNode().children.length).toBe 3 - - repositionedCustomGutterNode2 = gutterContainerComponent.getDomNode().children.item(0) - repositionedCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(1) - insertedCustomGutterNode3 = gutterContainerComponent.getDomNode().children.item(2) - - expect(initialCustomGutterNode1).toBe(repositionedCustomGutterNode1) - expect(repositionedCustomGutterNode2).toBe(insertedCustomGutterNode2) - expect(repositionedCustomGutterNode2.style.display).toBe('none') - expect(insertedCustomGutterNode3).toBeDefined() + expectedCustomGutterNode2 = gutterContainerComponent.getDomNode().children.item(0) + expect(expectedCustomGutterNode2).toBe atom.views.getView(customGutter2) + expect(expectedCustomGutterNode2.style.display).toBe 'none' + expectedCustomGutterNode1 = gutterContainerComponent.getDomNode().children.item(1) + expect(expectedCustomGutterNode1).toBe atom.views.getView(customGutter1) + expectedCustomGutterNode3 = gutterContainerComponent.getDomNode().children.item(2) + expect(expectedCustomGutterNode3).toBe atom.views.getView(customGutter3) diff --git a/src/custom-gutter-component.coffee b/src/custom-gutter-component.coffee index 5694f6182..39f5a80a1 100644 --- a/src/custom-gutter-component.coffee +++ b/src/custom-gutter-component.coffee @@ -1,4 +1,4 @@ -{createGutterView, setDimensionsAndBackground} = require './gutter-component-helpers' +{setDimensionsAndBackground} = require './gutter-component-helpers' # This class represents a gutter other than the 'line-numbers' gutter. # The contents of this gutter may be specified by Decorations. @@ -11,8 +11,10 @@ class CustomGutterComponent @decorationItemsById = {} @visible = true - @domNode = createGutterView(@gutter) + @domNode = atom.views.getView(@gutter) @decorationsNode = @domNode.firstChild + # Clear the contents in case the domNode is being reused. + @decorationsNode.innerHTML = '' getDomNode: -> @domNode diff --git a/src/line-number-gutter-component.coffee b/src/line-number-gutter-component.coffee index 4efcf8dd7..351275f63 100644 --- a/src/line-number-gutter-component.coffee +++ b/src/line-number-gutter-component.coffee @@ -1,5 +1,5 @@ _ = require 'underscore-plus' -{createGutterView, setDimensionsAndBackground} = require './gutter-component-helpers' +{setDimensionsAndBackground} = require './gutter-component-helpers' WrapperDiv = document.createElement('div') @@ -11,8 +11,9 @@ class LineNumberGutterComponent @lineNumberNodesById = {} @visible = true - @domNode = createGutterView(@gutter) + @domNode = atom.views.getView(@gutter) @lineNumbersNode = @domNode.firstChild + @lineNumbersNode.innerHTML = '' @domNode.addEventListener 'click', @onClick @domNode.addEventListener 'mousedown', @onMouseDown diff --git a/src/pane-container.coffee b/src/pane-container.coffee index e6eb97075..26ef22cac 100644 --- a/src/pane-container.coffee +++ b/src/pane-container.coffee @@ -2,6 +2,8 @@ Grim = require 'grim' {Emitter, CompositeDisposable} = require 'event-kit' Serializable = require 'serializable' +{createGutterView} = require './gutter-component-helpers' +Gutter = require './gutter' Model = require './model' Pane = require './pane' PaneElement = require './pane-element' @@ -60,6 +62,7 @@ class PaneContainer extends Model new PaneElement().initialize(model) atom.views.addViewProvider TextEditor, (model) -> new TextEditorElement().initialize(model) + atom.views.addViewProvider(Gutter, createGutterView) onDidChangeRoot: (fn) -> @emitter.on 'did-change-root', fn