From 07f41574036af55362e6b258b052673c607fddee Mon Sep 17 00:00:00 2001 From: Alice Koreman Date: Wed, 22 Nov 2023 13:27:00 +0100 Subject: [PATCH 1/8] fix: improvements for completer popup + inline preview --- src/autocomplete.js | 8 +++++++- src/autocomplete/popup.js | 17 +++++++++++++++-- src/virtual_renderer.js | 20 +++++++++++++++++++- 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/src/autocomplete.js b/src/autocomplete.js index 3dd1180580b..408e113c5c9 100644 --- a/src/autocomplete.js +++ b/src/autocomplete.js @@ -82,6 +82,8 @@ class Autocomplete { }.bind(this)); this.tooltipTimer = lang.delayedCall(this.updateDocTooltip.bind(this), 50); + this.popupTimer = lang.delayedCall(this.$updatePopupPosition.bind(this), 50); + this.stickySelectionTimer = lang.delayedCall(function() { this.stickySelection = true; @@ -150,8 +152,12 @@ class Autocomplete { this.tooltipTimer.call(null, null); return; } + + // Update the popup position after a short wait to account for potential scrolling + this.popupTimer.schedule(); + } else { + this.$updatePopupPosition(); } - this.$updatePopupPosition(); this.tooltipTimer.call(null, null); } diff --git a/src/autocomplete/popup.js b/src/autocomplete/popup.js index 51a491ec987..bc473f57e6e 100644 --- a/src/autocomplete/popup.js +++ b/src/autocomplete/popup.js @@ -44,6 +44,7 @@ class AcePopup { if (parentNode) { parentNode.appendChild(el); + popup.parentNode = parentNode; } el.style.display = "none"; popup.renderer.content.style.cursor = "default"; @@ -299,7 +300,13 @@ class AcePopup { var maxH = renderer.$maxLines * lineHeight * 1.4; var dims = { top: 0, bottom: 0, left: 0 }; - var spaceBelow = screenHeight - pos.top - 3 * this.$borderSize - lineHeight; + var spaceBelow; + // If the popup has a specified parent node, calculate spaceBelow w.r.t. the parent + if (popup.parentNode) { + spaceBelow = popup.parentNode.clientHeight - pos.top - 3 * this.$borderSize - lineHeight; + } else { + spaceBelow = screenHeight - pos.top - 3 * this.$borderSize - lineHeight; + } var spaceAbove = pos.top - 3 * this.$borderSize; if (!anchor) { if (spaceAbove <= spaceBelow || spaceBelow >= maxH) { @@ -317,7 +324,13 @@ class AcePopup { dims.bottom = dims.top + maxH; } - var fitsX = dims.top >= 0 && dims.bottom <= screenHeight; + var fitsX; + // If the popup has a specified parent node, keep the popup within the parent + if (popup.parentNode) { + fitsX = dims.top >= 0 && dims.bottom <= popup.parentNode.clientHeight; + } else { + fitsX = dims.top >= 0 && dims.bottom <= screenHeight; + } if (!forceShow && !fitsX) { return false; diff --git a/src/virtual_renderer.js b/src/virtual_renderer.js index 3947f44f24f..a853bfcd336 100644 --- a/src/virtual_renderer.js +++ b/src/virtual_renderer.js @@ -1620,8 +1620,26 @@ class VirtualRenderer { className: "ace_ghost_text" }; this.session.widgetManager.addLineWidget(this.$ghostTextWidget); + + // Check wether the line widget fits in the part of the screen currently in view + var pixelPosition = this.$cursorLayer.getPixelPosition(insertPosition, true); + console.log(pixelPosition) + var el = this.container; + var height = el.clientHeight || el.scrollHeight; + var fitsY = textLines.length * this.lineHeight < height - pixelPosition.top; + + // If it fits, no action needed + if (fitsY) return; + + // If it can fully fit in the screen, scroll down until it fits on the screen + // if it cannot fully fit, scroll so that the cursor is at the top of the screen + // to fit as much as possible. + if (textLines.length * this.lineHeight < height) { + this.scrollBy(0, (textLines.length - 1) * this.lineHeight); + } else { + this.scrollBy(0, -pixelPosition.top); + } } - } removeGhostText() { From 288e47041c6f60cbd514bd9b468fe47a624bb879 Mon Sep 17 00:00:00 2001 From: Alice Koreman Date: Wed, 22 Nov 2023 14:30:14 +0100 Subject: [PATCH 2/8] add test popup changes --- src/autocomplete.js | 2 +- src/autocomplete/popup_test.js | 28 ++++++++++++++++++++++++++-- src/virtual_renderer.js | 3 +-- 3 files changed, 28 insertions(+), 5 deletions(-) diff --git a/src/autocomplete.js b/src/autocomplete.js index 4d3d2a240fe..fd1e27c1762 100644 --- a/src/autocomplete.js +++ b/src/autocomplete.js @@ -92,7 +92,6 @@ class Autocomplete { this.tooltipTimer = lang.delayedCall(this.updateDocTooltip.bind(this), 50); this.popupTimer = lang.delayedCall(this.$updatePopupPosition.bind(this), 50); - this.stickySelectionTimer = lang.delayedCall(function() { this.stickySelection = true; }.bind(this), this.stickySelectionDelay); @@ -145,6 +144,7 @@ class Autocomplete { } this.hideDocTooltip(); this.stickySelectionTimer.cancel(); + this.popupTimer.cancel(); this.stickySelection = false; } diff --git a/src/autocomplete/popup_test.js b/src/autocomplete/popup_test.js index 8c4ee962ee4..ec59320abcd 100644 --- a/src/autocomplete/popup_test.js +++ b/src/autocomplete/popup_test.js @@ -32,8 +32,12 @@ var tryShowAndRender = function(pos, lineHeight, anchor, forceShow) { }; -var setupPopup = function() { - popup = new AcePopup(document.body); +var setupPopup = function(parentNode) { + if (parentNode) { + popup = new AcePopup(parentNode); + } else { + popup = new AcePopup(document.body); + } // Mock the CSS behaviour popup.container.style.width = "300px"; @@ -209,6 +213,26 @@ module.exports = { assert.strictEqual(popup.container.style.display, "none"); done(); }, + "test: should keep popup contained to parentNode if specified": function(done) { + document.body.style.height = 1000 + 'px'; + + var parentNode = document.createElement("div"); + parentNode.style.width = "500px"; + parentNode.style.height = "500px"; + document.body.appendChild(parentNode); + + setupPopup(parentNode); + + // below shouldn't fit + var result = tryShowAndRender({ top: 450, left: 0 }, lineHeight, "bottom", false); + assert.strictEqual(result, false); + + // above should fit + var result = tryShowAndRender({ top: 450, left: 0 }, lineHeight, "top", false); + assert.strictEqual(result, true); + + done(); + }, tearDown: tearDown }; diff --git a/src/virtual_renderer.js b/src/virtual_renderer.js index a853bfcd336..ec778ea65e9 100644 --- a/src/virtual_renderer.js +++ b/src/virtual_renderer.js @@ -1623,9 +1623,8 @@ class VirtualRenderer { // Check wether the line widget fits in the part of the screen currently in view var pixelPosition = this.$cursorLayer.getPixelPosition(insertPosition, true); - console.log(pixelPosition) var el = this.container; - var height = el.clientHeight || el.scrollHeight; + var height = el.clientHeight; var fitsY = textLines.length * this.lineHeight < height - pixelPosition.top; // If it fits, no action needed From 22257eb33e21b3e8bdcc731914263da0f183ce99 Mon Sep 17 00:00:00 2001 From: Alice Koreman Date: Wed, 22 Nov 2023 17:02:16 +0100 Subject: [PATCH 3/8] add baseline test popup --- src/autocomplete/popup_test.js | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/autocomplete/popup_test.js b/src/autocomplete/popup_test.js index ec59320abcd..e6e0067e176 100644 --- a/src/autocomplete/popup_test.js +++ b/src/autocomplete/popup_test.js @@ -228,7 +228,23 @@ module.exports = { assert.strictEqual(result, false); // above should fit - var result = tryShowAndRender({ top: 450, left: 0 }, lineHeight, "top", false); + result = tryShowAndRender({ top: 450, left: 0 }, lineHeight, "top", false); + assert.strictEqual(result, true); + + done(); + }, + "test: should not keep popup contained to container if no parentNode is specified": function(done) { + document.body.style.height = 1000 + 'px'; + + var parentNode = document.createElement("div"); + parentNode.style.width = "500px"; + parentNode.style.height = "500px"; + document.body.appendChild(parentNode); + + setupPopup(undefined); + + // below should fit + var result = tryShowAndRender({ top: 450, left: 0 }, lineHeight, "bottom", false); assert.strictEqual(result, true); done(); From 1073927421b89df241583938e0425f0a8aadd139 Mon Sep 17 00:00:00 2001 From: Alice Koreman Date: Thu, 23 Nov 2023 10:39:28 +0100 Subject: [PATCH 4/8] add test inline scrolling --- src/autocomplete/inline_test.js | 40 +++++++++++++++++++++++++++++++++ src/virtual_renderer.js | 2 +- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/src/autocomplete/inline_test.js b/src/autocomplete/inline_test.js index fc5705b99ba..5e0ee5f5c3a 100644 --- a/src/autocomplete/inline_test.js +++ b/src/autocomplete/inline_test.js @@ -42,6 +42,14 @@ var completions = [ value: "f should not show inline", score: 0, hideInlinePreview: true + }, + { + value: "long\nlong\nlong\nlong\nlong\nlong", + score: 0 + }, + { + value: "long\nlong\nlong\nlong\nlong\nlong".repeat(100), + score: 0 } ]; @@ -261,6 +269,38 @@ module.exports = { done(); }, + "test: should scroll if inline preview outside": function(done) { + // Fill the editor with new lines to get the cursor to the bottom + // of the container + editor.execCommand("insertstring", "\n".repeat(200)); + + var deltaY; + var initialScrollBy = editor.renderer.scrollBy; + editor.renderer.scrollBy = function(varX, varY) { + deltaY = varY; + }; + + inline.show(editor, completions[6], "l"); + editor.renderer.$loop._flush(); + + setTimeout(() => { + // Should scroll 5 lines to get the inline preview into view + assert.strictEqual(deltaY, 50); + + inline.hide(); + editor.renderer.$loop._flush(); + + inline.show(editor, completions[7], "l"); + editor.renderer.$loop._flush(); + + setTimeout(() => { + // Should scroll as much as possbile while keeping the cursor on screen + assert.strictEqual(deltaY, 490); + editor.renderer.scrollBy = initialScrollBy; + done(); + }, 50); + }, 50); + }, tearDown: function() { inline.destroy(); editor.destroy(); diff --git a/src/virtual_renderer.js b/src/virtual_renderer.js index ec778ea65e9..8d7d73b14e6 100644 --- a/src/virtual_renderer.js +++ b/src/virtual_renderer.js @@ -1636,7 +1636,7 @@ class VirtualRenderer { if (textLines.length * this.lineHeight < height) { this.scrollBy(0, (textLines.length - 1) * this.lineHeight); } else { - this.scrollBy(0, -pixelPosition.top); + this.scrollBy(0, pixelPosition.top); } } } From da455e7afb0d5d785cfc068df874bfef058764fb Mon Sep 17 00:00:00 2001 From: Alice Koreman Date: Thu, 23 Nov 2023 14:35:47 +0100 Subject: [PATCH 5/8] move doc tooltip along with popup after scrolling --- src/autocomplete.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/autocomplete.js b/src/autocomplete.js index fd1e27c1762..d8d4aa531a8 100644 --- a/src/autocomplete.js +++ b/src/autocomplete.js @@ -165,10 +165,11 @@ class Autocomplete { // Update the popup position after a short wait to account for potential scrolling this.popupTimer.schedule(); + this.tooltipTimer.schedule(); } else { this.$updatePopupPosition(); + this.tooltipTimer.call(null, null); } - this.tooltipTimer.call(null, null); } $onPopupShow(hide) { From 6d7f00a1972e12385604947c43612e18220cfa67 Mon Sep 17 00:00:00 2001 From: Alice Koreman Date: Fri, 24 Nov 2023 09:22:04 +0100 Subject: [PATCH 6/8] revert changes made to popup --- src/autocomplete/popup.js | 21 ++-------------- src/autocomplete/popup_test.js | 45 +++------------------------------- 2 files changed, 5 insertions(+), 61 deletions(-) diff --git a/src/autocomplete/popup.js b/src/autocomplete/popup.js index bc473f57e6e..5259feeb006 100644 --- a/src/autocomplete/popup.js +++ b/src/autocomplete/popup.js @@ -44,7 +44,6 @@ class AcePopup { if (parentNode) { parentNode.appendChild(el); - popup.parentNode = parentNode; } el.style.display = "none"; popup.renderer.content.style.cursor = "default"; @@ -300,13 +299,7 @@ class AcePopup { var maxH = renderer.$maxLines * lineHeight * 1.4; var dims = { top: 0, bottom: 0, left: 0 }; - var spaceBelow; - // If the popup has a specified parent node, calculate spaceBelow w.r.t. the parent - if (popup.parentNode) { - spaceBelow = popup.parentNode.clientHeight - pos.top - 3 * this.$borderSize - lineHeight; - } else { - spaceBelow = screenHeight - pos.top - 3 * this.$borderSize - lineHeight; - } + var spaceBelow = screenHeight - pos.top - 3 * this.$borderSize - lineHeight; var spaceAbove = pos.top - 3 * this.$borderSize; if (!anchor) { if (spaceAbove <= spaceBelow || spaceBelow >= maxH) { @@ -324,17 +317,7 @@ class AcePopup { dims.bottom = dims.top + maxH; } - var fitsX; - // If the popup has a specified parent node, keep the popup within the parent - if (popup.parentNode) { - fitsX = dims.top >= 0 && dims.bottom <= popup.parentNode.clientHeight; - } else { - fitsX = dims.top >= 0 && dims.bottom <= screenHeight; - } - - if (!forceShow && !fitsX) { - return false; - } + var fitsX = dims.top >= 0 && dims.bottom <= screenHeight; if (!fitsX) { if (anchor === "top") { diff --git a/src/autocomplete/popup_test.js b/src/autocomplete/popup_test.js index e6e0067e176..c00db0a5ef1 100644 --- a/src/autocomplete/popup_test.js +++ b/src/autocomplete/popup_test.js @@ -32,12 +32,9 @@ var tryShowAndRender = function(pos, lineHeight, anchor, forceShow) { }; -var setupPopup = function(parentNode) { - if (parentNode) { - popup = new AcePopup(parentNode); - } else { - popup = new AcePopup(document.body); - } + +var setupPopup = function() { + popup = new AcePopup(document.body); // Mock the CSS behaviour popup.container.style.width = "300px"; @@ -213,42 +210,6 @@ module.exports = { assert.strictEqual(popup.container.style.display, "none"); done(); }, - "test: should keep popup contained to parentNode if specified": function(done) { - document.body.style.height = 1000 + 'px'; - - var parentNode = document.createElement("div"); - parentNode.style.width = "500px"; - parentNode.style.height = "500px"; - document.body.appendChild(parentNode); - - setupPopup(parentNode); - - // below shouldn't fit - var result = tryShowAndRender({ top: 450, left: 0 }, lineHeight, "bottom", false); - assert.strictEqual(result, false); - - // above should fit - result = tryShowAndRender({ top: 450, left: 0 }, lineHeight, "top", false); - assert.strictEqual(result, true); - - done(); - }, - "test: should not keep popup contained to container if no parentNode is specified": function(done) { - document.body.style.height = 1000 + 'px'; - - var parentNode = document.createElement("div"); - parentNode.style.width = "500px"; - parentNode.style.height = "500px"; - document.body.appendChild(parentNode); - - setupPopup(undefined); - - // below should fit - var result = tryShowAndRender({ top: 450, left: 0 }, lineHeight, "bottom", false); - assert.strictEqual(result, true); - - done(); - }, tearDown: tearDown }; From 8f608189499400e44a8f8b7f8a57e43ee8011ebe Mon Sep 17 00:00:00 2001 From: Alice Koreman Date: Fri, 24 Nov 2023 09:24:43 +0100 Subject: [PATCH 7/8] fix reverting mistake --- src/autocomplete/popup.js | 4 ++++ src/autocomplete/popup_test.js | 1 - src/virtual_renderer.js | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/autocomplete/popup.js b/src/autocomplete/popup.js index 5259feeb006..51a491ec987 100644 --- a/src/autocomplete/popup.js +++ b/src/autocomplete/popup.js @@ -319,6 +319,10 @@ class AcePopup { var fitsX = dims.top >= 0 && dims.bottom <= screenHeight; + if (!forceShow && !fitsX) { + return false; + } + if (!fitsX) { if (anchor === "top") { renderer.$maxPixelHeight = spaceAbove; diff --git a/src/autocomplete/popup_test.js b/src/autocomplete/popup_test.js index c00db0a5ef1..8c4ee962ee4 100644 --- a/src/autocomplete/popup_test.js +++ b/src/autocomplete/popup_test.js @@ -32,7 +32,6 @@ var tryShowAndRender = function(pos, lineHeight, anchor, forceShow) { }; - var setupPopup = function() { popup = new AcePopup(document.body); diff --git a/src/virtual_renderer.js b/src/virtual_renderer.js index 8d7d73b14e6..4598819401b 100644 --- a/src/virtual_renderer.js +++ b/src/virtual_renderer.js @@ -1639,6 +1639,7 @@ class VirtualRenderer { this.scrollBy(0, pixelPosition.top); } } + } removeGhostText() { From 4bd2dbb6bd71883ddcc045c9bcc96d6ef0a27bf3 Mon Sep 17 00:00:00 2001 From: Alice Koreman Date: Tue, 28 Nov 2023 10:50:49 +0100 Subject: [PATCH 8/8] adress PR comments --- src/autocomplete.js | 2 +- src/virtual_renderer.js | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/autocomplete.js b/src/autocomplete.js index f7229e0b214..ba8458276d5 100644 --- a/src/autocomplete.js +++ b/src/autocomplete.js @@ -167,7 +167,7 @@ class Autocomplete { this.popupTimer.schedule(); this.tooltipTimer.schedule(); } else { - this.$updatePopupPosition(); + this.popupTimer.call(null, null); this.tooltipTimer.call(null, null); } } diff --git a/src/virtual_renderer.js b/src/virtual_renderer.js index 4598819401b..916b800a27a 100644 --- a/src/virtual_renderer.js +++ b/src/virtual_renderer.js @@ -1624,8 +1624,9 @@ class VirtualRenderer { // Check wether the line widget fits in the part of the screen currently in view var pixelPosition = this.$cursorLayer.getPixelPosition(insertPosition, true); var el = this.container; - var height = el.clientHeight; - var fitsY = textLines.length * this.lineHeight < height - pixelPosition.top; + var height = el.getBoundingClientRect().height; + var ghostTextHeight = textLines.length * this.lineHeight; + var fitsY = ghostTextHeight < height - pixelPosition.top; // If it fits, no action needed if (fitsY) return; @@ -1633,7 +1634,7 @@ class VirtualRenderer { // If it can fully fit in the screen, scroll down until it fits on the screen // if it cannot fully fit, scroll so that the cursor is at the top of the screen // to fit as much as possible. - if (textLines.length * this.lineHeight < height) { + if (ghostTextHeight < height) { this.scrollBy(0, (textLines.length - 1) * this.lineHeight); } else { this.scrollBy(0, pixelPosition.top);