From 9bf47f31cbf6a5aa557497dbac65b8469e0391a4 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 30 Aug 2017 12:38:44 +0200 Subject: [PATCH] Fix rendering of block decorations for invalid markers Previously, when a marker became invalid we would delete the corresponding block decoration from the DOM, without however removing it from the `lineTopIndex`. Also, we had a similar issue when decorating a marker that was already invalid: we would account for it in the index without adding it to the DOM. This commit addresses the above problems by ensuring that block decorations are never added for invalid markers. At the same time, the editor component will still keep track of changes on the marker that was decorated, so that it can detect when its validity changes and render it appropriately. --- spec/text-editor-component-spec.js | 93 +++++++++++++++++++++++++++++- src/text-editor-component.js | 68 +++++++++++++++------- src/text-editor.coffee | 2 +- 3 files changed, 137 insertions(+), 26 deletions(-) diff --git a/spec/text-editor-component-spec.js b/spec/text-editor-component-spec.js index 84d964b06..c1a2ff3cb 100644 --- a/spec/text-editor-component-spec.js +++ b/spec/text-editor-component-spec.js @@ -2276,6 +2276,93 @@ describe('TextEditorComponent', () => { ]) }) + it('removes block decorations whose markers are invalidated, and adds them back when they become valid again', async () => { + const editor = buildEditor({rowsPerTile: 3, autoHeight: false}) + const {item, decoration, marker} = createBlockDecorationAtScreenRow(editor, 3, {height: 44, position: 'before', invalidate: 'touch'}) + const {component, element} = buildComponent({editor, rowsPerTile: 3}) + + // Invalidating the marker removes the block decoration. + editor.getBuffer().deleteRows(2, 3) + await component.getNextUpdatePromise() + expect(item.parentElement).toBeNull() + assertLinesAreAlignedWithLineNumbers(component) + assertTilesAreSizedAndPositionedCorrectly(component, [ + {tileStartRow: 0, height: 3 * component.getLineHeight()}, + {tileStartRow: 3, height: 3 * component.getLineHeight()}, + {tileStartRow: 6, height: 3 * component.getLineHeight()} + ]) + + // Moving invalid markers is ignored. + marker.setScreenRange([[2, 0], [2, 0]]) + await component.getNextUpdatePromise() + expect(item.parentElement).toBeNull() + assertLinesAreAlignedWithLineNumbers(component) + assertTilesAreSizedAndPositionedCorrectly(component, [ + {tileStartRow: 0, height: 3 * component.getLineHeight()}, + {tileStartRow: 3, height: 3 * component.getLineHeight()}, + {tileStartRow: 6, height: 3 * component.getLineHeight()} + ]) + + // Making the marker valid again adds back the block decoration. + marker.bufferMarker.valid = true + marker.setScreenRange([[3, 0], [3, 0]]) + await component.getNextUpdatePromise() + expect(item.nextSibling).toBe(lineNodeForScreenRow(component, 3)) + assertLinesAreAlignedWithLineNumbers(component) + assertTilesAreSizedAndPositionedCorrectly(component, [ + {tileStartRow: 0, height: 3 * component.getLineHeight()}, + {tileStartRow: 3, height: 3 * component.getLineHeight() + 44}, + {tileStartRow: 6, height: 3 * component.getLineHeight()} + ]) + + // Destroying the decoration and invalidating the marker at the same time + // removes the block decoration correctly. + editor.getBuffer().deleteRows(2, 3) + decoration.destroy() + await component.getNextUpdatePromise() + expect(item.parentElement).toBeNull() + assertLinesAreAlignedWithLineNumbers(component) + assertTilesAreSizedAndPositionedCorrectly(component, [ + {tileStartRow: 0, height: 3 * component.getLineHeight()}, + {tileStartRow: 3, height: 3 * component.getLineHeight()}, + {tileStartRow: 6, height: 3 * component.getLineHeight()} + ]) + }) + + it('does not render block decorations when decorating invalid markers', async () => { + const editor = buildEditor({rowsPerTile: 3, autoHeight: false}) + const {component, element} = buildComponent({editor, rowsPerTile: 3}) + + const marker = editor.markScreenPosition([3, 0], {invalidate: 'touch'}) + const item = document.createElement('div') + item.style.height = 30 + 'px' + item.style.width = 30 + 'px' + editor.getBuffer().deleteRows(1, 4) + + const decoration = editor.decorateMarker(marker, {type: 'block', item, position: 'before'}) + await component.getNextUpdatePromise() + expect(item.parentElement).toBeNull() + assertLinesAreAlignedWithLineNumbers(component) + assertTilesAreSizedAndPositionedCorrectly(component, [ + {tileStartRow: 0, height: 3 * component.getLineHeight()}, + {tileStartRow: 3, height: 3 * component.getLineHeight()}, + {tileStartRow: 6, height: 3 * component.getLineHeight()} + ]) + + // Making the marker valid again causes the corresponding block decoration + // to be added to the editor. + marker.bufferMarker.valid = true + marker.setScreenRange([[2, 0], [2, 0]]) + await component.getNextUpdatePromise() + expect(item.nextSibling).toBe(lineNodeForScreenRow(component, 2)) + assertLinesAreAlignedWithLineNumbers(component) + assertTilesAreSizedAndPositionedCorrectly(component, [ + {tileStartRow: 0, height: 3 * component.getLineHeight() + 30}, + {tileStartRow: 3, height: 3 * component.getLineHeight()}, + {tileStartRow: 6, height: 3 * component.getLineHeight()} + ]) + }) + it('measures block decorations correctly when they are added before the component width has been updated', async () => { { const {editor, component, element} = buildComponent({autoHeight: false, width: 500, attach: false}) @@ -2343,8 +2430,8 @@ describe('TextEditorComponent', () => { expect(editor.getCursorScreenPosition()).toEqual([0, 0]) }) - function createBlockDecorationAtScreenRow(editor, screenRow, {height, margin, marginTop, marginBottom, position}) { - const marker = editor.markScreenPosition([screenRow, 0], {invalidate: 'never'}) + function createBlockDecorationAtScreenRow(editor, screenRow, {height, margin, marginTop, marginBottom, position, invalidate}) { + const marker = editor.markScreenPosition([screenRow, 0], {invalidate: invalidate || 'never'}) const item = document.createElement('div') item.style.height = height + 'px' if (margin != null) item.style.margin = margin + 'px' @@ -2352,7 +2439,7 @@ describe('TextEditorComponent', () => { if (marginBottom != null) item.style.marginBottom = marginBottom + 'px' item.style.width = 30 + 'px' const decoration = editor.decorateMarker(marker, {type: 'block', item, position}) - return {item, decoration} + return {item, decoration, marker} } function assertTilesAreSizedAndPositionedCorrectly (component, tiles) { diff --git a/src/text-editor-component.js b/src/text-editor-component.js index 2d904c401..682700220 100644 --- a/src/text-editor-component.js +++ b/src/text-editor-component.js @@ -2460,37 +2460,61 @@ class TextEditorComponent { const {model} = this.props const decorations = model.getDecorations({type: 'block'}) for (let i = 0; i < decorations.length; i++) { - this.didAddBlockDecoration(decorations[i]) + this.addBlockDecoration(decorations[i]) } } - didAddBlockDecoration (decoration) { + addBlockDecoration (decoration, subscribeToChanges = true) { const marker = decoration.getMarker() const {item, position} = decoration.getProperties() const element = TextEditor.viewForItem(item) - const row = marker.getHeadScreenPosition().row - this.lineTopIndex.insertBlock(decoration, row, 0, position === 'after') - this.blockDecorationsToMeasure.add(decoration) - this.blockDecorationsByElement.set(element, decoration) - this.blockDecorationResizeObserver.observe(element) + if (marker.isValid()) { + const row = marker.getHeadScreenPosition().row + this.lineTopIndex.insertBlock(decoration, row, 0, position === 'after') + this.blockDecorationsToMeasure.add(decoration) + this.blockDecorationsByElement.set(element, decoration) + this.blockDecorationResizeObserver.observe(element) - const didUpdateDisposable = marker.bufferMarker.onDidChange((e) => { - if (!e.textChanged) { - this.lineTopIndex.moveBlock(decoration, marker.getHeadScreenPosition().row) - this.scheduleUpdate() - } - }) - const didDestroyDisposable = decoration.onDidDestroy(() => { - this.blockDecorationsToMeasure.delete(decoration) - this.heightsByBlockDecoration.delete(decoration) - this.blockDecorationsByElement.delete(element) - this.blockDecorationResizeObserver.unobserve(element) - this.lineTopIndex.removeBlock(decoration) - didUpdateDisposable.dispose() - didDestroyDisposable.dispose() this.scheduleUpdate() - }) + } + + if (subscribeToChanges) { + let wasValid = marker.isValid() + + const didUpdateDisposable = marker.bufferMarker.onDidChange(({textChanged}) => { + const isValid = marker.isValid() + if (wasValid && !isValid) { + wasValid = false + this.blockDecorationsToMeasure.delete(decoration) + this.heightsByBlockDecoration.delete(decoration) + this.blockDecorationsByElement.delete(element) + this.blockDecorationResizeObserver.unobserve(element) + this.lineTopIndex.removeBlock(decoration) + this.scheduleUpdate() + } else if (!wasValid && isValid) { + wasValid = true + this.addBlockDecoration(decoration, false) + } else if (isValid && !textChanged) { + this.lineTopIndex.moveBlock(decoration, marker.getHeadScreenPosition().row) + this.scheduleUpdate() + } + }) + + const didDestroyDisposable = decoration.onDidDestroy(() => { + didUpdateDisposable.dispose() + didDestroyDisposable.dispose() + + if (marker.isValid()) { + this.blockDecorationsToMeasure.delete(decoration) + this.heightsByBlockDecoration.delete(decoration) + this.blockDecorationsByElement.delete(element) + this.blockDecorationResizeObserver.unobserve(element) + this.lineTopIndex.removeBlock(decoration) + this.scheduleUpdate() + } + }) + } } didResizeBlockDecorations (entries) { diff --git a/src/text-editor.coffee b/src/text-editor.coffee index a248be715..54de91054 100644 --- a/src/text-editor.coffee +++ b/src/text-editor.coffee @@ -738,7 +738,7 @@ class TextEditor extends Model # Called by DecorationManager when a decoration is added. didAddDecoration: (decoration) -> if decoration.isType('block') - @component?.didAddBlockDecoration(decoration) + @component?.addBlockDecoration(decoration) # Extended: Calls your `callback` when the placeholder text is changed. #