From 7d46fe6c287f4788aaf2e9d4fab3f14e1b9b1e26 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 4 Nov 2015 13:58:10 -0800 Subject: [PATCH 1/8] :fire: Unused properties on TextEditor.prototype --- src/text-editor.coffee | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/text-editor.coffee b/src/text-editor.coffee index c0c4dbe14..82169040f 100644 --- a/src/text-editor.coffee +++ b/src/text-editor.coffee @@ -54,13 +54,11 @@ GutterContainer = require './gutter-container' # soft wraps and folds to ensure your code interacts with them correctly. module.exports = class TextEditor extends Model - callDisplayBufferCreatedHook: false buffer: null languageMode: null cursors: null selections: null suppressSelectionMerging: false - updateBatchDepth: 0 selectionFlashDuration: 500 gutterContainer: null From 4d40e28c6b8d21e8cdc67dea6bdd99c453522782 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 4 Nov 2015 16:58:47 -0800 Subject: [PATCH 2/8] Remove presenter constructor parameters that aren't used in production --- spec/text-editor-presenter-spec.coffee | 91 ++++++++++++++++++-------- src/text-editor-presenter.coffee | 13 ++-- 2 files changed, 69 insertions(+), 35 deletions(-) diff --git a/spec/text-editor-presenter-spec.coffee b/spec/text-editor-presenter-spec.coffee index 1804b7b6b..8d7279610 100644 --- a/spec/text-editor-presenter-spec.coffee +++ b/spec/text-editor-presenter-spec.coffee @@ -25,27 +25,36 @@ describe "TextEditorPresenter", -> editor.destroy() buffer.destroy() - buildPresenter = (params={}) -> + buildPresenterWithoutMeasurements = (params={}) -> _.defaults params, model: editor - explicitHeight: 130 - contentFrameWidth: 500 - windowWidth: 500 - windowHeight: 130 - boundingClientRect: {left: 0, top: 0, width: 500, height: 130} - gutterWidth: 0 - lineHeight: 10 - baseCharacterWidth: 10 - horizontalScrollbarHeight: 10 - verticalScrollbarWidth: 10 - scrollTop: 0 - scrollLeft: 0 config: atom.config - + contentFrameWidth: 500 presenter = new TextEditorPresenter(params) presenter.setLinesYardstick(new FakeLinesYardstick(editor, presenter)) presenter + buildPresenter = (params={}) -> + presenter = buildPresenterWithoutMeasurements(_.defaults(params, + scrollTop: 0 + scrollLeft: 0 + )) + + presenter.setExplicitHeight(params.explicitHeight ? 130) + presenter.setWindowSize(params.windowWidth ? 500, params.windowHeight ? 130) + presenter.setBoundingClientRect(params.boundingClientRect ? { + left: 0 + top: 0 + width: 500 + height: 130 + }) + presenter.setGutterWidth(params.gutterWidth ? 0) + presenter.setLineHeight(params.lineHeight ? 10) + presenter.setBaseCharacterWidth(params.baseCharacterWidth ? 10) + presenter.setHorizontalScrollbarHeight(params.horizontalScrollbarHeight ? 10) + presenter.setVerticalScrollbarWidth(params.verticalScrollbarWidth ? 10) + presenter + expectValues = (actual, expected) -> for key, value of expected expect(actual[key]).toEqual value @@ -160,7 +169,7 @@ describe "TextEditorPresenter", -> expect(stateFn(presenter).tiles[12]).toBeDefined() it "is empty until all of the required measurements are assigned", -> - presenter = buildPresenter(explicitHeight: null, lineHeight: null, scrollTop: null) + presenter = buildPresenterWithoutMeasurements() expect(stateFn(presenter).tiles).toEqual({}) presenter.setExplicitHeight(25) @@ -999,20 +1008,25 @@ describe "TextEditorPresenter", -> describe ".backgroundColor", -> it "is assigned to ::backgroundColor unless the editor is mini", -> - presenter = buildPresenter(backgroundColor: 'rgba(255, 0, 0, 0)') + presenter = buildPresenter() + presenter.setBackgroundColor('rgba(255, 0, 0, 0)') expect(presenter.getState().content.backgroundColor).toBe 'rgba(255, 0, 0, 0)' + editor.setMini(true) - presenter = buildPresenter(backgroundColor: 'rgba(255, 0, 0, 0)') + presenter = buildPresenter() + presenter.setBackgroundColor('rgba(255, 0, 0, 0)') expect(presenter.getState().content.backgroundColor).toBeNull() it "updates when ::backgroundColor changes", -> - presenter = buildPresenter(backgroundColor: 'rgba(255, 0, 0, 0)') + presenter = buildPresenter() + presenter.setBackgroundColor('rgba(255, 0, 0, 0)') expect(presenter.getState().content.backgroundColor).toBe 'rgba(255, 0, 0, 0)' expectStateUpdate presenter, -> presenter.setBackgroundColor('rgba(0, 0, 255, 0)') expect(presenter.getState().content.backgroundColor).toBe 'rgba(0, 0, 255, 0)' it "updates when ::mini changes", -> - presenter = buildPresenter(backgroundColor: 'rgba(255, 0, 0, 0)') + presenter = buildPresenter() + presenter.setBackgroundColor('rgba(255, 0, 0, 0)') expect(presenter.getState().content.backgroundColor).toBe 'rgba(255, 0, 0, 0)' expectStateUpdate presenter, -> editor.setMini(true) expect(presenter.getState().content.backgroundColor).toBeNull() @@ -1040,6 +1054,7 @@ describe "TextEditorPresenter", -> describe "[tileId].lines[lineId]", -> # line state objects it "includes the state for visible lines in a tile", -> presenter = buildPresenter(explicitHeight: 3, scrollTop: 4, lineHeight: 1, tileSize: 3, stoppedScrollingDelay: 200) + presenter.setExplicitHeight(3) expect(lineStateForScreenRow(presenter, 2)).toBeUndefined() @@ -1293,7 +1308,7 @@ describe "TextEditorPresenter", -> expect(stateForCursor(presenter, 4)).toBeUndefined() it "is empty until all of the required measurements are assigned", -> - presenter = buildPresenter(explicitHeight: null, lineHeight: null, scrollTop: null, baseCharacterWidth: null, horizontalScrollbarHeight: null) + presenter = buildPresenterWithoutMeasurements() expect(presenter.getState().content.cursors).toEqual({}) presenter.setExplicitHeight(25) @@ -1308,6 +1323,15 @@ describe "TextEditorPresenter", -> presenter.setBaseCharacterWidth(8) expect(presenter.getState().content.cursors).toEqual({}) + presenter.setBoundingClientRect(top: 0, left: 0, width: 500, height: 130) + expect(presenter.getState().content.cursors).toEqual({}) + + presenter.setWindowSize(500, 130) + expect(presenter.getState().content.cursors).toEqual({}) + + presenter.setVerticalScrollbarWidth(10) + expect(presenter.getState().content.cursors).toEqual({}) + presenter.setHorizontalScrollbarHeight(10) expect(presenter.getState().content.cursors).not.toEqual({}) @@ -1439,7 +1463,8 @@ describe "TextEditorPresenter", -> it "alternates between true and false twice per ::cursorBlinkPeriod when the editor is focused", -> cursorBlinkPeriod = 100 cursorBlinkResumeDelay = 200 - presenter = buildPresenter({cursorBlinkPeriod, cursorBlinkResumeDelay, focused: true}) + presenter = buildPresenter({cursorBlinkPeriod, cursorBlinkResumeDelay}) + presenter.setFocused(true) expect(presenter.getState().content.cursorsVisible).toBe true expectStateUpdate presenter, -> advanceClock(cursorBlinkPeriod / 2) @@ -1466,7 +1491,8 @@ describe "TextEditorPresenter", -> it "stops alternating for ::cursorBlinkResumeDelay when a cursor moves or a cursor is added", -> cursorBlinkPeriod = 100 cursorBlinkResumeDelay = 200 - presenter = buildPresenter({cursorBlinkPeriod, cursorBlinkResumeDelay, focused: true}) + presenter = buildPresenter({cursorBlinkPeriod, cursorBlinkResumeDelay}) + presenter.setFocused(true) expect(presenter.getState().content.cursorsVisible).toBe true expectStateUpdate presenter, -> advanceClock(cursorBlinkPeriod / 2) @@ -1616,7 +1642,7 @@ describe "TextEditorPresenter", -> [[0, 2], [2, 4]], ]) - presenter = buildPresenter(explicitHeight: null, lineHeight: null, scrollTop: null, baseCharacterWidth: null, tileSize: 2) + presenter = buildPresenterWithoutMeasurements(tileSize: 2) for tileId, tileState of presenter.getState().content.tiles expect(tileState.highlights).toEqual({}) @@ -1925,7 +1951,7 @@ describe "TextEditorPresenter", -> marker = editor.markBufferRange([[2, 13], [4, 14]], invalidate: 'touch') decoration = editor.decorateMarker(marker, {type: 'overlay', position: 'tail', item}) - presenter = buildPresenter(baseCharacterWidth: null, lineHeight: null, windowWidth: null, windowHeight: null, boundingClientRect: null) + presenter = buildPresenterWithoutMeasurements() expect(presenter.getState().content.overlays).toEqual({}) presenter.setBaseCharacterWidth(10) @@ -1937,6 +1963,12 @@ describe "TextEditorPresenter", -> presenter.setWindowSize(500, 100) expect(presenter.getState().content.overlays).toEqual({}) + presenter.setVerticalScrollbarWidth(10) + expect(presenter.getState().content.overlays).toEqual({}) + + presenter.setHorizontalScrollbarHeight(10) + expect(presenter.getState().content.overlays).toEqual({}) + presenter.setBoundingClientRect({top: 0, left: 0, height: 100, width: 500}) expect(presenter.getState().content.overlays).not.toEqual({}) @@ -2123,7 +2155,8 @@ describe "TextEditorPresenter", -> expect(editor.getRowsPerPage()).toBe(20) it "tracks the computed content height if ::autoHeight is true so the editor auto-expands vertically", -> - presenter = buildPresenter(explicitHeight: null, autoHeight: true) + presenter = buildPresenter(explicitHeight: null) + presenter.setAutoHeight(true) expect(presenter.getState().height).toBe editor.getScreenLineCount() * 10 expectStateUpdate presenter, -> presenter.setAutoHeight(false) @@ -2140,7 +2173,9 @@ describe "TextEditorPresenter", -> describe ".focused", -> it "tracks the value of ::focused", -> - presenter = buildPresenter(focused: false) + presenter = buildPresenter() + presenter.setFocused(false) + expect(presenter.getState().focused).toBe false expectStateUpdate presenter, -> presenter.setFocused(true) expect(presenter.getState().focused).toBe true @@ -2819,7 +2854,9 @@ describe "TextEditorPresenter", -> describe ".backgroundColor", -> it "is assigned to ::gutterBackgroundColor if present, and to ::backgroundColor otherwise", -> - presenter = buildPresenter(backgroundColor: "rgba(255, 0, 0, 0)", gutterBackgroundColor: "rgba(0, 255, 0, 0)") + presenter = buildPresenter() + presenter.setBackgroundColor("rgba(255, 0, 0, 0)") + presenter.setGutterBackgroundColor("rgba(0, 255, 0, 0)") expect(getStylesForGutterWithName(presenter, 'line-number').backgroundColor).toBe "rgba(0, 255, 0, 0)" expect(getStylesForGutterWithName(presenter, 'test-gutter').backgroundColor).toBe "rgba(0, 255, 0, 0)" diff --git a/src/text-editor-presenter.coffee b/src/text-editor-presenter.coffee index 2e1d73c56..10fcfd6d3 100644 --- a/src/text-editor-presenter.coffee +++ b/src/text-editor-presenter.coffee @@ -13,15 +13,12 @@ class TextEditorPresenter minimumReflowInterval: 200 constructor: (params) -> - {@model, @config, @autoHeight, @explicitHeight, @contentFrameWidth, @scrollTop, @scrollLeft, @scrollColumn, @scrollRow, @boundingClientRect, @windowWidth, @windowHeight, @gutterWidth} = params - {horizontalScrollbarHeight, verticalScrollbarWidth} = params - {@lineHeight, @baseCharacterWidth, @backgroundColor, @gutterBackgroundColor, @tileSize} = params - {@cursorBlinkPeriod, @cursorBlinkResumeDelay, @stoppedScrollingDelay, @focused} = params - @measuredHorizontalScrollbarHeight = horizontalScrollbarHeight - @measuredVerticalScrollbarWidth = verticalScrollbarWidth - @gutterWidth ?= 0 - @tileSize ?= 6 + {@model, @config} = params + {@cursorBlinkPeriod, @cursorBlinkResumeDelay, @stoppedScrollingDelay, @tileSize} = params + {@contentFrameWidth, @scrollTop, @scrollLeft, @scrollColumn, @scrollRow} = params + @gutterWidth = 0 + @tileSize ?= 6 @realScrollTop = @scrollTop @realScrollLeft = @scrollLeft @disposables = new CompositeDisposable From b58752da38d8fd03f01f8ccd3463b9d57074feaa Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 4 Nov 2015 17:39:31 -0800 Subject: [PATCH 3/8] Keep model's logical scroll position up to date * Remove scrollRow and scrollColumn properties from the presenter * Assign presenter's scrollTop and scrollLeft based on model's first visible screen row and column, once the presenter has the required measurements. --- spec/text-editor-presenter-spec.coffee | 28 +++++++++++++------------- src/text-editor-component.coffee | 4 ---- src/text-editor-presenter.coffee | 21 +++++++------------ src/text-editor.coffee | 17 +++++++++------- 4 files changed, 31 insertions(+), 39 deletions(-) diff --git a/spec/text-editor-presenter-spec.coffee b/spec/text-editor-presenter-spec.coffee index 8d7279610..ae9dfb6a6 100644 --- a/spec/text-editor-presenter-spec.coffee +++ b/spec/text-editor-presenter-spec.coffee @@ -35,11 +35,9 @@ describe "TextEditorPresenter", -> presenter buildPresenter = (params={}) -> - presenter = buildPresenterWithoutMeasurements(_.defaults(params, - scrollTop: 0 - scrollLeft: 0 - )) - + presenter = buildPresenterWithoutMeasurements(params) + presenter.setScrollTop(params.scrollTop) if params.scrollTop? + presenter.setScrollLeft(params.scrollLeft) if params.scrollLeft? presenter.setExplicitHeight(params.explicitHeight ? 130) presenter.setWindowSize(params.windowWidth ? 500, params.windowHeight ? 130) presenter.setBoundingClientRect(params.boundingClientRect ? { @@ -175,10 +173,8 @@ describe "TextEditorPresenter", -> presenter.setExplicitHeight(25) expect(stateFn(presenter).tiles).toEqual({}) + # Sets scroll row from model's logical position presenter.setLineHeight(10) - expect(stateFn(presenter).tiles).toEqual({}) - - presenter.setScrollTop(0) expect(stateFn(presenter).tiles).not.toEqual({}) it "updates when ::scrollTop changes", -> @@ -621,6 +617,8 @@ describe "TextEditorPresenter", -> describe ".scrollingVertically", -> it "is true for ::stoppedScrollingDelay milliseconds following a changes to ::scrollTop", -> presenter = buildPresenter(scrollTop: 10, stoppedScrollingDelay: 200, explicitHeight: 100) + expect(presenter.getState().content.scrollingVertically).toBe true + advanceClock(300) expect(presenter.getState().content.scrollingVertically).toBe false expectStateUpdate presenter, -> presenter.setScrollTop(0) expect(presenter.getState().content.scrollingVertically).toBe true @@ -763,7 +761,8 @@ describe "TextEditorPresenter", -> expect(presenter.getState().content.scrollTop).toBe(10) it "corresponds to the passed logical coordinates when building the presenter", -> - presenter = buildPresenter(scrollRow: 4, lineHeight: 10, explicitHeight: 20) + editor.setFirstVisibleScreenRow(4) + presenter = buildPresenter(lineHeight: 10, explicitHeight: 20) expect(presenter.getState().content.scrollTop).toBe(40) it "tracks the value of ::scrollTop", -> @@ -777,11 +776,11 @@ describe "TextEditorPresenter", -> expectStateUpdate presenter, -> presenter.setScrollTop(50) presenter.getState() # commits scroll position - expect(editor.getScrollRow()).toBe(5) + expect(editor.getFirstVisibleScreenRow()).toBe 5 expectStateUpdate presenter, -> presenter.setScrollTop(57) presenter.getState() # commits scroll position - expect(editor.getScrollRow()).toBe(6) + expect(editor.getFirstVisibleScreenRow()).toBe 6 it "reassigns the scrollTop if it exceeds the max possible value after lines are removed", -> presenter = buildPresenter(scrollTop: 80, lineHeight: 10, explicitHeight: 50, horizontalScrollbarHeight: 0) @@ -890,7 +889,8 @@ describe "TextEditorPresenter", -> expect(presenter.getState().content.scrollLeft).toBe(50) it "corresponds to the passed logical coordinates when building the presenter", -> - presenter = buildPresenter(scrollColumn: 3, lineHeight: 10, baseCharacterWidth: 10, verticalScrollbarWidth: 10, contentFrameWidth: 500) + editor.setFirstVisibleScreenColumn(3) + presenter = buildPresenter(lineHeight: 10, baseCharacterWidth: 10, verticalScrollbarWidth: 10, contentFrameWidth: 500) expect(presenter.getState().content.scrollLeft).toBe(30) it "tracks the value of ::scrollLeft", -> @@ -904,11 +904,11 @@ describe "TextEditorPresenter", -> expectStateUpdate presenter, -> presenter.setScrollLeft(50) presenter.getState() # commits scroll position - expect(editor.getScrollColumn()).toBe(5) + expect(editor.getFirstVisibleScreenColumn()).toBe 5 expectStateUpdate presenter, -> presenter.setScrollLeft(57) presenter.getState() # commits scroll position - expect(editor.getScrollColumn()).toBe(6) + expect(editor.getFirstVisibleScreenColumn()).toBe 6 it "is always rounded to the nearest integer", -> presenter = buildPresenter(scrollLeft: 10, lineHeight: 10, baseCharacterWidth: 10, verticalScrollbarWidth: 10, contentFrameWidth: 500) diff --git a/src/text-editor-component.coffee b/src/text-editor-component.coffee index d72c50382..dc0abbe60 100644 --- a/src/text-editor-component.coffee +++ b/src/text-editor-component.coffee @@ -50,10 +50,6 @@ class TextEditorComponent @presenter = new TextEditorPresenter model: @editor - scrollTop: 0 - scrollLeft: 0 - scrollRow: @editor.getScrollRow() - scrollColumn: @editor.getScrollColumn() tileSize: tileSize cursorBlinkPeriod: @cursorBlinkPeriod cursorBlinkResumeDelay: @cursorBlinkResumeDelay diff --git a/src/text-editor-presenter.coffee b/src/text-editor-presenter.coffee index 10fcfd6d3..bda61ab87 100644 --- a/src/text-editor-presenter.coffee +++ b/src/text-editor-presenter.coffee @@ -15,7 +15,7 @@ class TextEditorPresenter constructor: (params) -> {@model, @config} = params {@cursorBlinkPeriod, @cursorBlinkResumeDelay, @stoppedScrollingDelay, @tileSize} = params - {@contentFrameWidth, @scrollTop, @scrollLeft, @scrollColumn, @scrollRow} = params + {@contentFrameWidth} = params @gutterWidth = 0 @tileSize ?= 6 @@ -72,7 +72,6 @@ class TextEditorPresenter @updateVerticalDimensions() @updateScrollbarDimensions() - @restoreScrollPosition() @commitPendingLogicalScrollTopPosition() @commitPendingScrollTopPosition() @@ -777,8 +776,7 @@ class TextEditorPresenter if scrollTop isnt @realScrollTop and not Number.isNaN(scrollTop) @realScrollTop = scrollTop @scrollTop = Math.round(scrollTop) - @scrollRow = Math.round(@scrollTop / @lineHeight) - @model.setScrollRow(@scrollRow) + @model.setFirstVisibleScreenRow(Math.round(@scrollTop / @lineHeight)) @updateStartRow() @updateEndRow() @@ -794,8 +792,7 @@ class TextEditorPresenter if scrollLeft isnt @realScrollLeft and not Number.isNaN(scrollLeft) @realScrollLeft = scrollLeft @scrollLeft = Math.round(scrollLeft) - @scrollColumn = Math.round(@scrollLeft / @baseCharacterWidth) - @model.setScrollColumn(@scrollColumn) + @model.setFirstVisibleScreenColumn(Math.round(@scrollLeft / @baseCharacterWidth)) @emitter.emit 'did-change-scroll-left', @scrollLeft @@ -1106,6 +1103,8 @@ class TextEditorPresenter setLineHeight: (lineHeight) -> unless @lineHeight is lineHeight @lineHeight = lineHeight + unless @scrollTop? + @updateScrollTop(@model.getFirstVisibleScreenRow() * lineHeight) @model.setLineHeightInPixels(lineHeight) @shouldUpdateHeightState = true @shouldUpdateHorizontalScrollState = true @@ -1133,6 +1132,8 @@ class TextEditorPresenter @halfWidthCharWidth = halfWidthCharWidth @koreanCharWidth = koreanCharWidth @model.setDefaultCharWidth(baseCharacterWidth, doubleWidthCharWidth, halfWidthCharWidth, koreanCharWidth) + unless @scrollLeft? + @updateScrollLeft(@model.getFirstVisibleScreenColumn() * baseCharacterWidth) @characterWidthsChanged() characterWidthsChanged: -> @@ -1617,14 +1618,6 @@ class TextEditorPresenter @updateScrollTop(@pendingScrollTop) @pendingScrollTop = null - restoreScrollPosition: -> - return if @hasRestoredScrollPosition or not @hasPixelPositionRequirements() - - @setScrollTop(@scrollRow * @lineHeight) if @scrollRow? - @setScrollLeft(@scrollColumn * @baseCharacterWidth) if @scrollColumn? - - @hasRestoredScrollPosition = true - clearPendingScrollPosition: -> @pendingScrollLogicalPosition = null @pendingScrollTop = null diff --git a/src/text-editor.coffee b/src/text-editor.coffee index 82169040f..327647cb7 100644 --- a/src/text-editor.coffee +++ b/src/text-editor.coffee @@ -87,7 +87,7 @@ class TextEditor extends Model super { - @softTabs, @scrollRow, @scrollColumn, initialLine, initialColumn, tabLength, + @softTabs, @firstVisibleScreenRow, @firstVisibleScreenColumn, initialLine, initialColumn, tabLength, softWrapped, @displayBuffer, buffer, suppressCursorCreation, @mini, @placeholderText, lineNumberGutterVisible, largeFileMode, @config, @notificationManager, @packageManager, @clipboard, @viewRegistry, @grammarRegistry, @project, @assert, @applicationDelegate @@ -102,6 +102,8 @@ class TextEditor extends Model throw new Error("Must pass a project parameter when constructing TextEditors") unless @project? throw new Error("Must pass an assert parameter when constructing TextEditors") unless @assert? + @firstVisibleScreenRow ?= 0 + @firstVisibleScreenColumn ?= 0 @emitter = new Emitter @disposables = new CompositeDisposable @cursors = [] @@ -141,8 +143,8 @@ class TextEditor extends Model deserializer: 'TextEditor' id: @id softTabs: @softTabs - scrollRow: @getScrollRow() - scrollColumn: @getScrollColumn() + firstVisibleScreenRow: @getFirstVisibleScreenRow() + firstVisibleScreenColumn: @getFirstVisibleScreenColumn() displayBuffer: @displayBuffer.serialize() subscribeToBuffer: -> @@ -3086,11 +3088,12 @@ class TextEditor extends Model Grim.deprecate("This is now a view method. Call TextEditorElement::getWidth instead.") @displayBuffer.getWidth() - getScrollRow: -> @scrollRow - setScrollRow: (@scrollRow) -> + setFirstVisibleScreenRow: (@firstVisibleScreenRow) -> + getFirstVisibleScreenRow: -> @firstVisibleScreenRow + + setFirstVisibleScreenColumn: (@firstVisibleScreenColumn) -> + getFirstVisibleScreenColumn: -> @firstVisibleScreenColumn - getScrollColumn: -> @scrollColumn - setScrollColumn: (@scrollColumn) -> getScrollTop: -> Grim.deprecate("This is now a view method. Call TextEditorElement::getScrollTop instead.") From 48cc5e713ea6f5110f680401b3bcdf287861a601 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 9 Nov 2015 10:13:09 -0800 Subject: [PATCH 4/8] Make presenter respond to external changes to model's first visible screen row --- spec/text-editor-spec.coffee | 65 ++++++++++++++++++++++++++++++++ src/text-editor-presenter.coffee | 6 ++- src/text-editor.coffee | 33 +++++++++++++--- 3 files changed, 98 insertions(+), 6 deletions(-) diff --git a/spec/text-editor-spec.coffee b/spec/text-editor-spec.coffee index ed299ab54..77a2ba30a 100644 --- a/spec/text-editor-spec.coffee +++ b/spec/text-editor-spec.coffee @@ -4784,6 +4784,71 @@ describe "TextEditor", -> editor.selectPageUp() expect(editor.getSelectedBufferRanges()).toEqual [[[0, 0], [12, 2]]] + describe "::setFirstVisibleScreenRow() and ::getFirstVisibleScreenRow()", -> + beforeEach -> + line = Array(9).join('0123456789') + editor.setText([1..100].map(-> line).join('\n')) + expect(editor.getLineCount()).toBe 100 + expect(editor.lineTextForBufferRow(0).length).toBe 80 + + describe "when the editor doesn't have a height and lineHeightInPixels", -> + it "does not affect the editor's visible row range", -> + expect(editor.getVisibleRowRange()).toBeNull() + + editor.setFirstVisibleScreenRow(1) + expect(editor.getFirstVisibleScreenRow()).toEqual 1 + + editor.setFirstVisibleScreenRow(3) + expect(editor.getFirstVisibleScreenRow()).toEqual 3 + + expect(editor.getVisibleRowRange()).toBeNull() + + describe "when the editor has a height and lineHeightInPixels", -> + beforeEach -> + atom.config.set('editor.scrollPastEnd', true) + editor.setHeight(100, true) + editor.setLineHeightInPixels(10) + + it "updates the editor's visible row range", -> + editor.setFirstVisibleScreenRow(2) + expect(editor.getFirstVisibleScreenRow()).toEqual 2 + expect(editor.getVisibleRowRange()).toEqual [2, 12] + + it "notifies ::onDidChangeFirstVisibleScreenRow observers", -> + changeCount = 0 + editor.onDidChangeFirstVisibleScreenRow -> changeCount++ + + editor.setFirstVisibleScreenRow(2) + expect(changeCount).toBe 1 + + editor.setFirstVisibleScreenRow(2) + expect(changeCount).toBe 1 + + editor.setFirstVisibleScreenRow(3) + expect(changeCount).toBe 2 + + it "ensures that the top row is less than the buffer's line count", -> + editor.setFirstVisibleScreenRow(102) + expect(editor.getFirstVisibleScreenRow()).toEqual 99 + expect(editor.getVisibleRowRange()).toEqual [99, 99] + + it "ensures that the left column is less than the length of the longest screen line", -> + editor.setFirstVisibleScreenRow(10) + expect(editor.getFirstVisibleScreenRow()).toEqual 10 + + editor.setText("\n\n\n") + + editor.setFirstVisibleScreenRow(10) + expect(editor.getFirstVisibleScreenRow()).toEqual 3 + + describe "when the 'editor.scrollPastEnd' option is set to false", -> + it "ensures that the bottom row is less than the buffer's line count", -> + atom.config.set('editor.scrollPastEnd', false) + + editor.setFirstVisibleScreenRow(95) + expect(editor.getFirstVisibleScreenRow()).toEqual 89 + expect(editor.getVisibleRowRange()).toEqual [89, 99] + describe '.get/setPlaceholderText()', -> it 'can be created with placeholderText', -> newEditor = atom.workspace.buildTextEditor( diff --git a/src/text-editor-presenter.coffee b/src/text-editor-presenter.coffee index bda61ab87..3b679b319 100644 --- a/src/text-editor-presenter.coffee +++ b/src/text-editor-presenter.coffee @@ -213,6 +213,7 @@ class TextEditorPresenter @disposables.add @model.onDidAddDecoration(@didAddDecoration.bind(this)) @disposables.add @model.onDidAddCursor(@didAddCursor.bind(this)) @disposables.add @model.onDidRequestAutoscroll(@requestAutoscroll.bind(this)) + @disposables.add @model.onDidChangeFirstVisibleScreenRow(@didChangeFirstVisibleScreenRow.bind(this)) @observeDecoration(decoration) for decoration in @model.getDecorations() @observeCursor(cursor) for cursor in @model.getCursors() @disposables.add @model.onDidAddGutter(@didAddGutter.bind(this)) @@ -776,7 +777,7 @@ class TextEditorPresenter if scrollTop isnt @realScrollTop and not Number.isNaN(scrollTop) @realScrollTop = scrollTop @scrollTop = Math.round(scrollTop) - @model.setFirstVisibleScreenRow(Math.round(@scrollTop / @lineHeight)) + @model.setFirstVisibleScreenRow(Math.round(@scrollTop / @lineHeight), true) @updateStartRow() @updateEndRow() @@ -1539,6 +1540,9 @@ class TextEditorPresenter @emitDidUpdateState() + didChangeFirstVisibleScreenRow: (screenRow) -> + @updateScrollTop(screenRow * @lineHeight) + getVerticalScrollMarginInPixels: -> Math.round(@model.getVerticalScrollMargin() * @lineHeight) diff --git a/src/text-editor.coffee b/src/text-editor.coffee index 327647cb7..0e678b066 100644 --- a/src/text-editor.coffee +++ b/src/text-editor.coffee @@ -448,6 +448,9 @@ class TextEditor extends Model onDidChangeCharacterWidths: (callback) -> @displayBuffer.onDidChangeCharacterWidths(callback) + onDidChangeFirstVisibleScreenRow: (callback, fromView) -> + @emitter.on 'did-change-first-visible-screen-row', callback + onDidChangeScrollTop: (callback) -> Grim.deprecate("This is now a view method. Call TextEditorElement::onDidChangeScrollTop instead.") @@ -3088,13 +3091,27 @@ class TextEditor extends Model Grim.deprecate("This is now a view method. Call TextEditorElement::getWidth instead.") @displayBuffer.getWidth() - setFirstVisibleScreenRow: (@firstVisibleScreenRow) -> + # Experimental: Scroll the editor such that the given screen row is at the + # top of the visible area. + setFirstVisibleScreenRow: (screenRow, fromView) -> + unless fromView + maxScreenRow = @getLineCount() - 1 + unless @config.get('editor.scrollPastEnd') + height = @displayBuffer.getHeight() + lineHeightInPixels = @displayBuffer.getLineHeightInPixels() + if height? and lineHeightInPixels? + maxScreenRow -= Math.floor(height / lineHeightInPixels) + screenRow = Math.min(screenRow, maxScreenRow) + + unless screenRow is @firstVisibleScreenRow + @emitter.emit 'did-change-first-visible-screen-row', screenRow unless fromView + @firstVisibleScreenRow = screenRow + getFirstVisibleScreenRow: -> @firstVisibleScreenRow setFirstVisibleScreenColumn: (@firstVisibleScreenColumn) -> getFirstVisibleScreenColumn: -> @firstVisibleScreenColumn - getScrollTop: -> Grim.deprecate("This is now a view method. Call TextEditorElement::getScrollTop instead.") @@ -3151,9 +3168,15 @@ class TextEditor extends Model @viewRegistry.getView(this).getMaxScrollTop() getVisibleRowRange: -> - Grim.deprecate("This is now a view method. Call TextEditorElement::getVisibleRowRange instead.") - - @viewRegistry.getView(this).getVisibleRowRange() + height = @displayBuffer.getHeight() + lineHeightInPixels = @displayBuffer.getLineHeightInPixels() + if height? and lineHeightInPixels? + [ + @firstVisibleScreenRow, + Math.min(@firstVisibleScreenRow + Math.floor(height / lineHeightInPixels), @getLineCount() - 1) + ] + else + null intersectsVisibleRowRange: (startRow, endRow) -> Grim.deprecate("This is now a view method. Call TextEditorElement::intersectsVisibleRowRange instead.") From 130464836194e257d1d87a471051ea20c817b9a4 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Mon, 9 Nov 2015 11:00:38 -0800 Subject: [PATCH 5/8] Make getLastVisibleScreenRow a model API again --- spec/text-editor-spec.coffee | 2 ++ src/text-editor.coffee | 35 +++++++++++++++-------------------- 2 files changed, 17 insertions(+), 20 deletions(-) diff --git a/spec/text-editor-spec.coffee b/spec/text-editor-spec.coffee index 77a2ba30a..b0c40d588 100644 --- a/spec/text-editor-spec.coffee +++ b/spec/text-editor-spec.coffee @@ -4802,6 +4802,7 @@ describe "TextEditor", -> expect(editor.getFirstVisibleScreenRow()).toEqual 3 expect(editor.getVisibleRowRange()).toBeNull() + expect(editor.getLastVisibleScreenRow()).toBeNull() describe "when the editor has a height and lineHeightInPixels", -> beforeEach -> @@ -4812,6 +4813,7 @@ describe "TextEditor", -> it "updates the editor's visible row range", -> editor.setFirstVisibleScreenRow(2) expect(editor.getFirstVisibleScreenRow()).toEqual 2 + expect(editor.getLastVisibleScreenRow()).toBe 12 expect(editor.getVisibleRowRange()).toEqual [2, 12] it "notifies ::onDidChangeFirstVisibleScreenRow observers", -> diff --git a/src/text-editor.coffee b/src/text-editor.coffee index 0e678b066..27376c7f2 100644 --- a/src/text-editor.coffee +++ b/src/text-editor.coffee @@ -3029,14 +3029,6 @@ class TextEditor extends Model @placeholderText = placeholderText @emitter.emit 'did-change-placeholder-text', @placeholderText - getFirstVisibleScreenRow: -> - deprecate("This is now a view method. Call TextEditorElement::getFirstVisibleScreenRow instead.") - @viewRegistry.getView(this).getVisibleRowRange()[0] - - getLastVisibleScreenRow: -> - Grim.deprecate("This is now a view method. Call TextEditorElement::getLastVisibleScreenRow instead.") - @viewRegistry.getView(this).getVisibleRowRange()[1] - pixelPositionForBufferPosition: (bufferPosition) -> Grim.deprecate("This method is deprecated on the model layer. Use `TextEditorElement::pixelPositionForBufferPosition` instead") @viewRegistry.getView(this).pixelPositionForBufferPosition(bufferPosition) @@ -3101,7 +3093,7 @@ class TextEditor extends Model lineHeightInPixels = @displayBuffer.getLineHeightInPixels() if height? and lineHeightInPixels? maxScreenRow -= Math.floor(height / lineHeightInPixels) - screenRow = Math.min(screenRow, maxScreenRow) + screenRow = Math.max(Math.min(screenRow, maxScreenRow), 0) unless screenRow is @firstVisibleScreenRow @emitter.emit 'did-change-first-visible-screen-row', screenRow unless fromView @@ -3109,6 +3101,20 @@ class TextEditor extends Model getFirstVisibleScreenRow: -> @firstVisibleScreenRow + getLastVisibleScreenRow: -> + height = @displayBuffer.getHeight() + lineHeightInPixels = @displayBuffer.getLineHeightInPixels() + if height? and lineHeightInPixels? + Math.min(@firstVisibleScreenRow + Math.floor(height / lineHeightInPixels), @getLineCount() - 1) + else + null + + getVisibleRowRange: -> + if lastVisibleScreenRow = @getLastVisibleScreenRow() + [@firstVisibleScreenRow, lastVisibleScreenRow] + else + null + setFirstVisibleScreenColumn: (@firstVisibleScreenColumn) -> getFirstVisibleScreenColumn: -> @firstVisibleScreenColumn @@ -3167,17 +3173,6 @@ class TextEditor extends Model @viewRegistry.getView(this).getMaxScrollTop() - getVisibleRowRange: -> - height = @displayBuffer.getHeight() - lineHeightInPixels = @displayBuffer.getLineHeightInPixels() - if height? and lineHeightInPixels? - [ - @firstVisibleScreenRow, - Math.min(@firstVisibleScreenRow + Math.floor(height / lineHeightInPixels), @getLineCount() - 1) - ] - else - null - intersectsVisibleRowRange: (startRow, endRow) -> Grim.deprecate("This is now a view method. Call TextEditorElement::intersectsVisibleRowRange instead.") From 9a35e4c9ec691e067bfd13ae8e4951d014a6071b Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 10 Nov 2015 16:26:52 -0800 Subject: [PATCH 6/8] :art: Extract methods for setting scroll position based on model --- src/text-editor-presenter.coffee | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/text-editor-presenter.coffee b/src/text-editor-presenter.coffee index 339280bf0..7bd66b87f 100644 --- a/src/text-editor-presenter.coffee +++ b/src/text-editor-presenter.coffee @@ -1090,8 +1090,7 @@ class TextEditorPresenter setLineHeight: (lineHeight) -> unless @lineHeight is lineHeight @lineHeight = lineHeight - unless @scrollTop? - @updateScrollTop(@model.getFirstVisibleScreenRow() * lineHeight) + @restoreScrollTopIfNeeded() @model.setLineHeightInPixels(lineHeight) @shouldUpdateHeightState = true @shouldUpdateHorizontalScrollState = true @@ -1119,8 +1118,7 @@ class TextEditorPresenter @halfWidthCharWidth = halfWidthCharWidth @koreanCharWidth = koreanCharWidth @model.setDefaultCharWidth(baseCharacterWidth, doubleWidthCharWidth, halfWidthCharWidth, koreanCharWidth) - unless @scrollLeft? - @updateScrollLeft(@model.getFirstVisibleScreenColumn() * baseCharacterWidth) + @restoreScrollLeftIfNeeded() @characterWidthsChanged() characterWidthsChanged: -> @@ -1525,6 +1523,14 @@ class TextEditorPresenter canScrollTopTo: (scrollTop) -> @scrollTop isnt @constrainScrollTop(scrollTop) + restoreScrollTopIfNeeded: -> + unless @scrollTop? + @updateScrollTop(@model.getFirstVisibleScreenRow() * @lineHeight) + + restoreScrollLeftIfNeeded: -> + unless @scrollLeft? + @updateScrollLeft(@model.getFirstVisibleScreenColumn() * @baseCharacterWidth) + onDidChangeScrollTop: (callback) -> @emitter.on 'did-change-scroll-top', callback From 0c72500b9e6c54ab5c1c0816799d21c2db82752c Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Tue, 10 Nov 2015 16:29:39 -0800 Subject: [PATCH 7/8] Set firstVisibleScreenRow property before emitting event --- src/text-editor.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/text-editor.coffee b/src/text-editor.coffee index a419429cc..478fbed71 100644 --- a/src/text-editor.coffee +++ b/src/text-editor.coffee @@ -3200,8 +3200,8 @@ class TextEditor extends Model screenRow = Math.max(Math.min(screenRow, maxScreenRow), 0) unless screenRow is @firstVisibleScreenRow - @emitter.emit 'did-change-first-visible-screen-row', screenRow unless fromView @firstVisibleScreenRow = screenRow + @emitter.emit 'did-change-first-visible-screen-row', screenRow unless fromView getFirstVisibleScreenRow: -> @firstVisibleScreenRow From 4c3d35529809498bfb96841d1d67b10580d144f8 Mon Sep 17 00:00:00 2001 From: Max Brunsfeld Date: Wed, 11 Nov 2015 08:44:36 -0800 Subject: [PATCH 8/8] :shirt: Add missing space after comma --- src/text-editor.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/text-editor.coffee b/src/text-editor.coffee index 478fbed71..e5b3bd481 100644 --- a/src/text-editor.coffee +++ b/src/text-editor.coffee @@ -88,7 +88,7 @@ class TextEditor extends Model super { - @softTabs, @firstVisibleScreenRow, @firstVisibleScreenColumn, initialLine,initialColumn, tabLength, + @softTabs, @firstVisibleScreenRow, @firstVisibleScreenColumn, initialLine, initialColumn, tabLength, softWrapped, @displayBuffer, @selectionsMarkerLayer, buffer, suppressCursorCreation, @mini, @placeholderText, lineNumberGutterVisible, largeFileMode, @config, @notificationManager, @packageManager, @clipboard, @viewRegistry, @grammarRegistry,