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.

Sign up for GitHub

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

Jump to bottom

Move to a simpler event source system for nav bars #54432

Merged
merged 6 commits into from
Jun 28, 2021
Merged

Move to a simpler event source system for nav bars #54432

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8b51a85
Simplify delay
CyrusNajmabadi Jun 27, 2021
aa4b86f
Simplify delay
CyrusNajmabadi Jun 27, 2021
16b2d20
Move to a simpler event source system for nav bars.
CyrusNajmabadi Jun 27, 2021
b97477b
remove semantic version check
CyrusNajmabadi Jun 27, 2021
e3ea8d9
Docs
CyrusNajmabadi Jun 27, 2021
ba968b8
Fix test
CyrusNajmabadi Jun 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
132 changes: 29 additions & 103 deletions src/EditorFeatures/Core/Implementation/NavigationBar/NavigationBarController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,11 @@
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Editor.Implementation.Classification;
using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
using Microsoft.CodeAnalysis.Editor.Shared.Tagging;
using Microsoft.CodeAnalysis.Editor.Shared.Utilities;
using Microsoft.CodeAnalysis.Editor.Tagging;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
Expand All @@ -36,7 +38,6 @@ internal partial class NavigationBarController : ForegroundThreadAffinitizedObje
private readonly IAsynchronousOperationListener _asyncListener;

private bool _disconnected = false;
private Workspace? _workspace;

/// <summary>
/// Latest model and selected items produced once <see cref="DetermineSelectedItemInfoAsync"/> completes and
Expand All @@ -51,6 +52,8 @@ internal partial class NavigationBarController : ForegroundThreadAffinitizedObje
/// </summary>
private (ImmutableArray<NavigationBarProjectItem> projectItems, NavigationBarProjectItem? selectedProjectItem, NavigationBarModel model, NavigationBarSelectedTypeAndMember selectedInfo) _lastPresentedInfo;

private readonly ITaggerEventSource _eventSource;

public NavigationBarController(
IThreadingContext threadingContext,
INavigationBarPresenter presenter,
Expand All @@ -70,80 +73,58 @@ public NavigationBarController(
presenter.DropDownFocused += OnDropDownFocused;
presenter.ItemSelected += OnItemSelected;

subjectBuffer.PostChanged += OnSubjectBufferPostChanged;

// Initialize the tasks to be an empty model so we never have to deal with a null case.
_latestModelAndSelectedInfo_OnlyAccessOnUIThread.model = new(
ImmutableArray<NavigationBarItem>.Empty,
semanticVersionStamp: default,
itemService: null!);
_latestModelAndSelectedInfo_OnlyAccessOnUIThread.model = new(ImmutableArray<NavigationBarItem>.Empty, itemService: null!);
_latestModelAndSelectedInfo_OnlyAccessOnUIThread.selectedInfo = new(typeItem: null, memberItem: null);

_modelTask = Task.FromResult(_latestModelAndSelectedInfo_OnlyAccessOnUIThread.model);

_eventSource = new CompilationAvailableTaggerEventSource(
subjectBuffer,
asyncListener,
TaggerEventSources.OnTextChanged(subjectBuffer),
TaggerEventSources.OnDocumentActiveContextChanged(subjectBuffer),
TaggerEventSources.OnWorkspaceChanged(subjectBuffer, asyncListener),
TaggerEventSources.OnWorkspaceRegistrationChanged(subjectBuffer));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already have a system for connecting to events and abstracting over them. This system also handles hooking up buffers to workspaces, making it so that this type doesn't need to track workspaces at all. Also, a new CompilationAvailableTaggerEventSource source was added as we've seen a bug when roslyn starts where the information can be slightly inaccurate (since we use a frozen-partial snapshot), and we want to move from that to correct data once the compilation is actually available.

_eventSource.Changed += OnEventSourceChanged;
_eventSource.Connect();
}

public void SetWorkspace(Workspace? newWorkspace)
{
DisconnectFromWorkspace();

if (newWorkspace != null)
{
ConnectToWorkspace(newWorkspace);
}
StartModelUpdateAndSelectedItemUpdateTasks();
}

private void ConnectToWorkspace(Workspace workspace)
private void StartModelUpdateAndSelectedItemUpdateTasks()
{
// If we disconnected before the workspace ever connected, just disregard
// If we're disconnected , just disregard
if (_disconnected)
{
return;
}

_workspace = workspace;
_workspace.WorkspaceChanged += this.OnWorkspaceChanged;
_workspace.DocumentActiveContextChanged += this.OnDocumentActiveContextChanged;

if (IsForeground())
{
ConnectToNewWorkspace();
StartModelUpdateAndSelectedItemUpdateTasksOnUIThread();
}
else
{
var asyncToken = _asyncListener.BeginAsyncOperation(nameof(ConnectToWorkspace));
var asyncToken = _asyncListener.BeginAsyncOperation(nameof(StartModelUpdateAndSelectedItemUpdateTasks));
Task.Run(async () =>
{
await ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync();

ConnectToNewWorkspace();
StartModelUpdateAndSelectedItemUpdateTasksOnUIThread();
}).CompletesAsyncOperation(asyncToken);
}

return;

void ConnectToNewWorkspace()
{
// For the first time you open the file, we'll start immediately
StartModelUpdateAndSelectedItemUpdateTasks(modelUpdateDelay: 0);
}
}

private void DisconnectFromWorkspace()
private void OnEventSourceChanged(object? sender, TaggerEventArgs e)
{
if (_workspace != null)
{
_workspace.DocumentActiveContextChanged -= this.OnDocumentActiveContextChanged;
_workspace.WorkspaceChanged -= this.OnWorkspaceChanged;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much simpler. no more explicit event hookups on our end to a lot of disparate sources.

_workspace = null;
}
StartModelUpdateAndSelectedItemUpdateTasks();
}

public void Disconnect()
{
AssertIsForeground();
DisconnectFromWorkspace();

_subjectBuffer.PostChanged -= OnSubjectBufferPostChanged;

_presenter.CaretMoved -= OnCaretMoved;
_presenter.ViewFocused -= OnViewFocused;
Expand All @@ -153,81 +134,26 @@ public void Disconnect()

_presenter.Disconnect();

_eventSource.Changed -= OnEventSourceChanged;
_eventSource.Disconnect();

_disconnected = true;

// Cancel off any remaining background work
_modelTaskCancellationSource.Cancel();
_selectedItemInfoTaskCancellationSource.Cancel();
}

private void OnWorkspaceChanged(object? sender, WorkspaceChangeEventArgs args)
{
// We're getting an event for a workspace we already disconnected from
if (args.NewSolution.Workspace != _workspace)
{
return;
}

// If the displayed project is being renamed, retrigger the update
if (args.Kind == WorkspaceChangeKind.ProjectChanged && args.ProjectId != null)
{
var oldProject = args.OldSolution.GetRequiredProject(args.ProjectId);
var newProject = args.NewSolution.GetRequiredProject(args.ProjectId);

if (oldProject.Name != newProject.Name)
{
var currentContextDocumentId = _workspace.GetDocumentIdInCurrentContext(_subjectBuffer.AsTextContainer());

if (currentContextDocumentId != null && currentContextDocumentId.ProjectId == args.ProjectId)
{
StartModelUpdateAndSelectedItemUpdateTasks(modelUpdateDelay: 0);
}
}
}

if (args.Kind == WorkspaceChangeKind.DocumentChanged &&
args.OldSolution == args.NewSolution)
{
var currentContextDocumentId = _workspace.GetDocumentIdInCurrentContext(_subjectBuffer.AsTextContainer());
if (currentContextDocumentId != null && currentContextDocumentId == args.DocumentId)
{
// The context has changed, so update everything.
StartModelUpdateAndSelectedItemUpdateTasks(modelUpdateDelay: 0);
}
}
}

private void OnDocumentActiveContextChanged(object? sender, DocumentActiveContextChangedEventArgs args)
{
AssertIsForeground();
if (args.Solution.Workspace != _workspace)
return;

var currentContextDocumentId = _workspace.GetDocumentIdInCurrentContext(_subjectBuffer.AsTextContainer());
if (args.NewActiveContextDocumentId == currentContextDocumentId ||
args.OldActiveContextDocumentId == currentContextDocumentId)
{
// if the active context changed, recompute the types/member as they may be changed as well.
StartModelUpdateAndSelectedItemUpdateTasks(modelUpdateDelay: 0);
}
}

private void OnSubjectBufferPostChanged(object? sender, EventArgs e)
{
AssertIsForeground();
StartModelUpdateAndSelectedItemUpdateTasks(modelUpdateDelay: TaggerConstants.MediumDelay);
}

private void OnCaretMoved(object? sender, EventArgs e)
{
AssertIsForeground();
StartSelectedItemUpdateTask(delay: TaggerConstants.NearImmediateDelay);
StartSelectedItemUpdateTask();
}

private void OnViewFocused(object? sender, EventArgs e)
{
AssertIsForeground();
StartSelectedItemUpdateTask(delay: TaggerConstants.ShortDelay);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the need for this fine-grained delays was not really necessary. We need to do the work anyways, and the user isn't going to notice us cmoputing things 50 vs 200 ms later for things like navbars.

StartSelectedItemUpdateTask();
}

private void OnDropDownFocused(object? sender, EventArgs e)
Expand Down Expand Up @@ -395,7 +321,7 @@ private async Task ProcessItemSelectionAsync(NavigationBarItem item, Cancellatio
// Now that the edit has been done, refresh to make sure everything is up-to-date.
// Have to make sure we come back to the main thread for this.
AssertIsForeground();
StartModelUpdateAndSelectedItemUpdateTasks(modelUpdateDelay: 0);
StartModelUpdateAndSelectedItemUpdateTasksOnUIThread();
}
}
}
39 changes: 13 additions & 26 deletions ...torFeatures/Core/Implementation/NavigationBar/NavigationBarController_ModelComputation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
using Microsoft.CodeAnalysis.Editor.Shared.Tagging;
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Internal.Log;
using Microsoft.CodeAnalysis.Shared.Extensions;
Expand All @@ -34,7 +35,7 @@ internal partial class NavigationBarController
/// <summary>
/// Starts a new task to compute the model based on the current text.
/// </summary>
private void StartModelUpdateAndSelectedItemUpdateTasks(int modelUpdateDelay)
private void StartModelUpdateAndSelectedItemUpdateTasksOnUIThread()
{
AssertIsForeground();

Expand All @@ -48,22 +49,22 @@ private void StartModelUpdateAndSelectedItemUpdateTasks(int modelUpdateDelay)

// Enqueue a new computation for the model
var asyncToken = _asyncListener.BeginAsyncOperation(GetType().Name + ".StartModelUpdateTask");
_modelTask = ComputeModelAfterDelayAsync(_modelTask, textSnapshot, modelUpdateDelay, cancellationToken);
_modelTask = ComputeModelAfterDelayAsync(_modelTask, textSnapshot, cancellationToken);
_modelTask.CompletesAsyncOperation(asyncToken);

StartSelectedItemUpdateTask(delay: 0);
StartSelectedItemUpdateTask();
}

private static async Task<NavigationBarModel> ComputeModelAfterDelayAsync(
Task<NavigationBarModel> modelTask, ITextSnapshot textSnapshot, int modelUpdateDelay, CancellationToken cancellationToken)
Task<NavigationBarModel> modelTask, ITextSnapshot textSnapshot, CancellationToken cancellationToken)
{
var previousModel = await modelTask.ConfigureAwait(false);
if (!cancellationToken.IsCancellationRequested)
{
try
{
await Task.Delay(modelUpdateDelay, cancellationToken).ConfigureAwait(false);
return await ComputeModelAsync(previousModel, textSnapshot, cancellationToken).ConfigureAwait(false);
await Task.Delay(TaggerConstants.ShortDelay, cancellationToken).ConfigureAwait(false);
return await ComputeModelAsync(textSnapshot, cancellationToken).ConfigureAwait(false);
}
catch (OperationCanceledException)
{
Expand All @@ -82,8 +83,7 @@ private static async Task<NavigationBarModel> ComputeModelAfterDelayAsync(
/// <summary>
/// Computes a model for the given snapshot.
/// </summary>
private static async Task<NavigationBarModel> ComputeModelAsync(
NavigationBarModel lastCompletedModel, ITextSnapshot snapshot, CancellationToken cancellationToken)
private static async Task<NavigationBarModel> ComputeModelAsync(ITextSnapshot snapshot, CancellationToken cancellationToken)
{
// Ensure we switch to the threadpool before calling GetDocumentWithFrozenPartialSemantics. It ensures
// that any IO that performs is not potentially on the UI thread.
Expand All @@ -102,33 +102,21 @@ private static async Task<NavigationBarModel> ComputeModelAsync(
var languageService = document.GetLanguageService<INavigationBarItemService>();
if (languageService != null)
{
// check whether we can re-use lastCompletedModel. otherwise, update lastCompletedModel here.
// the model should be only updated here
if (lastCompletedModel != null)
{
var semanticVersion = await document.Project.GetDependentSemanticVersionAsync(CancellationToken.None).ConfigureAwait(false);
if (lastCompletedModel.SemanticVersionStamp == semanticVersion && SpanStillValid(lastCompletedModel, snapshot, cancellationToken))
{
// it looks like we can re-use previous model
return lastCompletedModel;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the semantic version is computed based on things like file versions. but it's not really safe to use when dealing with fvrozen partial semantics as the file versions may be the same, but the overall symbol information might not be. now that we just recompute whenever an appropriate change happens, we really don't need this.


using (Logger.LogBlock(FunctionId.NavigationBar_ComputeModelAsync, cancellationToken))
{
var items = await languageService.GetItemsAsync(document, snapshot, cancellationToken).ConfigureAwait(false);
var version = await document.Project.GetDependentSemanticVersionAsync(cancellationToken).ConfigureAwait(false);
return new NavigationBarModel(items, version, languageService);
return new NavigationBarModel(items, languageService);
}
}

return new NavigationBarModel(ImmutableArray<NavigationBarItem>.Empty, new VersionStamp(), null);
return new NavigationBarModel(ImmutableArray<NavigationBarItem>.Empty, itemService: null);
}

/// <summary>
/// Starts a new task to compute what item should be selected.
/// </summary>
private void StartSelectedItemUpdateTask(int delay)
private void StartSelectedItemUpdateTask()
{
AssertIsForeground();

Expand All @@ -143,19 +131,18 @@ private void StartSelectedItemUpdateTask(int delay)
var cancellationToken = _selectedItemInfoTaskCancellationSource.Token;

var asyncToken = _asyncListener.BeginAsyncOperation(GetType().Name + ".StartSelectedItemUpdateTask");
var selectedItemInfoTask = DetermineSelectedItemInfoAsync(_modelTask, delay, subjectBufferCaretPosition.Value, cancellationToken);
var selectedItemInfoTask = DetermineSelectedItemInfoAsync(_modelTask, subjectBufferCaretPosition.Value, cancellationToken);
selectedItemInfoTask.CompletesAsyncOperation(asyncToken);
}

private async Task DetermineSelectedItemInfoAsync(
Task<NavigationBarModel> lastModelTask,
int delay,
SnapshotPoint caretPosition,
CancellationToken cancellationToken)
{
// First wait the delay before doing any other work. That way if we get canceled due to other events (like
// the user moving around), we don't end up doing anything, and the next task can take over.
await Task.Delay(delay, cancellationToken).ConfigureAwait(false);
await Task.Delay(TaggerConstants.NearImmediateDelay, cancellationToken).ConfigureAwait(false);

var lastModel = await lastModelTask.ConfigureAwait(false);
if (cancellationToken.IsCancellationRequested)
Expand Down
11 changes: 1 addition & 10 deletions src/EditorFeatures/Core/Implementation/NavigationBar/NavigationBarModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,18 @@
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Editor.Implementation.NavigationBar
{
internal sealed class NavigationBarModel
{
public ImmutableArray<NavigationBarItem> Types { get; }

/// <summary>
/// The VersionStamp of the project when this model was computed.
/// </summary>
public VersionStamp SemanticVersionStamp { get; }

public INavigationBarItemService ItemService { get; }

public NavigationBarModel(ImmutableArray<NavigationBarItem> types, VersionStamp semanticVersionStamp, INavigationBarItemService itemService)
public NavigationBarModel(ImmutableArray<NavigationBarItem> types, INavigationBarItemService itemService)
{
Contract.ThrowIfNull(types);

this.Types = types;
this.SemanticVersionStamp = semanticVersionStamp;
this.ItemService = itemService;
}
}
Expand Down