Provide VsLangProj.ProjectProperties through VSProject export by basoundr · Pull Request #1842 · dotnet/project-system

basoundr · 2017-03-24T06:32:16Z

This change exports VsLangProjectProperties as VsProject. This VsLangProjectProperties, in addition to VSProject, implements VsLangProj.ProjectProperties as well. This object will now start providing the values in the Project Property pages.

Currently I have implemented just the properties, expected from the CommonProperty, that are used in the Property Pages. We can implement the rest of them as and when required. In the future, this object might also have to implement CSharpProjectProperties7 and VBProjectProperties7, again, since we dont need them now, this change will not handle those cases.

Tagging @dotnet/project-system @davkean @srivatsn for review

basoundr · 2017-03-24T06:52:01Z

src/Microsoft.VisualStudio.Editors/PropPages/AdvBuildSettingsPropPage.vb

+                Dim throwAwayObject As Decimal = Nothing
+                If (TypeOf (value) Is UInteger OrElse
+                    (CpsPropertyDescriptorWrapper.IsAnyCpsComponent(m_Objects) AndAlso Decimal.TryParse(DirectCast(value, String), throwAwayObject))) Then
                    iBaseAddress = CUInt(value)


@jviau BaseAddress property is currently backed by a StringProperty in the XAML. I still get a string value, if I change the Property type to IntProperty from the StringProperty. For eg: "123456"

As it turns out, to obtain the value, DynamicType's GetValue is called, which always return a string. Is there a way it could return the actual type that the property is initially backed with?

I don't think there's a way today to get anything other than a string. There are other places in the prop pages where such code exists. However, for compat, it'll be good if the object returned here was the right data type instead of string.

@srivatsn is correct. Right now dynamic type system is always strings, because it is for UI and also msbuild properties are always strings. However, the various implementations of object IProperty.GetValueAsync() does try to return the value as the correct type. In this piece of code we are commenting on, what is the CPS object you are working on? Is it already the value from the DynamicType or something else?

It is the value from DynamicType since we get that from DynamicTypeBrowseObject

srivatsn · 2017-03-24T13:13:58Z

...VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties.cs

+        public object __project => throw new NotImplementedException();
+
+        public string StartupObject { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
+        public prjOutputType OutputType


Can you group the implemented ones together? Also there are some weird new lines - either have newlines between props or dont

sure. With CodeLens adornment, it is tough to see them.

tmeschter · 2017-03-24T16:06:04Z

...VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties.cs

+            _projectProperties = projectProperties;
+        }
+
+


Nit: extra whitespace.

tmeschter · 2017-03-24T16:10:22Z

...VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties.cs

+
+namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties
+{
+    internal class VsLangProjectProperties : VSProject, VSLangProj.ProjectProperties


Perhaps make this a partial class, and put the implementation of each interface in a different file? There's a lot of stuff here.

Partial class, by roslyn convention, would mean a nested type. So let me try to group the implementation separately without partial class. That way, it does not seem unorganized.

No, we have non-nested partial classes.

Lots of them, actually: AbstractProject, AbstractLegacyProject, CSharpProjectShim, etc.

RaulPerez1 · 2017-03-24T21:14:15Z

...VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties.cs

+            get
+            {
+                var configurationGeneralBrowseObjectProperties = JoinableTaskFactory.Run(ProjectProperties.GetConfigurationGeneralBrowseObjectPropertiesAsync);
+                var value = (IEnumValue)JoinableTaskFactory.Run(configurationGeneralBrowseObjectProperties.OutputTypeEx.GetValueAsync);


OutputTypeEx [](start = 107, length = 12)

You shouldn't need this, in fact that should be completely killed from the rules once you have the project properties completed. There should only be an OutputType (string) on the project and depending on the property that's getting asked it should convert it back to either projOutputType or prjOutputTypeEx

OutputType does not support AppContainerExe or WinMDObj. We could use this property to do the conversion but it looks unnecessary given that we can backup that property directly. Also, I dont see why we have to remove OutputTypeEx completely given that it is a VsLangProj property.

@srivatsn may have more context here but my understanding is that it was added to the rules page as a workaround to try and provide this property. In the old project system it's the same property (which is usually how "Ex" type properties work). I believe the current provider (which you should be able to delete) is doing that conversion if OutputType is set to either of the UWP values (it converts them to either exe or library).

That sounds right but I don't remember the details. You'll have to look at history.

Looking at the history, it does seem to be the case. But we will have to look at what breaks if we remove this. I can create an issue to track this work.

Opened #1867

RaulPerez1 · 2017-03-24T21:15:58Z

...VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties.cs

+
+        public prjOriginatorKeyMode AssemblyOriginatorKeyMode { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
+
+        public bool DelaySign { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }


public bool DelaySign { get => throw new NotImplementedException(); set => throw new NotImplementedException(); } [](start = 8, length = 113)

Property pages use this so leaving it unimplemented will cause issues there (or false results).

DelaySign property is not from Common properties. It is backed up by DynamicTypeBrowseObject from here. This will not be a problem.

Please file bugs for these unimplemented properties.

Opened #1868

basoundr · 2017-03-25T03:22:33Z

...ojectSystem.Managed.VS.UnitTests/ProjectSystem/VS/Properties/VsLangProjectPropertiesTests.cs

+            }
+
+            [Fact]
+            public void VsLangProjectProperties_OutputType_Get()


I was not able to write the tests for set methods because of Moq. I tried the same test below and added the following code but it does not seem to work.

var mockObject = Mock.Get(projectProperties.GetConfigurationGeneralBrowseObjectPropertiesAsync().Result.OutputType); // Get the mock object of IProperty, which is [here](https://github.com/dotnet/roslyn-project-system/blob/master/src/Microsoft.VisualStudio.ProjectSystem.Managed.UnitTests/Mocks/ProjectPropertiesFactory.cs#L110) mockObject.Verify(m => m.SetValueAsync(It.IsAny<object>()));

Is this valid? Am I missing something here?

Use SetupProperty

I want to mock the call SetValueAsync of the OutputType property.

SetupProperty can be used to mock properties that are overrideable. In this case though, the property will be OutputType, from ConfigurationGeneralBrowseObject , which is not overrideable. So it cannot be mocked.

I tried something like this:

[Fact] public void VsLangProjectProperties_AssemblyName_Get() { var project = UnconfiguredProjectFactory.Create(); var data = new PropertyPageData() { Category = ConfigurationGeneral.SchemaName, PropertyName = ConfigurationGeneral.AssemblyNameProperty, Value = "Blah" }; var projectProperties = ProjectPropertiesFactory.Create(project, data); var activeConfiguredProject = ActiveConfiguredProjectFactory.ImplementValue(() => projectProperties); var vsLangProjectProperties = new VSProject(Mock.Of<VSLangProj.VSProject>(), IProjectThreadingServiceFactory.Create(), activeConfiguredProject); Assert.Equal(vsLangProjectProperties.AssemblyName, "Blah"); var mockObject = Mock.Get(projectProperties.GetConfigurationGeneralPropertiesAsync().Result.AssemblyName); mockObject.Setup(prop => prop.SetValueAsync(It.IsAny<string>())) .Verifiable(); vsLangProjectProperties.AssemblyName = "Testing"; mockObject.Verify(); }

and it results in

Moq.MockVerificationException : The following setups were not matched: IEvaluatedProperty prop => prop.SetValueAsync(It.IsAny<String>()) Stack Trace: at Moq.Mock.Verify()

basoundr · 2017-03-25T03:29:21Z

Tagging @dotnet/project-system again. Tests are in.

davkean · 2017-03-27T01:53:32Z

...VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties.cs

+
+        public prjOriginatorKeyMode AssemblyOriginatorKeyMode { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }
+
+        public bool DelaySign { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }


Please file bugs for these unimplemented properties.

davkean · 2017-03-27T02:01:09Z

...VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties.cs

+
+            set
+            {
+                var configurationGeneralBrowseObjectProperties = JoinableTaskFactory.Run(ProjectProperties.GetConfigurationGeneralBrowseObjectPropertiesAsync);


Use IProjectThreadingService.ExecuteSynchronously and wrap the whole method instead of running in multiple contexts.

davkean · 2017-03-27T02:03:01Z

...udio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectPropertiesProvider.cs

+        {
+            get
+            {
+                return new VsLangProjectProperties(_vsProject, _threadingService, _projectProperties);


This should be cached - there should be one of these.

davkean · 2017-03-27T02:03:19Z

...udio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectPropertiesProvider.cs

+
+        [Export(ExportContractNames.VsTypes.VSProject, typeof(VSProject))]
+        [AppliesTo(ProjectCapability.CSharpOrVisualBasic)]
+        [Order(10)]


This order looks arbitrary how did you decide it?

The CPS's implementation is exported with default value(0) . Having an order 10, will give the ability for extensions to export their implementation with an order between 0 &10 and the ability to export with an order greater than 10. We dont want to export our's with MaxValue becauseno one else can have their export ordered higher than ours.

I think we need a pattern for this - I'm going to introduce one.

davkean · 2017-03-27T02:04:18Z

...udio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectPropertiesProvider.cs

+    /// <see cref="VSLangProj.ProjectProperties"/>. This enables us to provide
+    /// ProjectProperties to the Project Property Pages and maintain Backward Compatibility.
+    /// </summary>
+    internal class VsLangProjectPropertiesProvider


Why does this class even exist, why can't we just export the VsLangProjectProperties?

davkean · 2017-03-27T02:09:17Z

...VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties.cs

+    /// <summary>
+    /// See <see cref="VsLangProjectPropertiesProvider"/> for more info.
+    /// </summary>
+    internal partial class VsLangProjectProperties : VSLangProj.ProjectProperties


This is our VSProject implementation and should be named as such.

davkean · 2017-03-27T02:10:27Z

...io.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties_VsProject.cs

+    /// This implementation of VSProject just redirects the VSProject call to the contained
+    /// VSProject object imported from CPS
+    /// </summary>
+    internal partial class VsLangProjectProperties : VSProject


Can you file a bug to implement VSProject2 here?

Opened #1869

davkean · 2017-03-27T02:11:00Z

...io.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties_VsProject.cs

+
+        public VSProjectEvents Events => _vsProject.Events;
+
+        // Implementation of VSProject to redirect the call to the actual VSProject object


This comment applies the entire class right? Why is it in the middle of it?

davkean · 2017-03-27T02:12:55Z

...io.ProjectSystem.Managed.VS/ProjectSystem/VS/Properties/VsLangProjectProperties_VsProject.cs

+using EnvDTE;
+using VSLangProj;
+
+namespace Microsoft.VisualStudio.ProjectSystem.VS.Properties


This should live in the Automation folder/namespace.

davkean · 2017-03-27T02:15:19Z

...ojectSystem.Managed.VS.UnitTests/ProjectSystem/VS/Properties/VsLangProjectPropertiesTests.cs

+            }
+
+            [Fact]
+            public void VsLangProjectProperties_OutputType_Get()


Use SetupProperty

Remove the ExportVsProfferedProjectService export

Export the VSProject with a higher order attribute This way, this export gets picked up before the CPS export which is exported with Zero ordering Convert obj string to Decimal for Lib Base address This property is backed by the rule which says to get the value after context which only returns the string formatted value even if the property is backed by a PageIntProperty

Few other refactorings

…interfaces VsProject and ProjectProperties Add doc comments

Add Properties to access ProjectProperties and Threading service easily Order the methods after the properties

…ties

…n directly

…rties, which was also being implemented

…ectSystem.VS.Automation.

…ous operations

basoundr · 2017-03-27T18:38:15Z

@davkean Feedback addressed

basoundr · 2017-03-28T16:53:09Z

Fixes #1709 #1468 #1378

dnfclas added the cla-already-signed label Mar 24, 2017

basoundr commented Mar 24, 2017

View reviewed changes

srivatsn reviewed Mar 24, 2017

View reviewed changes

tmeschter reviewed Mar 24, 2017

View reviewed changes

RaulPerez1 reviewed Mar 24, 2017

View reviewed changes

basoundr commented Mar 25, 2017

View reviewed changes

davkean requested changes Mar 27, 2017

View reviewed changes

This was referenced Mar 27, 2017

Remove OutputTypeEnumProvider and OutputTypeEx calculation based on the rules #1867

Open

Implement the remaining VsLangProj.ProjectProperties properties #1868

Open

basoundr added 12 commits March 27, 2017 11:23

Add the required classes and dummy impl details

2fc3854

Implement some ProjectProperties Properties

a5ca0a1

Remove the ExportVsProfferedProjectService export

Use ThreadService JTF instead of ThreadHelper's JTF

87ecdf8

Few other refactorings

Refactor the class to partial classes to split the implementation of …

6602990

…interfaces VsProject and ProjectProperties Add doc comments

Add tests for VsLangProjectPropertiesProvider

c2d616e

Get the Threading service directly

9a0bd13

Add Properties to access ProjectProperties and Threading service easily Order the methods after the properties

Fix the test with the appropriate parameter name

709bf7e

Add tests for ProjectProperties implementation of VsLangProjectProper…

e19db12

…ties

Remove Provider and export the Properties and VSProject implementatio…

e51c40a

…n directly

Rename VSProject implementation to VSProject rather than ProjectPrope…

c14e6e9

…rties, which was also being implemented

Change the Namespace to be contained with Microsoft.VisualStudio.Proj…

3b0829b

…ectSystem.VS.Automation.

basoundr force-pushed the ProjectPropertiesProvider branch from 6a65ab3 to 097e3bf Compare March 27, 2017 18:30

Use IProjectThreadingService.ExecuteSynchronously to perform synchron…

403d07f

…ous operations

basoundr force-pushed the ProjectPropertiesProvider branch from 097e3bf to 403d07f Compare March 27, 2017 18:37

davkean approved these changes Mar 27, 2017

View reviewed changes

Edit tests to test Property Sets as well

afa2c57

basoundr merged commit e53c25d into dotnet:master Mar 28, 2017

basoundr deleted the ProjectPropertiesProvider branch March 28, 2017 16:27


		public prjOriginatorKeyMode AssemblyOriginatorKeyMode { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }

		public bool DelaySign { get => throw new NotImplementedException(); set => throw new NotImplementedException(); }


		public VSProjectEvents Events => _vsProject.Events;

		// Implementation of VSProject to redirect the call to the actual VSProject object

Conversation

basoundr commented Mar 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basoundr commented Mar 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davkean Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basoundr commented Mar 24, 2017 •

edited

Loading

davkean Mar 27, 2017 •

edited

Loading

basoundr commented Mar 27, 2017 •

edited

Loading