Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: try to scroll inline preview into view #5400

Merged
merged 11 commits into from
Nov 28, 2023
11 changes: 9 additions & 2 deletions src/autocomplete.js
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ 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;
Expand Down Expand Up @@ -143,6 +144,7 @@ class Autocomplete {
}
this.hideDocTooltip();
this.stickySelectionTimer.cancel();
this.popupTimer.cancel();
this.stickySelection = false;
}

Expand All @@ -160,9 +162,14 @@ class Autocomplete {
this.tooltipTimer.call(null, null);
return;
}

// Update the popup position after a short wait to account for potential scrolling
this.popupTimer.schedule();
this.tooltipTimer.schedule();
} else {
this.$updatePopupPosition();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the following code do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.tooltipTimer updates the position of the doc tooltip, this.popupTimer updates the position of the popup itself. They are both lang.delayedCalls which can be scheduled to happen after a timeout to allow some time for other stuff to happen.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it happen outside the this.inlineRenderer && this.inlineEnabled case? Is it to cancel if inline rendering gets disabled? Or is it a general improvement for the non-inline case? If so, we should update the GH description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this.popupTimer calls this.$updatePopupPosition so the same thing happens in both flows, only that for the inline case we wait 50ms to account for the scrolling which potentially happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried to make this a bit more clear in a new commit

this.tooltipTimer.call(null, null);
}
this.$updatePopupPosition();
this.tooltipTimer.call(null, null);
}

$onPopupShow(hide) {
Expand Down
40 changes: 40 additions & 0 deletions src/autocomplete/inline_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
];

Expand Down Expand Up @@ -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();
Expand Down
18 changes: 18 additions & 0 deletions src/virtual_renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1620,6 +1620,24 @@ 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);
var el = this.container;
var height = el.clientHeight;
andredcoliveira marked this conversation as resolved.
Show resolved Hide resolved
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) {
andredcoliveira marked this conversation as resolved.
Show resolved Hide resolved
this.scrollBy(0, (textLines.length - 1) * this.lineHeight);
} else {
this.scrollBy(0, pixelPosition.top);
}
}

}
Expand Down