Project-system: Visually de-prioritize references marked with `<Facade>true</Facade>` metadata in solution explorer

[ericstj]

See https://github.com/dotnet/project-system/issues/2132#issuecomment-304411619

/cc @srivatsn @terrajobst

[tmeschter]

@srivatsn @ericstj There are a couple of options here:

1. References marked with <Facade>True</Facade> are simply never shown under the "Assemblies" node. They would always be invisible to the user.
2. References marked as such would only display if you selected "Show All Items" in the Solution Explorer tool bar.

Any preference on that choice? Note that this is entirely separate from the changes we need to make to get them to show up under the SDK node.

[terrajobst]

Not showing them unless "Show All Items" is active would be awesome.

[davkean]

I would make this a value of the existing <Visible /> metadata and respect that. For example, <Visible>OnlyInShowAllFiles</Visible> (similar to how <CopyToOutputDirectory/> can be false or Never), rather than be tied to the specific "facade" concept - it's a concept we shouldn't be aware of.

[srivatsn]

I like Façade better - it's just a declarative piece of data at that point rather than trying to control UI behavior. Also in this case, adding Visible to the Reference item won't work anyway because we are saying that to filter out these Reference items from the Assemblies node we want to add Visible=false to them. We can't have it be both.

[davkean]

I'm suggesting that we add an additional concept to Visible that lets you say only show in ShowAllFiles, similar to what we already did with CopyToOutputDirectory which can be true, false, CopyAlways, PreserveNewest, Never. We would respect this for all items, not just references.

I don't like Facade because it's ties the project system to a extremely specific managed concept, today it's only facades - but what we want to apply this metadata to other things tomorrow?

[srivatsn]

So <Reference Include="System.Runtime.dll" /> in Netstandard.Library - what should it's Visible value be? We are saying in #2442 that it should be <Reference Include="System.Runtime.dll" Visible="False"/> so that the reference gets hidden in the Assemblies node but shows up under the SDK node. But when it shows up under the SDK node, we want it to have the value Visible="OnlyInShowAllFiles" - how would that work?

[davkean]

The <Reference Include="System.Runtime"/> doesn't end up under the SDK node by itself - something is putting it there, that is the thing that would mark it as such.

[srivatsn]

Not following you. So you are saying the package would just say <Reference Include="System.Runtime" Visible="False" NuGetPackageId="blah" /> and the task in the SDK when it reparents under the SDK node would change the visibility to OnlyInShowAllFiles? But how would it differentiate between System.Runtime and netstandard.dll - i.e how would the task in the SDK know that it should put Visible=OnlyInShowAllFiles for System.Runtime but Visible=true for netstandard.dll?

[davkean]

That would be the SDK itself to determine - not the project system. It could itself use Facade metadata to drive the visible metadata, the project system just wouldn't drive any behavior off that.

Remember: The item that shows under Assemblies and the item that shows up under SDKs are _two different_ items - it seems like you're thinking they are the same item. They may be sourced from the same item; <Reference Include="..."/> but ultimately they are different items.

[srivatsn]

Ok we are saying similar things then - I thought you were arguing for replacing the Façade metadata with Visible in the package. But you are just saying that from the project system's point of view, it doesn't understand the Façade metadata but it's the SDK that understands that. I'm ok with that.

[ericstj]

The <Reference Include="System.Runtime"/> doesn't end up under the SDK node by itself - something is putting it there, that is the thing that would mark it as such.

Today it is specifically not showing up there. That is what issue https://github.com/dotnet/project-system/issues/2132 is about, and what @srivatsn committed to fixing in the project system.

I'm ok if we decide to change where that fix goes, but I do want to make sure we have a complete fix and it doesn't sound to me like we have one at the moment.

[srivatsn]

Let me try to summarize the current position:

• The package continues to use the Façade metadata to differentiate between façade and regular reference.
• The task in the SDK repo which runs during a designtime build and reparents the reference under the SDK node adds the Visible=OnlyInShowAllFiles metadata to the item that's under the SDK node. (we will make that change).
• The project system then simply respects the Visible metadata.

Separately, to hide the references from the assembly node itself, the package should add Visible=False on all references and the project system should respect that as well.

@davkean @ericstj @nguerrera @tmeschter - does this sound fine?

[srivatsn]

Today it is specifically not showing up there.

Also @ericstj in case you missed it, a PR was merged yesterday (https://github.com/dotnet/sdk/pull/1330) that would make these references show up under the SDK node now but it's not hidden yet from the assemblies node and that's why #2132 is still open.

[tmeschter]

@srivatsn @davkean @ericstj @nguerrera I agree with using <Visible /> metadata to determine whether or not to show a reference under the Assemblies node, and will change this PR to implement that functionality.

However, I don't see the value in hiding assemblies underneath the NuGet and SDK nodes. If the user doesn't want to see the assemblies added by the NETStandard.Library package (or any other package) they can simply not expand that node. If they do want to see the assemblies added by the package, making them click the "Show All Files" button and then expand the package node is a pointless extra step (and not at all obvious).

[nguerrera]

Façade metadata but it's the SDK that understands that. I'm ok with that.

I'm not. The whole reason for using Facade instead of visible was because there was some debate about whether to show facades or not and we decided to provide the information that it is a facade and let the IDE decide how to visualize that (maybe grey it out, maybe hide it, maybe have an option of whether facades are show etc.) This is not an SDK-level concern, it is an IDE rendering concern.

[nguerrera]

You can implement Facade --> Visible only in show all files in the design-time-only targets (I mean the ones in project system repo) if you want to handle it at the msbuild layer, but I don't think there should be code in the SDK that does it.

[ericstj]

If the IDE wants to provide a different hook, then that is something NETStandard.Library can plug into. I'm OK with it being Visible=OnlyInShowAllFiles rather than Facade=true.

My impression is that the nodes will already be "moved" under the SDK node so there is no need for us to specify Visible on the netstandard.dll assembly, correct?

I agree with @nguerrera that involving the SDK here is unnecessary complexity. I'd also prefer to only have a single piece of metadata so that things don't keep growing and bloating these objects/log files.

[tmeschter]

My impression is that the nodes will already be "moved" under the SDK node so there is no need for us to specify Visible on the netstandard.dll assembly, correct?

Right. The change that went into the dotnet/SDK release/2.0.0 branch yesterday causes the assemblies to show up under the NETStandard.Library node under SDK. I don't see the need to support twiddling the visibility of those nodes.

[ericstj]

Ok, so then current plan will be:

• The package switches from Facade=true metadata to Visible=OnlyInShowAllFiles to differentiate between façade and regular reference.
• The project system then simply respects the Visible metadata.

[terrajobst]

Works for me. So long we get rid of the noise, I'm all good :-)

[nguerrera]

OK by me.

My concern was that if we kept Facade metadata under the reasoning that something else than the package decides how to render Facades, then that something else should be tied to the IDE, not the SDK.

I think replacing Facade=true with Visible=OnlyInShowAllFiles in the package is better for everyone. Much simpler.

[ericstj]

@srivatsn please confirm and indicate precisely the metadata that needs to be set here. Is it Visible or Visibility? Can we please make sure that we are closed on the naming and behavior before moving forward with changes?

[nguerrera]

There is a lot of confusion across several github issues here. I suggest we get in to a room and spec everything end-to-end from package to sdk to project system.

[srivatsn]

This won't work. Let me summarize how the nodes get populated today:

• A Reference item is injected to the project from the targets. The Assemblies node in VS simply looks at these reference items and nothing else. So if Visible=OnlyInShowAllFiles was set on the Reference items, then these facades will show up under the Assemblies node if show all files is on.
• A task that lives in the SDK today runs during a design-time build to create a set of items that populate the tree that goes under the SDK node. This task doesn't belong in the SDK. It is a design-time-only, VS-only thing that's in the SDK today for historical reasons (this task depends on items coming from the task that raises the lock file into items and we didn't get the time to split this out cleanly in v1). I'm using https://github.com/dotnet/project-system/issues/1984 to track moving this out of the SDK.
• The change that Tom made yesterday was to modify this task to create a new item for every Reference injected by the package. It's this new item that will show the dlls under the SDK node and it's this one that needs the Visible=OnlyInShowAllFiles metadata. So setting it on the Reference doesn't help at all.
• Since this second item is synthesized from a Reference item, something needs to tell the task that it's façade vs not and it cannot be Visible=OnlyInShowAllFiles because then that'll control the Reference's behavior under the Assemblies node.

I fully agree with Nick that this is a UI concern and shouldn't be in the SDK but that applies to reparenting the nodes, showing some nuget packages as an SDK etc. etc. The SDK has some UI concerns today that should be removed but until then I'd rather keep all the UI concerns localized and in one place. Hence my suggestion that this task (which again, only runs during a design-time build in VS) take the Façade metadata from a Reference item and map that to a Visible=OnlyInShowAllFiles

I am happy to get on a call to discuss this (looks like many ppl are wfh today and are not online right now).

[ericstj]

I think I'm seeing the full picture now.

The Assemblies node in VS simply looks at these reference items and nothing else. So if Visible=OnlyInShowAllFiles was set on the Reference items, then these facades will show up under the Assemblies node if show all files is on.

Ideally this would be fixed by the same thing that was moving them to be parented by a package/sdk. Now I see why #2442 was trying to do this, but it doesn't have a good indication of which items to hide. For some reason this is different than hiding the items that come from packages: I'd like to understand why? As a result you'd like us to make all the references we add Visible=False as the good indication of what should be hidden from the Assemblies node.

To get the behavior we want under the package/SDK node you still need a different bit of Visible state on the facades. I had originally assumed you'd just copy metadata, but now I see that this isn't happening and indeed it needs to be different state given the current behavior.

Now ideally I'd want the following:

1. Prescribe a way for us to provide references via targets and have them behave the same way as those from NuGet. It shouldn't involve setting a bunch of extra metadata and hardcoding package Ids and versions in SDK tasks or the project system. Consider that you could do that by changing how the references are provided.
2. Prescribe a way for any reference to define its visibility.

Looking forward to the call.