Extend scriptability

[nickspoons]

This PR is a possible alternative to #676, and also rolls back #675

Instead of adding an "open all usages" function, this PR attempts to improve the composability of the OmniSharp-vim API, allowing simpler custom integrations.

This PR allows:

" Open all usages of the symbol under the cursor in tabs
:call OmniSharp#actions#usages#Find({locs -> OmniSharp#locations#Navigate(locs, 'tabedit')})

" Open the definition of the symbol under the cursor in a vertical split
:OmniSharpGotoDefinition sp

[amosonn]

Wow, this is extensive and looks cool! I haven't read everything carefully, but this looks generally good, I will try to read more and comment. One thing that is worth noting, which I ran into when writing my private use-case on #676: The paths the are coming from OmniSharp are sometimes absolute and sometime relative. This makes the correctness of the paths fragile, because they rely on having the path of the original file. In particular, interaction with autochdir were un-fun (this affects BTW also the Rename command, which doesn't properly open the original path). My workaround was to disable autochdir for the duration of the function (see https://github.com/amosonn/omnisharp-vim/blob/3dad5bce8a7f4cb03d514083b8dc10a0ea5302ee/autoload/OmniSharp/actions/usages.vim#L76-L80), but I guess normalizing the filepaths somewhere lower would be a better solution.

[nickspoons]

That's interesting that you mention normalising file paths, because, I currently have a branch with a modification to OmniSharp#util#TranslatePathForClient(), which attempts to make all paths relative, if possible: https://github.com/nickspoons/omnisharp-vim/commit/8db23e309ef74b8762609772739f87774fb42ed9

I had been trying it out as a possibility, after someone else was trying to work out how to get this functionality, but I had pretty much come to the conclusion that it is the wrong approach.

However, normalising paths at this point to always make them absolute makes a lot of sense. Of course I could always add an option e.g. g:OmniSharp_filepaths_relative (default 0), and let people decide for themselves.

[nickspoons]

Have a look at the latest commit, @amosonn. Will this resolve any normalization issues for you, if you add the following to your .vimrc?

let g:OmniSharp_filename_modifiers = ':p'

[amosonn]

Yes, that fix works for me, thanks!

[nickspoons]

Hi @amosonn, thanks for https://github.com/nickspoons/omnisharp-vim/pull/2

I think this is a good approach but not quite right. If we don't translate paths in location#Parse(), then anyone writing their own Callback and any other usage of locations has to remember to translate them. And in WSL environments, failure to do this results in unusable paths, so the example from this issue PR will not work:

" Opens untranslated paths, i.e. C:\code\file.cs instead of /mnt/c/code/file.cs
:call OmniSharp#actions#usages#Find({locs -> OmniSharp#locations#Navigate(locs, 'tabedit')})

" Correct usage:
call OmniSharp#actions#usages#Find({locs -> OmniSharp#locations#Navigate(OmniSharp#util#TranslateLocationsForClient(locs), 'tabedit')})

Instead, I think we should always perform the path translation in locations#Parse(), but have the translation always be absolute. This means we will always be working with good paths from this point on. Making the paths relative can then happen separately, as you have suggested previously.

I am therefore including your first commit from the PR, but not the second (or at least, a modified version of the second), see what you think.

[amosonn]

A yes, this makes sense! I missed the fact that util#Translate does also other things, not related to making the path relative etc, which always need to happen. This split looks good.

I think the next step should still be what I said: pushing the Location#Modify calls into the various Navigate, QuickFix etc. methods. But this is more of an internal code organization thing, so doesn't block this, unless somebody is using those methods externally?

[nickspoons]

What would be the benefit of moving the OmniSharp#locations#Modify() call into the higher level functions?

We'd need to update each of these:

• OmniSharp#locations#Navigate
• OmniSharp#locations#Preview
• OmniSharp#locations#SetQuickfix
• clap#OmniSharp#FindSymbols
• clap#OmniSharp#FindUsages
• ctrlp#OmniSharp#findsymbols#setsymbols
• fzf#OmniSharp#FindSymbols
• fzf#OmniSharp#FindUsages

[nickspoons]

I suppose the answer is that it keeps us using absolute paths as long as possible, and means that the various high level methods are able to retain final control over what they do with those paths.

And in fact now I think about it, OmniSharp#locations#Navigate and OmniSharp#locations#Preview do not need to be updated - they will work best with absolute paths.

[nickspoons]

Actually, OmniSharp#locations#SetQuickfix is not always used with locations which need to be modified - it is also populated with test run locations and code-structure locations (:OmniSharpFindMembers). So I have cleaned up the OmniSharp#locations#Navigate and OmniSharp#locations#Preview usages and left it at that.

I'm pretty happy with this now. Anything else to add, @amosonn?

[amosonn]

This looks good, thanks a lot for the effort!

I think it would make sense to switch the other actions to the same behaviour as Find: If a callback is present, call it with the found result(s) instead of opening them. This enables extending scriptability for them as well. This also seems consistent with passing in the requested edit command: GotoDefinition tabedit passes the found location to tabedit (almost, since it also navigates), GotoDefinition MyFunc passes it to MyFunc.

What do you say? I could also draft this.

[Melandel]

Since this PR improves composability, would it be the right place to add a follow-up action (Funcref or command) for things like:

• NavigateUp
• NavigateDown
• FixUsings
• FormatCode
• any CodeAction ?

Actually, doesn't it make sense that every asynchronous command should have the right to be customized with a follow-up action (Funcref or command)?

In a synchronous plugin, it is done natively, but with asynchronous commands, it has to be done with Funcrefs/Callbacks.

[nickspoons]

@Melandel yes this is a good place for that, and I think it would be a good idea to unify the way all of these functions are called, giving the option of passing in a Callback function to all of them in the same way.

Some situations have to be handled differently, such as CodeActions, where there are 2 different async events: first a request to /getcodeactions and then later a request to /runcodeaction. In this case we may need to allow passing in 2 optional Callbacks, to be run in each case?

edit Actually I'm not quite sure that's the way to go. There should be an option to pass in an optional callback to OmniSharp#actions#codeactions#Get, and if it exists, the available code actions would be passed to the callback, and the callback would be responsible for either displaying the options or selecting one and running it or whatever. This would then be consistent with how the other functions in this PR have been modified, i.e. @amosonn's current PR-on-this-PR modifies OmniSharp#actions#implementations#Find to pass the actual implementations to the callback, rather than just passing the number of implementations.

[nickspoons]

Sorry for the slow progress on this PR. I think it looks really good but has become big and needs a good concentrated looking over before merging. I'm not sure right now if there are other places we should be adding callbacks, or standardising public APIs.

And I don't have time right now because my SSD has died so I have some systems and test environments to rebuild...

[nickspoons]

OK let's merge this, shall we? There are some things outstanding, including adding callbacks to the remaining functions Melandel mentioned. However, this PR has been open for a very long time now, I think it would be better to merge it and come back to those other things later.

I apologize one final time for the delays on my part.

[nickspoons]

Thanks very much for all your input @amosonn, these are some very nice improvements.