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] #27649 pin intellisense documentation widget to top #62115

Merged
merged 1 commit into from Dec 3, 2018

Conversation

Projects
None yet
4 participants
@riltsken
Copy link
Contributor

riltsken commented Oct 30, 2018

Fixes: #27649

Long docs when cursor is near the bottom will jump to the top to allow for more space, but if the next suggestion docs are smaller it will jump back down creating a jarring experience when flipping through suggestions quickly. Pin the widget to the top if it flips to the top once so that the experience is more fluid.

This is just one way we might handle it. Open to suggestions as this is my first PR to VSCode.

@msftclas

This comment has been minimized.

Copy link

msftclas commented Oct 30, 2018

CLA assistant check
All CLA requirements met.

@riltsken riltsken force-pushed the riltsken:fix-27649-pin-doc-widget-top branch from 8328061 to 8586c32 Oct 30, 2018

@kieferrm kieferrm requested a review from ramya-rao-a Nov 2, 2018

this.docsPositionPreviousWidgetY < widgetY &&
!this.preferDocPositionTop) {
this.preferDocPositionTop = true;
this.adjustDocsPosition();

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Nov 19, 2018

Member

Why do we need to call this.adjustDocsPosition again here? None of the constants declared above this if condition gets changed by this.preferDocPositionTop = true

This comment has been minimized.

@riltsken

riltsken Nov 19, 2018

Contributor

I will double check tomorrow when I have a moment, but if I remember correctly I believe it's because at this point it has now flipped back to the bottom so I am forcing it to rerender back to the top. I will verify though.

This comment has been minimized.

@riltsken

riltsken Nov 20, 2018

Contributor

To give context and explain a bit better I will outline the original behavior, without the adjustDocsPosition, and with the adjustDocsPosition

Original behavior
When opening intellisense documentation. Moving through each option with the arrow keys the documentation will swap up and down depending on the space creating the jarring experience.

Without adjustDocsPosition
When opening intellisense documentation. The first time the docs position swaps from the bottom to the top it will trigger the preferDocPositionTop flag so subsequent moves through the documentation suggestions will stick to the top after the first one triggered goes to the bottom.

With adjustDocsPosition
When opening intellisense documentation. The first time the docs position adjusts it will trigger the preferDocPositionTop flag but prevent any noticeable difference that it had swapped at all and just look like it was at the top the whole time.

This is because of this line right above which creates a side effect causing the documentation position to re-evaluate where things should go.

const cursorCoords = this.editor.getScrolledVisiblePosition(this.editor.getPosition());

Let me know if I can answer this question more clearly or differently to help. Thank you for looking and commenting!

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Nov 21, 2018

Member

In that case, why not exit after calling adjustDocsPosition?

ie. Replace the this.adjustDocsPosition() here with

this.docsPositionPreviousWidgetY = widgetY;
this.adjustDocsPosition();
return;

This comment has been minimized.

@riltsken

riltsken Nov 21, 2018

Contributor

Yes makes sense :) I'll update and rebase.

@ramya-rao-a ramya-rao-a requested a review from jrieken Nov 19, 2018

@ramya-rao-a ramya-rao-a assigned jrieken and unassigned ramya-rao-a Nov 19, 2018

@riltsken riltsken force-pushed the riltsken:fix-27649-pin-doc-widget-top branch from 8586c32 to 2619e9b Nov 21, 2018

@jrieken jrieken assigned ramya-rao-a and unassigned jrieken Nov 21, 2018

@@ -706,6 +709,8 @@ export class SuggestWidget implements IContentWidget, IListVirtualDelegate<IComp
}

showSuggestions(completionModel: CompletionModel, selectionIndex: number, isFrozen: boolean, isAuto: boolean): void {
this.preferDocPositionTop = false;

This comment has been minimized.

@ramya-rao-a

ramya-rao-a Nov 22, 2018

Member

Shouldnt we reset this.docsPositionPreviousWidgetY here as well?

This comment has been minimized.

@riltsken

riltsken Nov 25, 2018

Contributor

Updated :)

[fix] #27649 pin intellisense documentation widget to top
Fixes: #27649

Long docs when cursor is near the bottom will jump to the top to allow for more space, but if the next suggestion docs are smaller it will jump back down creating a jarring experience when flipping through suggestions quickly. Pin the widget to the top if it flips to the top once so that the experience is more fluid.

This is just one way we might handle it. Open to suggestions as this is my first PR to VSCode.

@riltsken riltsken force-pushed the riltsken:fix-27649-pin-doc-widget-top branch from 2619e9b to 357fe9f Nov 25, 2018

@jrieken jrieken removed their request for review Nov 26, 2018

@ramya-rao-a ramya-rao-a added this to the November 2018 milestone Dec 3, 2018

@ramya-rao-a ramya-rao-a merged commit 9e62a05 into Microsoft:master Dec 3, 2018

2 checks passed

VS Code #20181125.1 succeeded
Details
license/cla All CLA requirements met.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment