Document outline updates #69921
Document outline updates #69921
Conversation
sharwell
commented
Sep 12, 2023
sharwell
commented
- Only move caret in editor if Document Outline has keyboard focus (Fixes Cursor jumps during editing with Document Outline enabled #69292)
- Expand outlining regions if necessary (Fixes Document Outline clicking isn't working with collapsed regions #69902)
- Avoid updating the UI if the tree view is not visible (Fixes Freeze when switching to some C# file tabs while Document Outline feature is enabled #69903)
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| // Retry the update after a delay | ||
| _workQueue.AddWork(cancelExistingWork: true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
instead of all 3, it likely suffices to just have teh first case. worst that happens is that the user switches away, and we finish up the work here. but any future work requsts will bail out at the top.
There was a problem hiding this comment.
Sure, that works for me
There was a problem hiding this comment.
On second thought, given the difficulty of proving the change works, I'm concerned about edge cases like switching back and forth between a Windows Forms designer and a C# source document (resulting in a visibility change for each switch). I'm more comfortable with the original change in terms of confidence that work is avoided when the UI is not visible. If you'd like, I could eliminate the middle of the three checks, though the explanation of why it's omitted might end up longer than the check itself.
| @@ -282,7 +288,8 @@ private void SymbolTreeItem_SourceUpdated(object sender, DataTransferEventArgs e | |||
| { | |||
| var textView = _viewTracker.GetActiveView(); | |||
| textView.TryMoveCaretToAndEnsureVisible( | |||
| symbolModel.Data.SelectionRangeSpan.TranslateTo(textView.TextSnapshot, SpanTrackingMode.EdgeInclusive).Start); | |||
| symbolModel.Data.SelectionRangeSpan.TranslateTo(textView.TextSnapshot, SpanTrackingMode.EdgeInclusive).Start, | |||
| _outliningManagerService); | |||
There was a problem hiding this comment.
how does this work, does this helper just know to expand in that case?
note: if so, it seems like a super weird helper since we'd always want htis behavior, in which case we should always require the outlining manager. but this is ok for now.
There was a problem hiding this comment.
This was exactly my conclusion
Could the searchText, sortOption, and lastPresentedViewState locals be captured in the first time on the ui thread in this method, and thus not need this UI thread hop? Refers to: src/VisualStudio/Core/Def/DocumentOutline/DocumentOutlineViewModel.cs:291 in 7404ed0. [](commit_id = 7404ed0, deletion_comment = False) |
sharwell
force-pushed
the
doc-outline-updates
branch
from
7404ed0
to
b7f2177
Compare
only if changes to those are guaranteed to kick off new computation. otherwise you risk the user chnaging UI state and that not being respected when updating the UI with new results. |
The background work to compute the items in the document could take a noticeable amount of time, so I prefer to capture these variables after the long-running work is complete. |
|
@arkalyanms Do you know what version this will ship in? |
Its inserted into the next 17.7 servicing build. We are waiting on the final build next week. Barring any unexpected regressions, it should catch 17.7.5. |
