From 5a1459cf0a1abe02f20fab1a2d6864f5a13320b7 Mon Sep 17 00:00:00 2001 From: Nathan Sobo Date: Thu, 5 Oct 2017 15:01:46 -0600 Subject: [PATCH 1/9] Clear the dimensions cache after updating the soft wrap column Updating the soft wrap column could cause us to compute different values for derived dimensions, so any dimensions that were cached *in the process* of updating the soft wrap column need to be cleared. --- src/text-editor-component.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index 5667a733e..8dda2297d 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -2118,6 +2118,7 @@ class TextEditorComponent { // rendered start row accurately. 😥 this.populateVisibleRowRange(renderedStartRow) this.props.model.setEditorWidthInChars(this.getScrollContainerClientWidthInBaseCharacters()) + this.derivedDimensionsCache = {} this.suppressUpdates = false } From 43fcdf84a10fcc9544e4aefe10345057ec7c6660 Mon Sep 17 00:00:00 2001 From: Linus Eriksson Date: Fri, 6 Oct 2017 00:58:46 +0200 Subject: [PATCH 2/9] WIP --- spec/text-editor-component-spec.js | 42 ++++++++++++++++++++++++++---- 1 file changed, 37 insertions(+), 5 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 029cfee19..475a68f7d 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -6,7 +6,7 @@ const TextEditorComponent = require('../src/text-editor-component') const TextEditorElement = require('../src/text-editor-element') const TextEditor = require('../src/text-editor') const TextBuffer = require('text-buffer') -const {Point} = TextBuffer +const {Point, Range} = TextBuffer const fs = require('fs') const path = require('path') const Grim = require('grim') @@ -908,11 +908,42 @@ describe('TextEditorComponent', () => { jasmine.getEnv().defaultTimeoutInterval = originalTimeout }) - it('renders the visible rows correctly after randomly mutating the editor', async () => { + it('failing test', async () => { + const rowsPerTile = 1 + const {component, element, editor} = buildComponent({rowsPerTile, autoHeight: false}) + + await setEditorHeightInLines(component, 1) + await setEditorWidthInCharacters(component, 7) + + element.focus() + component.setScrollTop(component.measurements.lineHeight) + + component.scheduleUpdate() + await component.getNextUpdatePromise() + + editor.setSelectedBufferRange(new Range(Point(0,1),Point(12,2))) + editor.backspace() + + component.scheduleUpdate() + await component.getNextUpdatePromise() + + const renderedLines = queryOnScreenLineElements(element).sort((a, b) => a.dataset.screenRow - b.dataset.screenRow) + const renderedLineNumbers = queryOnScreenLineNumberElements(element).sort((a, b) => a.dataset.screenRow - b.dataset.screenRow) + const renderedStartRow = component.getRenderedStartRow() + const expectedLines = editor.displayLayer.getScreenLines(renderedStartRow, component.getRenderedEndRow()) + + expect(renderedLines.length).toBe(expectedLines.length) + expect(renderedLineNumbers.length).toBe(expectedLines.length) + + element.remove() + editor.destroy() + }) + + xit('renders the visible rows correctly after randomly mutating the editor', async () => { const initialSeed = Date.now() - for (var i = 0; i < 20; i++) { + for (var i = 0; i < 1; i++) { let seed = initialSeed + i - // seed = 1507224195357 + seed = 1507231571985 const failureMessage = 'Randomized test failed with seed: ' + seed const random = Random(seed) @@ -921,9 +952,10 @@ describe('TextEditorComponent', () => { editor.setSoftWrapped(Boolean(random(2))) await setEditorWidthInCharacters(component, random(20)) await setEditorHeightInLines(component, random(10)) + element.focus() - for (var j = 0; j < 5; j++) { + for (var j = 0; j < 10; j++) { const k = random(100) const range = getRandomBufferRange(random, editor.buffer) From c1c9d3f75f184313100fd9434fd8dbd2449c0034 Mon Sep 17 00:00:00 2001 From: Linus Eriksson Date: Thu, 19 Oct 2017 21:39:59 +0200 Subject: [PATCH 3/9] Handle edits that scroll up due to hiding the horizontal scrollbar --- spec/text-editor-component-spec.js | 66 +++++++++++++++--------------- src/text-editor-component.js | 19 ++++++--- 2 files changed, 47 insertions(+), 38 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 475a68f7d..a4dc7412d 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -896,6 +896,39 @@ describe('TextEditorComponent', () => { expect(component.getLineNumberGutterWidth()).toBe(originalLineNumberGutterWidth) }) + it('gracefully handles edits that change the maxScrollTop by causing the horizontal scrollbar to disappear', async () => { + const rowsPerTile = 1 + const {component, element, editor} = buildComponent({rowsPerTile, autoHeight: false}) + + await setEditorHeightInLines(component, 1) + await setEditorWidthInCharacters(component, 7) + + element.focus() + component.setScrollTop(component.measurements.lineHeight) + + component.scheduleUpdate() + await component.getNextUpdatePromise() + + editor.setSelectedBufferRange(new Range(Point(0,1),Point(12,2))) + editor.backspace() + + // component.scheduleUpdate() + await component.getNextUpdatePromise() + + expect(component.getScrollTop()).toBe(0) + + const renderedLines = queryOnScreenLineElements(element).sort((a, b) => a.dataset.screenRow - b.dataset.screenRow) + const renderedLineNumbers = queryOnScreenLineNumberElements(element).sort((a, b) => a.dataset.screenRow - b.dataset.screenRow) + const renderedStartRow = component.getRenderedStartRow() + const expectedLines = editor.displayLayer.getScreenLines(renderedStartRow, component.getRenderedEndRow()) + + expect(renderedLines.length).toBe(expectedLines.length) + expect(renderedLineNumbers.length).toBe(expectedLines.length) + + element.remove() + editor.destroy() + }) + describe('randomized tests', () => { let originalTimeout @@ -908,38 +941,7 @@ describe('TextEditorComponent', () => { jasmine.getEnv().defaultTimeoutInterval = originalTimeout }) - it('failing test', async () => { - const rowsPerTile = 1 - const {component, element, editor} = buildComponent({rowsPerTile, autoHeight: false}) - - await setEditorHeightInLines(component, 1) - await setEditorWidthInCharacters(component, 7) - - element.focus() - component.setScrollTop(component.measurements.lineHeight) - - component.scheduleUpdate() - await component.getNextUpdatePromise() - - editor.setSelectedBufferRange(new Range(Point(0,1),Point(12,2))) - editor.backspace() - - component.scheduleUpdate() - await component.getNextUpdatePromise() - - const renderedLines = queryOnScreenLineElements(element).sort((a, b) => a.dataset.screenRow - b.dataset.screenRow) - const renderedLineNumbers = queryOnScreenLineNumberElements(element).sort((a, b) => a.dataset.screenRow - b.dataset.screenRow) - const renderedStartRow = component.getRenderedStartRow() - const expectedLines = editor.displayLayer.getScreenLines(renderedStartRow, component.getRenderedEndRow()) - - expect(renderedLines.length).toBe(expectedLines.length) - expect(renderedLineNumbers.length).toBe(expectedLines.length) - - element.remove() - editor.destroy() - }) - - xit('renders the visible rows correctly after randomly mutating the editor', async () => { + it('renders the visible rows correctly after randomly mutating the editor', async () => { const initialSeed = Date.now() for (var i = 0; i < 1; i++) { let seed = initialSeed + i diff --git a/src/text-editor-component.js b/src/text-editor-component.js index 8dda2297d..a6505d760 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -266,9 +266,13 @@ class TextEditorComponent { if (useScheduler === true) { const scheduler = etch.getScheduler() scheduler.readDocument(() => { - this.measureContentDuringUpdateSync() + const restartFrame = this.measureContentDuringUpdateSync() scheduler.updateDocument(() => { - this.updateSyncAfterMeasuringContent() + if (restartFrame) { + this.updateSync(true) + } else { + this.updateSyncAfterMeasuringContent() + } }) }) } else { @@ -391,15 +395,16 @@ class TextEditorComponent { this.measureHorizontalPositions() this.updateAbsolutePositionedDecorations() + const isHorizontalScrollbarVisible = ( + this.canScrollHorizontally() && + this.getHorizontalScrollbarHeight() > 0 + ) + if (this.pendingAutoscroll) { this.derivedDimensionsCache = {} const {screenRange, options} = this.pendingAutoscroll this.autoscrollHorizontally(screenRange, options) - const isHorizontalScrollbarVisible = ( - this.canScrollHorizontally() && - this.getHorizontalScrollbarHeight() > 0 - ) if (!wasHorizontalScrollbarVisible && isHorizontalScrollbarVisible) { this.autoscrollVertically(screenRange, options) } @@ -408,6 +413,8 @@ class TextEditorComponent { this.linesToMeasure.clear() this.measuredContent = true + + return wasHorizontalScrollbarVisible !== isHorizontalScrollbarVisible } updateSyncAfterMeasuringContent () { From 612feb7cea2697501fa8b8769dd05dd29f5fcde6 Mon Sep 17 00:00:00 2001 From: Linus Eriksson Date: Thu, 19 Oct 2017 21:46:09 +0200 Subject: [PATCH 4/9] Fix the randomized test --- spec/text-editor-component-spec.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index a4dc7412d..0bf1c849a 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -943,9 +943,9 @@ describe('TextEditorComponent', () => { it('renders the visible rows correctly after randomly mutating the editor', async () => { const initialSeed = Date.now() - for (var i = 0; i < 1; i++) { + for (var i = 0; i < 20; i++) { let seed = initialSeed + i - seed = 1507231571985 + // seed = 1507231571985 const failureMessage = 'Randomized test failed with seed: ' + seed const random = Random(seed) @@ -957,7 +957,7 @@ describe('TextEditorComponent', () => { element.focus() - for (var j = 0; j < 10; j++) { + for (var j = 0; j < 5; j++) { const k = random(100) const range = getRandomBufferRange(random, editor.buffer) From 04507e9ee2a4e4aa5573dcae39e454b1b2179e35 Mon Sep 17 00:00:00 2001 From: Linus Eriksson Date: Fri, 19 Jan 2018 19:09:54 +0100 Subject: [PATCH 5/9] Use nested arrays instead of Range --- spec/text-editor-component-spec.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index d7489348a..62d00056e 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -6,7 +6,7 @@ const TextEditorComponent = require('../src/text-editor-component') const TextEditorElement = require('../src/text-editor-element') const TextEditor = require('../src/text-editor') const TextBuffer = require('text-buffer') -const {Point, Range} = TextBuffer +const {Point} = TextBuffer const fs = require('fs') const path = require('path') const Grim = require('grim') @@ -918,7 +918,7 @@ describe('TextEditorComponent', () => { component.scheduleUpdate() await component.getNextUpdatePromise() - editor.setSelectedBufferRange(new Range(Point(0,1),Point(12,2))) + editor.setSelectedBufferRange([[0, 1], [12, 2]]) editor.backspace() // component.scheduleUpdate() From b7138a041079fc85fa13b4c1218f0a7c197957dc Mon Sep 17 00:00:00 2001 From: Linus Eriksson Date: Fri, 19 Jan 2018 19:11:16 +0100 Subject: [PATCH 6/9] Add these changes to the non scheduled code path --- src/text-editor-component.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index 67323e66f..d1a9d9a50 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -276,8 +276,12 @@ class TextEditorComponent { }) }) } else { - this.measureContentDuringUpdateSync() - this.updateSyncAfterMeasuringContent() + const restartFrame = this.measureContentDuringUpdateSync() + if (restartFrame) { + this.updateSync(false) + } else { + this.updateSyncAfterMeasuringContent() + } } this.updateScheduled = false From ad6b2131d6f66631e14ad657135762bbd6f8fdbf Mon Sep 17 00:00:00 2001 From: Linus Eriksson Date: Thu, 15 Feb 2018 22:47:33 +0100 Subject: [PATCH 7/9] Style the scrollbar in the test --- spec/text-editor-component-spec.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 69bb4dbe0..ba36b7e0a 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -905,13 +905,20 @@ describe('TextEditorComponent', () => { expect(component.getLineNumberGutterWidth()).toBe(originalLineNumberGutterWidth) }) - it('gracefully handles edits that change the maxScrollTop by causing the horizontal scrollbar to disappear', async () => { + fit('gracefully handles edits that change the maxScrollTop by causing the horizontal scrollbar to disappear', async () => { const rowsPerTile = 1 const {component, element, editor} = buildComponent({rowsPerTile, autoHeight: false}) await setEditorHeightInLines(component, 1) await setEditorWidthInCharacters(component, 7) + // Updating scrollbar styles. + const style = document.createElement('style') + style.textContent = '::-webkit-scrollbar { height: 17px; width: 10px; }' + jasmine.attachToDOM(style) + TextEditor.didUpdateScrollbarStyles() + await component.getNextUpdatePromise() + element.focus() component.setScrollTop(component.measurements.lineHeight) From 63326e969a89d4e122993f094c0516ef69137719 Mon Sep 17 00:00:00 2001 From: Linus Eriksson Date: Thu, 15 Feb 2018 22:53:29 +0100 Subject: [PATCH 8/9] Unfocus test and always return false to see if this fails on circle --- spec/text-editor-component-spec.js | 2 +- src/text-editor-component.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index ba36b7e0a..4faeea8ed 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -905,7 +905,7 @@ describe('TextEditorComponent', () => { expect(component.getLineNumberGutterWidth()).toBe(originalLineNumberGutterWidth) }) - fit('gracefully handles edits that change the maxScrollTop by causing the horizontal scrollbar to disappear', async () => { + it('gracefully handles edits that change the maxScrollTop by causing the horizontal scrollbar to disappear', async () => { const rowsPerTile = 1 const {component, element, editor} = buildComponent({rowsPerTile, autoHeight: false}) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index 26dbd9f9a..a0406796f 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -418,7 +418,8 @@ class TextEditorComponent { this.linesToMeasure.clear() this.measuredContent = true - return wasHorizontalScrollbarVisible !== isHorizontalScrollbarVisible + return false + // return wasHorizontalScrollbarVisible !== isHorizontalScrollbarVisible } updateSyncAfterMeasuringContent () { From 8cfcdf8c00fa8cc118d3755dde48e452ddba4003 Mon Sep 17 00:00:00 2001 From: Linus Eriksson Date: Thu, 15 Feb 2018 23:12:35 +0100 Subject: [PATCH 9/9] Revert always return false --- src/text-editor-component.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/text-editor-component.js b/src/text-editor-component.js index a0406796f..26dbd9f9a 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -418,8 +418,7 @@ class TextEditorComponent { this.linesToMeasure.clear() this.measuredContent = true - return false - // return wasHorizontalScrollbarVisible !== isHorizontalScrollbarVisible + return wasHorizontalScrollbarVisible !== isHorizontalScrollbarVisible } updateSyncAfterMeasuringContent () {