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

Reveal closer to top (center body, not signature) #80727

Merged
merged 7 commits into from Feb 5, 2020

Conversation

@wkornewald
Copy link
Contributor

wkornewald commented Sep 11, 2019

This improves the jump to definition and outline UX esp. for long functions/classes.

Fixes #80726 (see issue description for reasoning behind this PR).

@wkornewald wkornewald force-pushed the wkornewald:reveal-closer-to-top branch from a4b380f to 17cf5a2 Sep 11, 2019
@jrieken jrieken requested review from alexdima and rebornix Sep 12, 2019
Copy link
Member

alexdima left a comment

This cannot be implemented by changing the behavior of VerticalRevealType.Center and VerticalRevealType.CenterIfOutsideViewport. These are well established and used by various places inside VS Code, and by extensions, e.g. the vim extension makes use of the revealLine command which accepts as argument "at": "center".

This could be implemented by adding a new kind of VerticalRevealType in the editor and adopting this new kind for the desired features (go to definition, etc.)

@wkornewald wkornewald force-pushed the wkornewald:reveal-closer-to-top branch 2 times, most recently from 28113cb to ab3692a Sep 14, 2019
@wkornewald

This comment has been minimized.

Copy link
Contributor Author

wkornewald commented Sep 14, 2019

This needs some testing whether the UX really is better in all cases. I had to make quite a few changes and I removed the IfOutsideViewport part to also behave more like the JetBrains IDEs. They always jump to the same (consistent) screen location.

Only the history navigation is different because JetBrains remembers the scroll position of each history entry. If you resize the window it always scrolls like with go to definition. I think VS Code should remember the scroll position, too, but I didn't look into that as part of this PR.

Very small viewports (e.g. when pressing SHIFT+F12) needed some tuning, so you can see enough preceding lines. The code ensures a minimum scroll position of 100px from the top.

What do you think?

@alexdima

This comment has been minimized.

Copy link
Member

alexdima commented Sep 16, 2019

@wkornewald I am sorry, but I cannot accept a PR that changes the world, because I cannot estimate its impact. That was really the point of my first review. Can you please make a change that is limited to go to definition or outline? Pick one, and let's tweak that one. If we receive positive feedback (I can also take the change to VSCode's UX call), we can then expand it to the entire universe of reveal calls... but I cannot accept such a huge PR because I cannot take responsibility for such a large impact change.

@wkornewald wkornewald force-pushed the wkornewald:reveal-closer-to-top branch from ab3692a to ada2e60 Sep 16, 2019
wkornewald added 2 commits Sep 16, 2019
This scrolls the target closer to the top,
but leaves sufficient room for e.g. class/function comments,
while showing more of the source below the target
(i.e.. more of the class/function body).
@wkornewald wkornewald force-pushed the wkornewald:reveal-closer-to-top branch from ada2e60 to 00ef6f1 Sep 16, 2019
@wkornewald

This comment has been minimized.

Copy link
Contributor Author

wkornewald commented Sep 16, 2019

@alexandrudima I see, then let's give outline a try. The change is split into one commit for the infrastructure and one for the actual outline behavior change. I hope that's easier to review and simplifies testing individual aspects.

@wkornewald wkornewald requested a review from alexdima Sep 30, 2019
@alexdima alexdima added this to the October 2019 milestone Oct 2, 2019
@jrieken jrieken removed their assignment Oct 7, 2019
@alexdima alexdima modified the milestones: October 2019, November 2019 Oct 30, 2019
@alexdima alexdima modified the milestones: November 2019, January 2020 Jan 6, 2020
alexdima added 5 commits Feb 5, 2020
@alexdima alexdima modified the milestones: January 2020, February 2020 Feb 5, 2020
@alexdima alexdima removed the request for review from rebornix Feb 5, 2020
@alexdima

This comment has been minimized.

Copy link
Member

alexdima commented Feb 5, 2020

Thank you!

@alexdima alexdima merged commit 395f34d into microsoft:master Feb 5, 2020
4 of 5 checks passed
4 of 5 checks passed
linux
Details
windows
Details
darwin
Details
VS Code queued
Details
license/cla All CLA requirements met.
Details
@alexdima

This comment has been minimized.

Copy link
Member

alexdima commented Feb 5, 2020

I have created #90068 to track the further adoption of this reveal type for other actions.

@wkornewald

This comment has been minimized.

Copy link
Contributor Author

wkornewald commented Feb 5, 2020

Thank you, that's great news.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.