From 4eecf8d1a683e6b4e4c3b16a08907b1bfd7f0af7 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Tue, 9 May 2017 15:09:14 -0600 Subject: [PATCH] Don't change number of tiles based on block decorations This means we may render more tiles than necessary when we have block decorations, but it prevents changing the number of rendered tiles during scrolling with certain combinations of line height and editor height. If it ever becomes a problem we can get smarter about subtracting the height of the visible block decorations from the editor height, but for now this gives us more reliable performance for the common case. --- spec/text-editor-component-spec.js | 72 +++++++++++++++--------------- src/text-editor-component.js | 10 +++-- 2 files changed, 43 insertions(+), 39 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 78ab6a8e2..66bbb6b97 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -1694,7 +1694,7 @@ describe('TextEditorComponent', () => { const {component, element} = buildComponent({editor, rowsPerTile: 3}) await setEditorHeightInLines(component, 4) expect(component.getRenderedStartRow()).toBe(0) - expect(component.getRenderedEndRow()).toBe(6) + expect(component.getRenderedEndRow()).toBe(9) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item1) + getElementHeight(item2) @@ -1704,7 +1704,7 @@ describe('TextEditorComponent', () => { {tileStartRow: 3, height: 3 * component.getLineHeight()} ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(6) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) expect(item1.previousSibling.className).toBe('highlights') expect(item1.nextSibling).toBe(lineNodeForScreenRow(component, 0)) expect(item2.previousSibling).toBe(lineNodeForScreenRow(component, 1)) @@ -1717,7 +1717,7 @@ describe('TextEditorComponent', () => { const {item: item6, decoration: decoration6} = createBlockDecorationAtScreenRow(editor, 12, {height: 66, position: 'after'}) await component.getNextUpdatePromise() expect(component.getRenderedStartRow()).toBe(0) - expect(component.getRenderedEndRow()).toBe(6) + expect(component.getRenderedEndRow()).toBe(9) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item1) + getElementHeight(item2) + getElementHeight(item3) + @@ -1728,22 +1728,22 @@ describe('TextEditorComponent', () => { {tileStartRow: 3, height: 3 * component.getLineHeight() + getElementHeight(item3)} ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(6) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) expect(item1.previousSibling.className).toBe('highlights') expect(item1.nextSibling).toBe(lineNodeForScreenRow(component, 0)) expect(item2.previousSibling).toBe(lineNodeForScreenRow(component, 1)) expect(item2.nextSibling).toBe(lineNodeForScreenRow(component, 2)) expect(item3.previousSibling).toBe(lineNodeForScreenRow(component, 3)) expect(item3.nextSibling).toBe(lineNodeForScreenRow(component, 4)) - expect(element.contains(item4)).toBe(false) - expect(element.contains(item5)).toBe(false) + expect(item4.nextSibling).toBe(lineNodeForScreenRow(component, 7)) + expect(item5.previousSibling).toBe(lineNodeForScreenRow(component, 7)) expect(element.contains(item6)).toBe(false) // destroy decoration1 decoration1.destroy() await component.getNextUpdatePromise() expect(component.getRenderedStartRow()).toBe(0) - expect(component.getRenderedEndRow()).toBe(6) + expect(component.getRenderedEndRow()).toBe(9) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item2) + getElementHeight(item3) + @@ -1754,14 +1754,14 @@ describe('TextEditorComponent', () => { {tileStartRow: 3, height: 3 * component.getLineHeight() + getElementHeight(item3)} ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(6) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) expect(element.contains(item1)).toBe(false) expect(item2.previousSibling).toBe(lineNodeForScreenRow(component, 1)) expect(item2.nextSibling).toBe(lineNodeForScreenRow(component, 2)) expect(item3.previousSibling).toBe(lineNodeForScreenRow(component, 3)) expect(item3.nextSibling).toBe(lineNodeForScreenRow(component, 4)) - expect(element.contains(item4)).toBe(false) - expect(element.contains(item5)).toBe(false) + expect(item4.nextSibling).toBe(lineNodeForScreenRow(component, 7)) + expect(item5.previousSibling).toBe(lineNodeForScreenRow(component, 7)) expect(element.contains(item6)).toBe(false) // move decoration2 and decoration3 @@ -1769,7 +1769,7 @@ describe('TextEditorComponent', () => { decoration3.getMarker().setHeadScreenPosition([0, 0]) await component.getNextUpdatePromise() expect(component.getRenderedStartRow()).toBe(0) - expect(component.getRenderedEndRow()).toBe(6) + expect(component.getRenderedEndRow()).toBe(9) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item2) + getElementHeight(item3) + @@ -1780,21 +1780,21 @@ describe('TextEditorComponent', () => { {tileStartRow: 3, height: 3 * component.getLineHeight()} ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(6) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) expect(element.contains(item1)).toBe(false) expect(item2.previousSibling).toBe(lineNodeForScreenRow(component, 0)) expect(item2.nextSibling).toBe(lineNodeForScreenRow(component, 1)) expect(item3.previousSibling.className).toBe('highlights') expect(item3.nextSibling).toBe(lineNodeForScreenRow(component, 0)) - expect(element.contains(item4)).toBe(false) - expect(element.contains(item5)).toBe(false) + expect(item4.nextSibling).toBe(lineNodeForScreenRow(component, 7)) + expect(item5.previousSibling).toBe(lineNodeForScreenRow(component, 7)) expect(element.contains(item6)).toBe(false) // change the text editor.getBuffer().setTextInRange([[0, 5], [0, 5]], '\n\n') await component.getNextUpdatePromise() expect(component.getRenderedStartRow()).toBe(0) - expect(component.getRenderedEndRow()).toBe(6) + expect(component.getRenderedEndRow()).toBe(9) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item2) + getElementHeight(item3) + @@ -1805,7 +1805,7 @@ describe('TextEditorComponent', () => { {tileStartRow: 3, height: 3 * component.getLineHeight() + getElementHeight(item2)} ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(6) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) expect(element.contains(item1)).toBe(false) expect(item2.previousSibling.className).toBe('highlights') expect(item2.nextSibling).toBe(lineNodeForScreenRow(component, 3)) @@ -1818,7 +1818,7 @@ describe('TextEditorComponent', () => { // scroll past the first tile await setScrollTop(component, 3 * component.getLineHeight() + getElementHeight(item3)) expect(component.getRenderedStartRow()).toBe(3) - expect(component.getRenderedEndRow()).toBe(9) + expect(component.getRenderedEndRow()).toBe(12) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item2) + getElementHeight(item3) + @@ -1829,13 +1829,13 @@ describe('TextEditorComponent', () => { {tileStartRow: 6, height: 3 * component.getLineHeight()} ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(6) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) expect(element.contains(item1)).toBe(false) expect(item2.previousSibling.className).toBe('highlights') expect(item2.nextSibling).toBe(lineNodeForScreenRow(component, 3)) expect(element.contains(item3)).toBe(false) - expect(element.contains(item4)).toBe(false) - expect(element.contains(item5)).toBe(false) + expect(item4.nextSibling).toBe(lineNodeForScreenRow(component, 9)) + expect(item5.previousSibling).toBe(lineNodeForScreenRow(component, 9)) expect(element.contains(item6)).toBe(false) await setScrollTop(component, 0) @@ -1843,7 +1843,7 @@ describe('TextEditorComponent', () => { editor.undo() await component.getNextUpdatePromise() expect(component.getRenderedStartRow()).toBe(0) - expect(component.getRenderedEndRow()).toBe(6) + expect(component.getRenderedEndRow()).toBe(9) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item2) + getElementHeight(item3) + @@ -1854,14 +1854,14 @@ describe('TextEditorComponent', () => { {tileStartRow: 3, height: 3 * component.getLineHeight()} ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(6) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) expect(element.contains(item1)).toBe(false) expect(item2.previousSibling).toBe(lineNodeForScreenRow(component, 0)) expect(item2.nextSibling).toBe(lineNodeForScreenRow(component, 1)) expect(item3.previousSibling.className).toBe('highlights') expect(item3.nextSibling).toBe(lineNodeForScreenRow(component, 0)) - expect(element.contains(item4)).toBe(false) - expect(element.contains(item5)).toBe(false) + expect(item4.nextSibling).toBe(lineNodeForScreenRow(component, 7)) + expect(item5.previousSibling).toBe(lineNodeForScreenRow(component, 7)) expect(element.contains(item6)).toBe(false) // invalidate decorations. this also tests a case where two decorations in @@ -1875,7 +1875,7 @@ describe('TextEditorComponent', () => { component.invalidateBlockDecorationDimensions(decoration3) await component.getNextUpdatePromise() expect(component.getRenderedStartRow()).toBe(0) - expect(component.getRenderedEndRow()).toBe(6) + expect(component.getRenderedEndRow()).toBe(9) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item2) + getElementHeight(item3) + @@ -1886,14 +1886,14 @@ describe('TextEditorComponent', () => { {tileStartRow: 3, height: 3 * component.getLineHeight()} ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(6) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) expect(element.contains(item1)).toBe(false) expect(item2.previousSibling).toBe(lineNodeForScreenRow(component, 0)) expect(item2.nextSibling).toBe(lineNodeForScreenRow(component, 1)) expect(item3.previousSibling.className).toBe('highlights') expect(item3.nextSibling).toBe(lineNodeForScreenRow(component, 0)) - expect(element.contains(item4)).toBe(false) - expect(element.contains(item5)).toBe(false) + expect(item4.nextSibling).toBe(lineNodeForScreenRow(component, 7)) + expect(item5.previousSibling).toBe(lineNodeForScreenRow(component, 7)) expect(element.contains(item6)).toBe(false) // make decoration before row 0 as wide as the editor, and insert some text into it so that it wraps. @@ -1914,7 +1914,7 @@ describe('TextEditorComponent', () => { ) + 'px' await component.getNextUpdatePromise() expect(component.getRenderedStartRow()).toBe(0) - expect(component.getRenderedEndRow()).toBe(6) + expect(component.getRenderedEndRow()).toBe(9) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item2) + getElementHeight(item3) + @@ -1925,14 +1925,14 @@ describe('TextEditorComponent', () => { {tileStartRow: 3, height: 3 * component.getLineHeight()} ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(6) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) expect(element.contains(item1)).toBe(false) expect(item2.previousSibling).toBe(lineNodeForScreenRow(component, 0)) expect(item2.nextSibling).toBe(lineNodeForScreenRow(component, 1)) expect(item3.previousSibling.className).toBe('highlights') expect(item3.nextSibling).toBe(lineNodeForScreenRow(component, 0)) - expect(element.contains(item4)).toBe(false) - expect(element.contains(item5)).toBe(false) + expect(item3.nextSibling).toBe(lineNodeForScreenRow(component, 0)) + expect(item4.nextSibling).toBe(lineNodeForScreenRow(component, 7)) expect(element.contains(item6)).toBe(false) // make the editor taller and wider and the same time, ensuring the number @@ -1940,7 +1940,7 @@ describe('TextEditorComponent', () => { setEditorHeightInLines(component, 13) await setEditorWidthInCharacters(component, 50) expect(component.getRenderedStartRow()).toBe(0) - expect(component.getRenderedEndRow()).toBe(9) + expect(component.getRenderedEndRow()).toBe(13) expect(component.getScrollHeight()).toBe( editor.getScreenLineCount() * component.getLineHeight() + getElementHeight(item2) + getElementHeight(item3) + @@ -1952,7 +1952,7 @@ describe('TextEditorComponent', () => { {tileStartRow: 6, height: 3 * component.getLineHeight() + getElementHeight(item4) + getElementHeight(item5)}, ]) assertLinesAreAlignedWithLineNumbers(component) - expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(9) + expect(element.querySelectorAll('.line:not(.dummy)').length).toBe(13) expect(element.contains(item1)).toBe(false) expect(item2.previousSibling).toBe(lineNodeForScreenRow(component, 0)) expect(item2.nextSibling).toBe(lineNodeForScreenRow(component, 1)) @@ -1962,7 +1962,7 @@ describe('TextEditorComponent', () => { expect(item4.nextSibling).toBe(lineNodeForScreenRow(component, 7)) expect(item5.previousSibling).toBe(lineNodeForScreenRow(component, 7)) expect(item5.nextSibling).toBe(lineNodeForScreenRow(component, 8)) - expect(element.contains(item6)).toBe(false) + expect(item6.previousSibling).toBe(lineNodeForScreenRow(component, 12)) }) function createBlockDecorationAtScreenRow(editor, screenRow, {height, margin, position}) { @@ -3104,7 +3104,7 @@ describe('TextEditorComponent', () => { expect(element.querySelectorAll('.line:not(.dummy)').length).toBeGreaterThan(initialRenderedLineCount) verifyCursorPosition(component, cursorNode, 1, 29) - element.style.fontSize = initialFontSize + 5 + 'px' + element.style.fontSize = initialFontSize + 10 + 'px' TextEditor.didUpdateStyles() await component.getNextUpdatePromise() expect(editor.getDefaultCharWidth()).toBeGreaterThan(initialBaseCharacterWidth) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index fe3eb15c3..ea28c0f3c 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -2553,12 +2553,16 @@ class TextEditorComponent { return this.derivedDimensionsCache.lastVisibleRow } + // We may render more tiles than needed if some contain block decorations, + // but keeping this calculation simple ensures the number of tiles remains + // fixed for a given editor height, which eliminates situations where a + // tile is repeatedly added and removed during scrolling in certain + // comibinations of editor height and line height. getVisibleTileCount () { if (this.derivedDimensionsCache.visibleTileCount == null) { - const visibleRowCount = this.getLastVisibleRow() - this.getFirstVisibleRow() - this.derivedDimensionsCache.visibleTileCount = Math.ceil(visibleRowCount / this.getRowsPerTile()) + 1 + const editorHeightInTiles = this.getScrollContainerHeight() / this.getLineHeight() / this.getRowsPerTile() + this.derivedDimensionsCache.visibleTileCount = Math.ceil(editorHeightInTiles) + 1 } - return this.derivedDimensionsCache.visibleTileCount }