Quickinfo Tooltip and GotoDefinition Navigation Improvements #2683
Conversation
tooltip from .fsi links to .fsi tooltip from .fs links to .fs quick navigation if no redirect is necessary
| match candidates.Length with | ||
| | 0 -> None | ||
| | 1 -> Some (self.GetDocument candidates.[0]) | ||
| | _ -> |
There was a problem hiding this comment.
what about
match candidates with
| [||] -> None
| [|_|] -> ...
| _ -> ...?
There was a problem hiding this comment.
| let r = candidates |> Seq.tryFind (fun docId -> | ||
| (self.GetDependentProjects docId.ProjectId | ||
| |> Seq.tryFind (fun proj -> proj.Id = projectId)) | ||
| |> Option.isSome) |
| Path.GetExtension filePath = ".fsi" | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Why so many empty lines?
|
|
||
| static member RunTaskSynchronously task = | ||
| task |> | ||
| Async.AwaitTask |> Async.RunSynchronously |
There was a problem hiding this comment.
Weird formatting.
| /// Parse and check the provided document and try to find the defition of the symbol at the position | ||
| let checkAndFindDefinition | ||
| (checker: FSharpChecker, documentKey: DocumentId, sourceText: SourceText, filePath: string, position: int, | ||
| defines: string list, options: FSharpProjectOptions, textVersionHash: int) : Option<range> = maybe { |
There was a problem hiding this comment.
I understand that we pass so many stuff here in order to make it tastable, because we cannot use Document here. But this bunch of parameters are passed to many functions in FSharp.Editor project. What about a new data type, like this:
type EditorDocument =
{ FilePath: string
Key: DocumentId
Text: SourceText
TextVersionHash: int
Defines: string[] }?
There was a problem hiding this comment.
(maybe add FSharpOptions into it as well)
| let fileName = try System.IO.Path.GetFullPath range.FileName with _ -> range.FileName | ||
| let refDocumentIds = document.Project.Solution.GetDocumentIdsWithFilePath fileName | ||
| if not refDocumentIds.IsEmpty then | ||
| let refDocumentId = refDocumentIds.First() |
There was a problem hiding this comment.
|> Seq.toList ... match ... with | refDocumentId :: _ -> ... | [] -> return None ?
| | false, _ , Some range, _ -> return Some range | ||
| | false, _ , None , Some range -> return Some range | ||
| | false, Some range, None , None -> return Some range | ||
| | _ , None , None , None -> |
There was a problem hiding this comment.
This big decision table is hard to understand.
|
|
||
| /// find the definition location (implementation file/.fs) of the target symbol | ||
| let findDefinitionOfSymbolAtRange | ||
| (document:Document, range:range, sourceText:SourceText, checker: FSharpChecker, projectInfoManager: ProjectInfoManager) = |
There was a problem hiding this comment.
I'm not sure type annotations here are worth the noise.
|
|
||
|
|
||
|
|
||
| [<Shared>] |
There was a problem hiding this comment.
Shared is by default, you can remove it.
| try let navList = List<INavigableItem>() | ||
| let! navItem = navFunction args | ||
| navList.Add navItem | ||
| return navList.AsEnumerable() |
There was a problem hiding this comment.
Returning seq from an async computation looks a bit unsafe. In this case it's ok, but I'd prefer an array or a list.
| navigationTask.RunSynchronously () | ||
|
|
||
| if navigationTask.Status = TaskStatus.RanToCompletion && navigationTask.Result.Any() then | ||
| let navigableItem = navigationTask.Result.First() // F# API provides only one INavigableItem |
There was a problem hiding this comment.
Seq.toList ... match?
There was a problem hiding this comment.
I just feel uncomfortable seeing partial functions usage.
| ClassificationTags.GetClassificationTypeName | ||
| let targetPath = range.FileName | ||
| logInfof "origin document - %s \n" initialDoc.FilePath | ||
| logInfof "target range to - %s \n %A" range.FileName range |
There was a problem hiding this comment.
Remove logging. It will appear in release configuration as well, which we certainly don't want.
| return! gotoDefinitionService.NavigateToSymbolDefinitionAsync (targetDoc, targetSource, range) | ||
| // adjust the target from implmentation to signature | ||
| | true, false -> | ||
| return! gotoDefinitionService.NavigateToSymbolDeclarationAsync (targetDoc, targetSource, range) |
There was a problem hiding this comment.
Maybe add a simple DU to make this true-false thing more readable, like:
type FileType = Signature | Implementation
match isSignatureFile initialDoc.FilePath, isSignatureFile targetPath with
| Signature, Signature
| Implementation, Implementation -> ...
| Implementation, Signature -> ...
| Signature, Implementation -> ...?
|
|
||
| let layoutTagToFormatting = | ||
| roslynTag | ||
| >> ClassificationTags.GetClassificationTypeName | ||
| >> typemap.GetClassificationType | ||
| >> formatMap.GetTextProperties |
There was a problem hiding this comment.
Maybe use piping instead of composition? It's quite hard to read.
There was a problem hiding this comment.
it'd basically look the same with pipes
| yield run | ||
| //| :? Layout.NavigableTaggedText as nav -> | ||
| // run.ToolTip <- nav.FullName | ||
| // run :> Documents.Inline |
There was a problem hiding this comment.
Ooops, that's mine. @cloudRoutine pulled in rather wip stuff.
| module internal FSharpQuickInfo = | ||
|
|
||
| [<Literal>] | ||
| let SessionCapturingProviderName = "Session Capturing Quick Info Source Provider" |
There was a problem hiding this comment.
Maybe let's leave the literal.
| let h = Documents.Hyperlink(Documents.Run(text), ToolTip = range.FileName) | ||
| h.Click.Add <| fun _ -> goTo range | ||
| for taggedText in content do | ||
| let run = Documents.Run(taggedText.Text, ToolTip = taggedText.Tag ) :> Documents.Inline |
There was a problem hiding this comment.
Tooltip here was or debug, it shows LayoutTag for element at the moment. We can remove it for now.
|
|
|
I'm going to throw a spanner in the works and say I prefer it underlined. I don't think the dotted is readable at all - maybe dashed? Again I think this should be an option. |
|
Also I'd like to say that I don't think we should be making any changes to the way the "Could not navigate to symbol under caret" dialog. I think the status bar changes should be put into a separate into for discussion. |
|
@majocha i tried what you suggested and I got this error it seems the problem was |
|
@majocha it appears that links to internally defined types don't appear in the signatures for externally defined types but they do appear for the methods |
|
what is the status of this PR? Thanks Kevin |
|
@KevinRansom It works great now, but as I understand there's a not settled discussion. |
|
Agreed with @saul and @vasily-kirichenko about the status bar change. I think that's more of a usability change than a bug fix or feature enhancement. |
|
The light and bold lines are not at the same position:
|
|
@cartermp it's also keeping consistency with the gotodefinition VFPT feature in vs2015 |
|
Fixed
|
|
I'd prefer to not show link for the symbol itself if we are at it's definition already
|
|
@vasily-kirichenko misalignment should be fixed, waiting to be merged. |
|
@cloudRoutine pull my branch again, I've fixed showing link for symbol itself. |
|
@KevinRansom this branch is good to merge now |
|
@clousdroutine Thanks for this @majocha, @cloudRoutine, @vasily-kirichenko |
|
@saul ... Raise an issue for the status line changes. |
…2683) * full type name in tooltip, provisional tab preferred * more entities made navigable * use IGoToDefinition service * this is used only here * MEF import FSharpGotoDefinitionService into QuickInfoProvider * speed up gotoDefinition * additional GotoDefn navigation strategies * quickinfo navigation stays in its lane tooltip from .fsi links to .fsi tooltip from .fs links to .fs quick navigation if no redirect is necessary * fix unittests * restore recursive matchingDoc * asynchronous navigation from tooltips * fix cross project .fs -> .fs and .fsi -> .fsi Navigation * cleanup and extra documentation fixed bug in cross project .fs -> .fs navigation * missed this one * gotodefinition sig <-> impl at declaration location * fix async workflow * animate status bar search and timeout on msgs * Better links styling * integrate sig doccoms * fix error introduced by prior merge * fixed invalid type access in `getUnusedOpens` * fix invalid span bug in `symbolIsFullyQualified` * check if normalized doccom text matches * cleanup status bar usage * fix underline pen position, code cleanup and formatting * do not show links for symbol itself
addresses parts of #2673