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.

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

Bring SignatureHelp support from EditorFeatures.Wpf to EditorFeatures.Cocoa #50389

Closed
wants to merge 47 commits into from

Conversation

davidwengier
Copy link
Member

#50345 moved some files to EditorFeatures.Wpf that are needed by VS for Mac, so this adds them to EditorFeatures.Cocoa so VS Mac can consume them.

An ideal solution here would presumably be an EditorFeatures.Common that can reference Microsoft.VisualStudio.Language.Intellisense, but not WPF, and then these could be shared. I'm not doing that here simply because I have no idea what infrastructure pieces this would require, and its 4pm my time :) Happy to do it if someone wants to let me know whats involved.

sharwell and others added 22 commits December 15, 2020 20:40
These files are based on dotnet/runtime@cf258a14 (v5.0.0-rtm.20519.4).
The APIs providing the scenario are not public, and not easily ported.
This code normally runs in a 64-bit process, so favor the faster
implementation for that case.
@davidwengier davidwengier requested review from a team as code owners January 12, 2021 05:17
…istration

Use IEventListener to register workspaces for LSP instead of inside AbstractPackage
Copy link
Member

@KirillOsenkov KirillOsenkov left a comment

Choose a reason for hiding this comment

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

👍

@davidwengier
Copy link
Member Author

No changes at all. Linked files is a good idea, I didn't think of, and makes me realise that an EditorFeatures.Common could potentially just be a shared project, if it's easier than shipping something new.

Copy link
Member

@genlu genlu left a comment

Choose a reason for hiding this comment

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

Thanks @davidwengier! Approved assuming linked files will be used.

@davidwengier
Copy link
Member Author

This is now using linked files. Verified from a local build in ILSpy:
image

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

sharwell and others added 3 commits January 12, 2021 16:17
Implement SegmentedDictionary<TKey, TValue>
Bring SignatureHelp support from EditorFeatures.Wpf to EditorFeatures.Cocoa
@ghost
Copy link

ghost commented Jan 13, 2021

Apologies, I am afraid I am encountering technical difficulties that might have hampered my ability to assist with merging this pull request. I will continue to try to assist if there are further changes to this pull request.

@davidwengier davidwengier requested review from a team as code owners January 13, 2021 01:06
@davidwengier davidwengier deleted the SigHelpToCocoa branch January 13, 2021 01:06
@jasonmalinowski
Copy link
Member

@davidwengier Did you mean to close?

@davidwengier
Copy link
Member Author

Yeah, I have no idea what happened, I didn't close it manually, but at the end of the day the changes are in master so we're good.

When I looked at it, the checks were green but GitHub said there were conflicts (without enumerating what they were) and msftbot said it encountered difficulties. So I merged master in locally, which had no conflicts, shrugged, and pushed. After I pushed I saw it close, and all of Sam's commits from other branches came in. I'm guessing msftbot merged the PR, but then somehow GitHub saw my push included only commits already in master, so closed the PR automatically.

@CyrusNajmabadi
Copy link
Member

Err.. can we def have a meeting on this . I didn't even know this lib existed. Def want to have a clean story here for work, esp. around ensuring we aren't unnecessary duplicaitng htings or having to fix things in multiple locations.

@CyrusNajmabadi
Copy link
Member

Yeah, I have no idea what happened, I didn't close it manually, but at the end of the day the changes are in master so we're good.

can you clarify which chnages went into master?

@davidwengier
Copy link
Member Author

can you clarify which chnages went into master?

Before GitHub did whatever it did, this PR had these three commits in it, which are now in master:
image
(plus one more, which is the "copy" that the first one reverts, but it was too old to put into one screenshot 😛)

I didn't even know this lib existed. Def want to have a clean story here for work, esp. around ensuring we aren't unnecessary duplicaitng htings or having to fix things in multiple locations.

The library has existed for as long time AFAIK, but was in the VS Editor repo until recently. It got moved to Roslyn in #48866 to make changes easier, and to avoid having to duplicate (which stupidly, I forgot about and initially did, before Jason rightly pointed out that linked files were better).

Having it Roslyn does solve a dependency mess that I used to have to deal with weekly for VS Mac insertions, but unfortunately it doesn't help us avoid breaking VS for Mac itself, which is what this PR is in response to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants