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

Feature #94173: Scroll when inserting suggestion #94327

Merged

Conversation

shskwmt
Copy link
Contributor

@shskwmt shskwmt commented Apr 2, 2020

This PR fixes #94173

94173

@jrieken jrieken self-assigned this Apr 3, 2020
@jrieken jrieken modified the milestones: March 2020, April 2020 Apr 3, 2020
@@ -273,6 +273,9 @@ export class SuggestController implements IEditorContribution {
// keep item in memory
this._memoryService.memorize(model, this.editor.getPosition(), item);

// keep line number for scrolling
const initialLineNumber = this.editor.getPosition().lineNumber;
Copy link
Member

Choose a reason for hiding this comment

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

You should be using this StableEditorScrollState

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrieken
Thanks for your review. I've update the PR.

@shskwmt shskwmt force-pushed the 94173-scroll-when-inserting-suggestion branch from fed8165 to 8a4b4e9 Compare April 3, 2020 12:37
@shskwmt shskwmt requested a review from jrieken April 3, 2020 12:45
@jrieken jrieken requested a review from alexdima April 17, 2020 09:46
@jrieken
Copy link
Member

jrieken commented Apr 17, 2020

Adding @alexdima to review the changes in the scroll state. Tested this, it kinda works. When inserting import-completion by current line doesn't move anymore, but when undoing it seems to shift up and that's not nice

Apr-17-2020 11-48-40

@shskwmt
Copy link
Contributor Author

shskwmt commented Apr 23, 2020

@jrieken
Thank you.
I added pushUndoStop() to stop undoing.

20200423

Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

LGTM, except the pushUndoStop call in restoreRelativeVerticalPositionOfCursor

src/vs/editor/browser/core/editorState.ts Outdated Show resolved Hide resolved
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

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

There is still a call to editor.pushUndoStop in restoreRelativeVerticalPositionOfCursor. It needs to be removed from there, as restoring the scrolling position should not affect the undo stack in any way.

src/vs/editor/browser/core/editorState.ts Outdated Show resolved Hide resolved
@shskwmt
Copy link
Contributor Author

shskwmt commented Apr 27, 2020

@alexdima
Thanks.
Could you please review and let me know if there is any problem.

@@ -290,6 +293,9 @@ export class SuggestController implements IEditorContribution {
adjustWhitespace: !(suggestion.insertTextRules! & CompletionItemInsertTextRule.KeepWhitespace)
});

scrollState.restoreRelativeVerticalPositionOfCursor(this.editor);
this.editor.pushUndoStop();
Copy link
Member

@jrieken jrieken Apr 27, 2020

Choose a reason for hiding this comment

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

Why are you adding an undo-stop at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrieken
I'm sorry, I misunderstood. I'll remove it.

@jrieken jrieken modified the milestones: April 2020, May 2020 Apr 27, 2020
@jrieken
Copy link
Member

jrieken commented Apr 27, 2020

The scroll jump after undo still happens (#94327 (comment)). Adding another undo-stop is not the right approach here but I also don't know what to do. @alexdima can you advice? We now control the scroll state when doing the suggest edits but I don't thing we have a say when undoing these changes.

@alexdima
Copy link
Member

Effectively, when doing and inserting an import, the editor is scrolled down by 1 line (scrollTop = scrollTop + lineHeight)... But when undoing the scroll top is maintained constant, the lines move up because the import line is removed.

The undo does not capture the scrollTop of the file because in the meantime between doing and undoing the file might have been resized, the scroll position might be somewhere outside of the cursor position... Since last milestone it is possible that the file was closed and reopened in the meantime.

That is just how undo/redo works currently, and is not 100% awesome for this specific case, and we can make new issues about that if we want to, but I don't know how to tackle it, nor do I think that it should block this PR...

@jrieken jrieken merged commit 896de33 into microsoft:master May 4, 2020
@github-actions github-actions bot locked and limited conversation to collaborators Jun 18, 2020
@shskwmt shskwmt deleted the 94173-scroll-when-inserting-suggestion branch October 2, 2020 02:09
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Virtually prevent pushing code down when new lines are automatically added
3 participants