-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Document outline updates #69921
Document outline updates #69921
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
using System.Runtime.CompilerServices; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using System.Windows; | ||
using Microsoft.CodeAnalysis; | ||
using Microsoft.CodeAnalysis.Collections; | ||
using Microsoft.CodeAnalysis.Editor.Shared.Extensions; | ||
|
@@ -63,6 +64,7 @@ internal sealed partial class DocumentOutlineViewModel : INotifyPropertyChanged, | |
|
||
// Mutable state. Should only update on UI thread. | ||
|
||
private Visibility _visibility_doNotAccessDirectly = Visibility.Visible; | ||
private SortOption _sortOption_doNotAccessDirectly = SortOption.Location; | ||
private string _searchText_doNotAccessDirectly = ""; | ||
private ImmutableArray<DocumentSymbolDataViewModel> _documentSymbolViewModelItems_doNotAccessDirectly = ImmutableArray<DocumentSymbolDataViewModel>.Empty; | ||
|
@@ -111,7 +113,7 @@ public DocumentOutlineViewModel( | |
_taggerEventSource.Connect(); | ||
|
||
// queue initial model update | ||
_workQueue.AddWork(default(VoidResult)); | ||
_workQueue.AddWork(); | ||
} | ||
|
||
public void Dispose() | ||
|
@@ -129,7 +131,7 @@ private static DocumentOutlineViewState CreateEmptyViewState(ITextSnapshot curre | |
IntervalTree<DocumentSymbolDataViewModel>.Empty); | ||
|
||
private void OnEventSourceChanged(object sender, TaggerEventArgs e) | ||
=> _workQueue.AddWork(default(VoidResult), cancelExistingWork: true); | ||
=> _workQueue.AddWork(cancelExistingWork: true); | ||
|
||
/// <summary> | ||
/// Keeps track if we're currently in the middle of navigating or not. For example, when the user clicks on an | ||
|
@@ -173,6 +175,23 @@ private DocumentOutlineViewState LastPresentedViewState | |
} | ||
} | ||
|
||
/// <remarks>This property is bound to the UI. However, it is only read/written by the UI. We only act as | ||
/// storage for the value. When this value is true, UI updates are deferred.</remarks> | ||
public Visibility Visibility | ||
{ | ||
get | ||
{ | ||
_threadingContext.ThrowIfNotOnUIThread(); | ||
return _visibility_doNotAccessDirectly; | ||
} | ||
|
||
set | ||
{ | ||
_threadingContext.ThrowIfNotOnUIThread(); | ||
_visibility_doNotAccessDirectly = value; | ||
} | ||
} | ||
|
||
/// <remarks>This property is bound to the UI. However, it is only read/written by the UI. We only act as | ||
/// storage for the value. When the value changes, the sorting is actually handled by | ||
/// DocumentSymbolDataViewModelSorter.</remarks> | ||
|
@@ -208,7 +227,7 @@ public string SearchText | |
_threadingContext.ThrowIfNotOnUIThread(); | ||
_searchText_doNotAccessDirectly = value; | ||
|
||
_workQueue.AddWork(default(VoidResult), cancelExistingWork: true); | ||
_workQueue.AddWork(cancelExistingWork: true); | ||
} | ||
} | ||
|
||
|
@@ -250,6 +269,17 @@ static void ExpandOrCollapse(ImmutableArray<DocumentSymbolDataViewModel> models, | |
|
||
private async ValueTask ComputeViewStateAsync(CancellationToken cancellationToken) | ||
{ | ||
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); | ||
if (_isDisposed) | ||
return; | ||
|
||
if (Visibility != Visibility.Visible) | ||
{ | ||
// Retry the update after a delay | ||
_workQueue.AddWork(cancelExistingWork: true); | ||
return; | ||
} | ||
|
||
// Do any expensive semantic/computation work in the background. | ||
await TaskScheduler.Default; | ||
cancellationToken.ThrowIfCancellationRequested(); | ||
|
@@ -262,6 +292,13 @@ private async ValueTask ComputeViewStateAsync(CancellationToken cancellationToke | |
if (_isDisposed) | ||
return; | ||
|
||
if (Visibility != Visibility.Visible) | ||
{ | ||
// Retry the update after a delay | ||
_workQueue.AddWork(cancelExistingWork: true); | ||
return; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, that works for me There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
var searchText = this.SearchText; | ||
var sortOption = this.SortOption; | ||
var lastPresentedViewState = this.LastPresentedViewState; | ||
|
@@ -306,6 +343,13 @@ private async ValueTask ComputeViewStateAsync(CancellationToken cancellationToke | |
if (_isDisposed) | ||
return; | ||
|
||
if (Visibility != Visibility.Visible) | ||
{ | ||
// Retry the update after a delay | ||
_workQueue.AddWork(cancelExistingWork: true); | ||
return; | ||
} | ||
|
||
this.LastPresentedViewState = newViewState; | ||
this.DocumentSymbolViewModelItems = newViewModelItems; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was exactly my conclusion