Vscode: Consider adopting TextEditorSelectionRevealType.NearTop for other actions

[alexdima]

Currently, after the merge of https://github.com/microsoft/vscode/pull/80727 , the outline panel reveals its elements using NearTop. We should try this out and consider using this reveal type for other actions, like for go to definition.

[bpasero]

I wonder if we should simply change the workbench editor option revealInCenterIfOutsideViewport to behave like you described today in the standup: visually to the top with some padding. It could then be renamed to reveaIfOutsideViewport

[alexdima]

I was against that because there are many things being revealed in the editor. That is what the PR initially did, it changed the editor's implementation of in CenterIfOutsideViewport. I suggested that we do an incremental adoption to the places that make most sense, this new reveal type might not work well for things like stepping through code, etc.

[jrieken]

List of potential candidates 👇 I agree that we shouldn't use the big hammer. I feel that "reveal" gestures should be near top, like outline, quick outline and breadcrumb but not necessarily things like go to def and friends

• [ ] src/vs/workbench/services/history/browser/history.ts
  354, 4: selectionRevealType: TextEditorSelectionRevealType.CenterIfOutsideViewport
• [ ] src/vs/workbench/contrib/tasks/browser/abstractTaskService.ts
  965, 6: selectionRevealType: TextEditorSelectionRevealType.CenterIfOutsideViewport
• [x] src/vs/workbench/contrib/outline/browser/outlinePane.ts
  633, 5: selectionRevealType: TextEditorSelectionRevealType.NearTop,
• [x] src/vs/workbench/contrib/debug/common/debugSource.ts
  105, 5: selectionRevealType: TextEditorSelectionRevealType.CenterIfOutsideViewport,
• [x] src/vs/workbench/contrib/debug/browser/breakpointsView.ts
  637, 4: selectionRevealType: TextEditorSelectionRevealType.CenterIfOutsideViewport,
• [ ] src/vs/workbench/browser/parts/editor/breadcrumbsControl.ts
  494, 7: selectionRevealType: TextEditorSelectionRevealType.CenterIfOutsideViewport
• [x] src/vs/editor/contrib/gotoSymbol/symbolNavigation.ts
  131, 5: selectionRevealType: TextEditorSelectionRevealType.CenterIfOutsideViewport
• [x] src/vs/editor/contrib/gotoSymbol/goToCommands.ts
  170, 5: selectionRevealType: TextEditorSelectionRevealType.CenterIfOutsideViewport
• [x] src/vs/editor/contrib/gotoError/gotoError.ts
  400, 52: revealIfOpened: true, selectionRevealType: TextEditorSelectionRevealType.CenterIfOutsideViewport, selection: newMarker }

[wkornewald]

My original change was intended exactly for go to def because currently, by centering, long functions don't fit on the lower half of the screen and the upper half of the screen mostly shows unrelated code you're probably not interested in (apart from a few lines for comments, but NearTop already leaves enough space for those).

By the way, the JetBrains IDEs have a behavior very similar to NearTop everywhere, so someone there most likely already did the usability testing and found NearTop to be better for everything.

[alexdima]

@wkornewald @bpasero I still believe debugging should not use NearTop, so I don't think changing it everywhere without a conscious decision works:

| Current | With NearTop |
|---|---|
| | |

[wkornewald]

@alexdima You're right. I suppose we need NearTopIfOutsideViewport for breakpoints? That's the IntelliJ behavior when debugging, BTW.

This would mean that the actual question is when to use NearTop vs NearTopIfOutsideViewport. The old Center[IfOutsideViewport] behavior is IMHO only needed in very rare cases like literal "center on screen" actions (e.g. in Vim extensions like you once mentioned).

I've tried NearTopIfOutsideViewport with go to definition. In case of functions/classes I think it's only useful if the whole body stays visible (e.g. two small functions below each other, both fitting on the screen). However, in case of variables it was mostly fine, except if the result was too close to the bottom. Still with IfOutsideViewport I often wish to have the target line highlighted for maybe 100ms because I sometimes don't immediately notice where the cursor has gone. This problem doesn't exist while debugging because the active line is always highlighted.

I guess the real solution is to analyze which lines belong to the target location (for functions/classes: starting from preceding docstring/comment up to body's closing character e.g. }). If the whole area is within the viewport we don't need to scroll (often the case during debugging, go to def for a variable, etc.). Otherwise we scroll that area to (or near) the top of the viewport. However, I don't know if doing that would be complicated with the current codebase and whether it's worth the effort if NearTop and NearTopIfOutsideViewport work well enough.

[jrieken]

I suppose we need NearTopIfOutsideViewport

:+1: for NearTopIfOutsideViewport - I have tried the NearTop with go to def and it didn't feel right but I can see it being good for locations that are brought into the view first

[jrieken]

@alexdima fyi I have pushed support for NearTopIfOutsideViewport and adopted it for outline and goto symbols. I am also considering to adopt this for breadcrumbs and quick outline

[alexdima]

Thanks @jrieken !

[jrieken]

@isidorn for debug and @sandy081 for markers

[isidorn]

Thought about this and I will keep the curent behavior for debug.
Because when a breakpoint gets hit it is usually even more important what happend before than what will happen after. Thus revealing in center actually makes sense for our use case.
I am open to suggestions if someone sees benefits for revealing near center.

[sandy081]

I tried other newly reveal positions and I liked revealPositionNearTop better for goto error. So adopting to it.