-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Change navigation helpers to operate in two steps. #59798
Changes from all commits
779c8b0
e18cef8
95ef3e9
988cdf6
f3ad0ba
ec02282
9db7519
ac8820a
31dd3ee
67e91b0
d419a10
3596605
33b6fbc
5a7c62c
f963940
757220b
fc5df02
b7a3faf
a3a29fe
23491d4
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 | ||||
---|---|---|---|---|---|---|
|
@@ -37,19 +37,19 @@ public Task<bool> CanNavigateToLineAndOffsetAsync(Workspace workspace, DocumentI | |||||
public Task<bool> CanNavigateToPositionAsync(Workspace workspace, DocumentId documentId, int position, int virtualSpace, CancellationToken cancellationToken) | ||||||
=> SpecializedTasks.False; | ||||||
|
||||||
public async Task<bool> TryNavigateToSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken) | ||||||
public async Task<INavigableLocation?> GetLocationForSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken) | ||||||
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.
Suggested change
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. I actually liked the new name, since it's the DocNavigationService and it returns a NavigableLocation, i found it just fine to say GetLocation. but if you feewl strongly, i can change :) |
||||||
{ | ||||||
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); | ||||||
if (workspace is not InteractiveWindowWorkspace interactiveWorkspace) | ||||||
{ | ||||||
Debug.Fail("InteractiveDocumentNavigationService called with incorrect workspace!"); | ||||||
return false; | ||||||
return null; | ||||||
} | ||||||
|
||||||
if (interactiveWorkspace.Window is null) | ||||||
{ | ||||||
Debug.Fail("We are trying to navigate with a workspace that doesn't have a window!"); | ||||||
return false; | ||||||
return null; | ||||||
} | ||||||
|
||||||
var textView = interactiveWorkspace.Window.TextView; | ||||||
|
@@ -58,35 +58,40 @@ public async Task<bool> TryNavigateToSpanAsync(Workspace workspace, DocumentId d | |||||
var textSnapshot = document?.GetTextSynchronously(cancellationToken).FindCorrespondingEditorTextSnapshot(); | ||||||
CyrusNajmabadi marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
if (textSnapshot == null) | ||||||
{ | ||||||
return false; | ||||||
return null; | ||||||
} | ||||||
|
||||||
var snapshotSpan = new SnapshotSpan(textSnapshot, textSpan.Start, textSpan.Length); | ||||||
var virtualSnapshotSpan = new VirtualSnapshotSpan(snapshotSpan); | ||||||
|
||||||
if (!textView.TryGetSurfaceBufferSpan(virtualSnapshotSpan, out var surfaceBufferSpan)) | ||||||
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. Does this bit need to move into the deferred work? Since I think if there was async stuff streaming to the interactive window, the buffers could be updated and this would be stale? 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. in this case, we're technically on the UI thread the entire time. It's teh first line of GetLocationForSpanAsync. So it's less clear how much value any split has here. The split i was at least going for was having the first half be the part that detemines "is this safe to nav to?" and the second half is the actual "ok, now actually go there". |
||||||
{ | ||||||
return false; | ||||||
return null; | ||||||
} | ||||||
|
||||||
textView.Selection.Select(surfaceBufferSpan.Start, surfaceBufferSpan.End); | ||||||
textView.ViewScroller.EnsureSpanVisible(surfaceBufferSpan.SnapshotSpan, EnsureSpanVisibleOptions.AlwaysCenter); | ||||||
return new NavigableLocation(async cancellationToken => | ||||||
{ | ||||||
await _threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); | ||||||
|
||||||
textView.Selection.Select(surfaceBufferSpan.Start, surfaceBufferSpan.End); | ||||||
textView.ViewScroller.EnsureSpanVisible(surfaceBufferSpan.SnapshotSpan, EnsureSpanVisibleOptions.AlwaysCenter); | ||||||
|
||||||
// Moving the caret must be the last operation involving surfaceBufferSpan because | ||||||
// it might update the version number of textView.TextSnapshot (VB does line commit | ||||||
// when the caret leaves a line which might cause pretty listing), which must be | ||||||
// equal to surfaceBufferSpan.SnapshotSpan.Snapshot's version number. | ||||||
textView.Caret.MoveTo(surfaceBufferSpan.Start); | ||||||
// Moving the caret must be the last operation involving surfaceBufferSpan because | ||||||
// it might update the version number of textView.TextSnapshot (VB does line commit | ||||||
// when the caret leaves a line which might cause pretty listing), which must be | ||||||
// equal to surfaceBufferSpan.SnapshotSpan.Snapshot's version number. | ||||||
textView.Caret.MoveTo(surfaceBufferSpan.Start); | ||||||
|
||||||
textView.VisualElement.Focus(); | ||||||
textView.VisualElement.Focus(); | ||||||
|
||||||
return true; | ||||||
return true; | ||||||
}); | ||||||
} | ||||||
|
||||||
public Task<bool> TryNavigateToLineAndOffsetAsync(Workspace workspace, DocumentId documentId, int lineNumber, int offset, NavigationOptions options, CancellationToken cancellationToken) | ||||||
public Task<INavigableLocation?> GetLocationForLineAndOffsetAsync(Workspace workspace, DocumentId documentId, int lineNumber, int offset, NavigationOptions options, CancellationToken cancellationToken) | ||||||
=> throw new NotSupportedException(); | ||||||
|
||||||
public Task<bool> TryNavigateToPositionAsync(Workspace workspace, DocumentId documentId, int position, int virtualSpace, NavigationOptions options, CancellationToken cancellationToken) | ||||||
public Task<INavigableLocation?> GetLocationForPositionAsync(Workspace workspace, DocumentId documentId, int position, int virtualSpace, NavigationOptions options, CancellationToken cancellationToken) | ||||||
=> throw new NotSupportedException(); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,13 +20,13 @@ public Task<bool> CanNavigateToLineAndOffsetAsync(Workspace workspace, DocumentI | |
public Task<bool> CanNavigateToPositionAsync(Workspace workspace, DocumentId documentId, int position, int virtualSpace, CancellationToken cancellationToken) | ||
=> SpecializedTasks.False; | ||
|
||
public Task<bool> TryNavigateToSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken) | ||
=> SpecializedTasks.False; | ||
public Task<INavigableLocation?> GetLocationForSpanAsync(Workspace workspace, DocumentId documentId, TextSpan textSpan, NavigationOptions options, bool allowInvalidSpan, CancellationToken cancellationToken) | ||
=> SpecializedTasks.Null<INavigableLocation>(); | ||
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. This actually seems a bit conceptually nicer in that "we can't navigate to something" is something you can find out without actually trying the operation. |
||
|
||
public Task<bool> TryNavigateToLineAndOffsetAsync(Workspace workspace, DocumentId documentId, int lineNumber, int offset, NavigationOptions options, CancellationToken cancellationToken) | ||
=> SpecializedTasks.False; | ||
public Task<INavigableLocation?> GetLocationForLineAndOffsetAsync(Workspace workspace, DocumentId documentId, int lineNumber, int offset, NavigationOptions options, CancellationToken cancellationToken) | ||
=> SpecializedTasks.Null<INavigableLocation>(); | ||
|
||
public Task<bool> TryNavigateToPositionAsync(Workspace workspace, DocumentId documentId, int position, int virtualSpace, NavigationOptions options, CancellationToken cancellationToken) | ||
=> SpecializedTasks.False; | ||
public Task<INavigableLocation?> GetLocationForPositionAsync(Workspace workspace, DocumentId documentId, int position, int virtualSpace, NavigationOptions options, CancellationToken cancellationToken) | ||
=> SpecializedTasks.Null<INavigableLocation>(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using System.Threading; | ||
using System.Threading.Tasks; | ||
using Roslyn.Utilities; | ||
|
||
namespace Microsoft.CodeAnalysis.Navigation | ||
{ | ||
internal interface INavigableLocation | ||
{ | ||
/// <summary> | ||
/// Navigates to a location opening or presenting it in a UI if necessary. This work must happen quickly. Any | ||
/// expensive async work must be done by whatever component creates this value. This method is async only to | ||
/// allow final clients to call this from a non-UI thread while allowing the navigation to jump to the UI | ||
/// thread. | ||
/// </summary> | ||
Task<bool> NavigateToAsync(CancellationToken cancelletionToken); | ||
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. @jasonmalinowski i started with this being sync... but it still felt oogy. i'm not sure how i feel about any particular feature needing to have to have way to move itself to the UI thread before calling this. that said, i don't feel super strongly about this. i think there's merit in your point that thsi must be fast/sync/UI bound. 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. note: for this PR/set-of-refactorings, i'd like to have this async for the moment. then, once all the work is done, we should have a good idea if it can be safely made sync. 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. I'm OK with this, but let's keep an eye out for places the "this should be async only..." rule gets violated. And maybe we will still have cases where this needs to be async; I could imagine in VS even opening a file this may still have some async bits since we at least have to check if the file exists, or something.
CyrusNajmabadi marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
internal class NavigableLocation : INavigableLocation | ||
{ | ||
private readonly Func<CancellationToken, Task<bool>> _callback; | ||
|
||
public NavigableLocation(Func<CancellationToken, Task<bool>> callback) | ||
=> _callback = callback; | ||
|
||
public Task<bool> NavigateToAsync(CancellationToken cancellationToken) | ||
=> _callback(cancellationToken); | ||
|
||
public static class TestAccessor | ||
{ | ||
#pragma warning disable VSTHRD200 // Use "Async" suffix for async methods | ||
public static Task<INavigableLocation?> Create(bool value) | ||
#pragma warning restore VSTHRD200 // Use "Async" suffix for async methods | ||
{ | ||
return Task.FromResult<INavigableLocation?>( | ||
new NavigableLocation(c => value ? SpecializedTasks.True : SpecializedTasks.False)); | ||
} | ||
} | ||
} | ||
} |
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.
View with whitespace off.