Skip to content

Conversation

@CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Apr 24, 2025

This moves everything down, except for special code we expose to Xaml to allow them to use our Peek service. This PR also adds code into their existing EA layer to do this instead.

So we can:

  1. merge this in.
  2. insert it
  3. get xaml to use the new ea helper in their VS.Xaml project
  4. remove the wpf layer.
  5. insert that.

Precursor to #77707

Build: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=11516158&view=results
PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/633485

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner April 24, 2025 22:29
@ghost ghost added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 24, 2025

namespace Microsoft.VisualStudio.LanguageServices.Xaml.Features.Peek;

internal interface IXamlPeekableItemFactory
Copy link
Member Author

Choose a reason for hiding this comment

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

new EA layer code for xaml to use.

@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski ptal.

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner April 25, 2025 19:16
@CyrusNajmabadi
Copy link
Member Author

@jasonmalinowski @JoeRobich @dibarbet ptal.

Copy link
Member

@dibarbet dibarbet left a comment

Choose a reason for hiding this comment

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

Fine with the change overall, but holding off on approval until the PR validation comes back.

@CyrusNajmabadi
Copy link
Member Author

Fine with the change overall, but holding off on approval until the PR validation comes back.

SUre. Teh build has been kicked off. will report back on results.

Copy link
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

Approved, but there's still one line that's concerning about CodeLens, but I presume if we break that internal tests will make it very clear to us.

<RootNamespace>Microsoft.CodeAnalysis.ExternalAccess.Debugger</RootNamespace>
<TargetFramework>netstandard2.0</TargetFramework>
<TargetFramework>net472</TargetFramework>
<UseWPF>true</UseWPF>
Copy link
Member

Choose a reason for hiding this comment

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

I don't imagine this change was necessary? This reminds me of the other CodeLens change you weren't sure about.

Copy link
Member Author

Choose a reason for hiding this comment

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

No. this debugger lib def talks to EditorFEatures. it is @tmat 's EA layer so that he can populate the FAR window with symbolic results. He'd have to say why this is needed. BUt it will def have to change like this.

<ProjectReference Include="$(MSBuildThisFileDirectory)..\ServiceHub\Microsoft.CodeAnalysis.Remote.ServiceHub.csproj" />
<ProjectReference Include="$(MSBuildThisFileDirectory)..\..\..\Features\CSharp\Portable\Microsoft.CodeAnalysis.CSharp.Features.csproj" />
<ProjectReference Include="$(MSBuildThisFileDirectory)..\..\..\Features\VisualBasic\Portable\Microsoft.CodeAnalysis.VisualBasic.Features.vbproj" />
<!-- CodeLens provider is not part of Roslyn ServiceHub, we just use the existing ServiceHub package as a delivery vehicle -->
Copy link
Member

Choose a reason for hiding this comment

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

So was this comment wrong that we weren't using it as a delivery vehicle? Or does this need to be undone with the other CodeLens changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah. this needs to be undone. it broke VS insertion. But i don't understand why. It seems bad that we depend on this line here for things to work. i would love your help on understanding that @JoeRobich @jasonmalinowski @ToddGrun .

@CyrusNajmabadi
Copy link
Member Author

Test insertion is clean. Merging this in.

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

Labels

Area-IDE Needs UX Triage untriaged Issues and PRs which have not yet been triaged by a lead

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants