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 EventHookup to EditorFeatures layer #34190

Closed
KirillOsenkov opened this issue Mar 16, 2019 · 6 comments · Fixed by #49092
Closed

Move EventHookup to EditorFeatures layer #34190

KirillOsenkov opened this issue Mar 16, 2019 · 6 comments · Fixed by #49092
Labels
Area-IDE Concept-Continuous Improvement
Milestone
Backlog

Comments

@KirillOsenkov
Copy link
Member

Currently EventHookup code is in the VisualStudio layer. To be consumed by VS for Mac it would be nice to move it down to EditorFeatures. From cursory glance we haven't found dependencies that would be hard to isolate from VS.

Currently VSMac has a copy of the code.
@jasonmalinowski
Copy link
Member

History: this feature used to do evil things with quick info to display the tooltip, and by "evil" I think I mean "reflection against the VS shims". It looks like when we moved to the new quick info APIs in 15.8 I think the hackery went away.

@jinujoseph jinujoseph added this to the Backlog milestone Mar 20, 2019
@jasonmalinowski
Copy link
Member

@tmat: let me know when it's safe to start cleaning some things up out of the EditorFeatures.Cocoa stuff; I know you still have PRs in flight but there's some pretty obvious cleanups that are available once you're done.

@tmat
Copy link
Member

tmat commented Oct 29, 2020

I think it's already safe. The only problem is with testing though. It'd be good to have some test coverage before/as we start cleaning up. /cc @davidwengier

@davidwengier
Copy link
Contributor

davidwengier commented Oct 29, 2020 •
edited
Loading

I think we should do one "clean" insertion first, where nothing has changed but the source of the DLL, but no reason PRs can't be opened.

Definitely testing is an issue. I started cleaning up the snippets code by removing unused parameters, and had pretty quickly removed enough code that makes me wonder if its ever actually used.

@KirillOsenkov
Copy link
Member Author

FYI @DavidKarlas who wrote the Cocoa layer

@jasonmalinowski
Copy link
Member

Agreeing with @davidwengier that let's get a clean insertion first, but duh of course I can do PRs now. 😄

@jasonmalinowski jasonmalinowski mentioned this issue Oct 31, 2020
Merged
jasonmalinowski added a commit to jasonmalinowski/roslyn that referenced this issue Nov 5, 2020
@jasonmalinowski
Remove duplicated implementations of C# event hookup
bdf8701 
This feature originally existed in the C# editor layer, but then moved
to the VS layer once we split out EditorFeatures.Wpf; we didn't have a
CSharpEditorFeatures.Wpf so we had no place to put this since it had
dependencies on WPF at the time. It no longer has a direct WPF
dependency, so we can move this back to EditorFeatures and simply delete
the Cocoa copy.

The copy in EditorFeatures.Cocoa was verified to have no improvements
against the current code; there were some differences but those were
places where we made later fixes that hadn't been reflected into
EditorFeatures.Cocoa.

Fixes dotnet#34190
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Area-IDE Concept-Continuous Improvement
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Remove duplicated implementations of C# event hookup jasonmalinowski/roslyn
6 participants
@tmat @jasonmalinowski @KirillOsenkov @davidwengier @gafter @jinujoseph