This change fixes several issues: #1091

abpiskunov · 2017-01-06T02:46:34Z

support automation (VSLangProj.References) by switching to IProjectItemTree instead of IProjectTree. This is needed since CPS uses IProjectItemTree nodes with Reference flag to implement VSLangProj.References. (Note: this still would need some matching CPS changes to make sure we work OK for regular references (assemblies and projects) and packages (for packages CPS needs to listen for package xaml rules and add e new reference type). Note: i reuse a Folder xaml type for custom dependency nodes (implemented outside of managed project system), however we might want to create a separate BrowseObject xaml rule "GenericDependency" to be precise and add there standard set of properties.
organize dependency nodes flags: all generic (implemented by us nodes) will have CPS's Reference flags to allow CPS default commands behavior and our own Dependencies flags to allow to figure out if nodes are ours in the future. All custom nodes implemented outside of managed project system, will have only Dependencies flags to avoid CPS logic be executed for them - they are not References in old meaning of them.
fixes issue Dependencies sub nodes have objects for opening properties and View in object browser #398 and does not show View In ObjectBrowser for dependencies sub nodes. Properties is still showing up though
fixes The delete button doesn't work for nuget package references or project references in the solution explorer #864 and Remove UI button missing in Right-click list on Dependencies->Projects #708 to support Remove menu command and delete key for all standard top level dependencies (custom ones like web bower, should implement their own delete command)
fixes "View In Object Browser" does not work for project references #403 to show View in Object Browser command for standard dependencies, however projects are not displayed in the Browser (still investigating why - could be CPS) and also need to hide this command for packages, (this is also pending).

TODO:
projects are not displayed in the Browser (still investigating why - could be CPS) and also need to hide this command for packages, (this is also pending).
Properties is still showing up for Dependencies sub nodes. If it is controlled by some CPS command handler will override it, if by IVsUIHierarchy implementation would need CPS change.
see if we want to create a separate BrowseObject xaml rule "GenericDependency" for custom dependency nodes and add there standard set of properties.

abpiskunov · 2017-01-06T02:47:26Z

@dotnet/project-system @srivatsn @davkean @333fred @jviau @BillHiebert #Resolved

jviau · 2017-01-06T03:39:16Z

...dio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesGraphProvider.cs

-                        var childSubTreeProvider = childrenSubTreeProviders == null 
-                                                    ? subTreeProvider 
-                                                    : childrenSubTreeProviders[i];
+                        var childSubTreeProvider = childrenSubTreeProviders?[i];


Null coalesce to subTreeProvider? #Resolved

jviau · 2017-01-06T03:48:45Z

...dio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesGraphProvider.cs

+                                                 .FirstOrDefault();
                if (node == null)
                {
+                    // node is not ours or does node xist anymore


Spelling: node -> not, xist -> exist #Resolved

jviau · 2017-01-06T03:56:29Z

...ojectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesProjectTreeProvider.cs


+        private string ExtractNameFromCaption(string caption)
+        {
+            var index = caption?.IndexOf(' ');


Is space the right delimiter here for the caption? Does this mean no Dependency node can have a space in its name? #Resolved

jviau · 2017-01-06T03:58:04Z

...ojectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesProjectTreeProvider.cs

+                                                       .Add(Folder.FullPathProperty, string.Empty);
+
+            // For generic node types we do set correct, known item types, however for custom nodes
+            // provided by third party extensions we can not guaranty that item type will be known. 


spelling: guarnty -> guarantee #Resolved

jviau · 2017-01-06T04:02:49Z

...ojectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesProjectTreeProvider.cs

            return dependenciesNode;
        }

+


Remove extra newline here #Resolved

jviau · 2017-01-06T04:03:10Z

...ojectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesProjectTreeProvider.cs

Remove extra whitespace #Resolved

jviau · 2017-01-06T04:04:43Z

...dio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/SubTreeRootDependencyNode.cs

            Caption = caption;
            Icon = icon;
            ExpandedIcon = expandedIcon ?? Icon;
+            Flags = flags.Add(ProjectTreeFlags.Common.NonFileSystemProjectItem.ToString());


ToString() is not necessary with common flags. #Resolved

jviau · 2017-01-06T04:05:44Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/Mocks/IGraphContextFactory.cs

 using Microsoft.VisualStudio.ProjectSystem.VS.Tree.Dependencies;
 using Microsoft.VisualStudio.Shell.Interop;
 using Moq;
+using Microsoft.VisualStudio.Shell;


sort usings. #Resolved

jviau · 2017-01-06T04:06:35Z

...dio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesGraphProvider.cs

 using Microsoft.VisualStudio.Shell.Interop;
 using Microsoft.VisualStudio.Threading;
 using Task = System.Threading.Tasks.Task;
+using System.IO;


Sort usings #Resolved

jviau · 2017-01-06T04:07:54Z

...dio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesGraphProvider.cs


            graphContext.OutputNodes.Add(newNode);
-
+            


Remove extra whitespace #Resolved

abpiskunov · 2017-01-06T20:17:47Z

@jviau i followed up with all your comments. Good catch on "space" parsing, it was ugly and actually i found related issue which fixed in the new commit (searching for nodes in the tree was not right) #Resolved

333fred · 2017-01-06T21:38:52Z

src/Microsoft.VisualStudio.ProjectSystem.Managed.VS.UnitTests/Mocks/IVsHiearchyItemFactory.cs

+            return Mock.Of<IVsHierarchyItem>();
+        }
+
+        public static IVsHierarchyItem Implement(string text = null,


Could you say what method/getter you're implementing, in case we have to add new things in the future? #Resolved

333fred · 2017-01-06T22:35:53Z

...dio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesGraphProvider.cs

+                {
+                    var fullItemSpecPath = Path.GetFullPath(
+                                            Path.Combine(projectFolder,
+                                                         nodeInfo.Id.ItemSpec?.ToLowerInvariant()));


Path.Combine throws if an argument is null. Test if ItemSpec is null before calling. #Resolved

333fred · 2017-01-06T22:42:21Z

...dio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesGraphProvider.cs

+                    var fullItemSpecPath = Path.GetFullPath(
+                                            Path.Combine(projectFolder,
+                                                         nodeInfo.Id.ItemSpec?.ToLowerInvariant()));
+                    if (!string.IsNullOrEmpty(fullItemSpecPath))


Could this ever not be satisfied? GetFullPath will throw if the path is illegal (such as an empty string), and will otherwise return a string that cannot be null or empty. #Resolved

333fred · 2017-01-06T22:43:51Z

...dio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesGraphProvider.cs


            graphContext.OutputNodes.Add(newNode);
-
+            


Whitespace #Resolved

333fred · 2017-01-06T22:47:38Z

...ojectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesProjectTreeProvider.cs

-            {
-                path = "file:///" + path.Trim('/');
-            }
+            //if (!path.StartsWith("file:///"))


Let's not leave commented code in the code base. If this needed to be commented out because of a bug, please file one. #Resolved

333fred · 2017-01-06T22:47:53Z

...ojectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesProjectTreeProvider.cs

+            // For generic node types we do set correct, known item types, however for custom nodes
+            // provided by third party extensions we can not guarantee that item type will be known. 
+            // Thus always set predefined itemType for all custom nodes.
+            // TODO: generate specific xaml rule for generic Dependency nodes


Link issue. #Resolved

333fred · 2017-01-06T22:49:07Z

...ojectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependenciesProjectTreeProvider.cs

+
+            // when itemSpec is not in valid absolute path format, property page does not show 
+            // item name correctly. Use real Name for the node here instead of caption, since caption
+            // can have other info liek version in it.


Spelling on like #Resolved

333fred · 2017-01-06T23:19:46Z

...t.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Tree/Dependencies/DependencyNode.cs

+        /// Actual formal name of the node. In most cases it will be equal to Caption,
+        /// however in some cases caption would have name + version or something else.
+        /// So to allow tree provider store formal name we need this property too. 
+        /// It is a hack now and we should make it public and move to IDependencyNode when


Please file a bug for this. #Resolved

Additionally, when will be a better time to do this that we can make breaking changes? I find it highly unlikely we'll be able to make major breaking changes after we ship 1.0, so we should make this public now if we can. #Resolved

i agree. I opened a bug for RTW and if it is approved we coudl do that - it is not hard to implement, but logistics ... we need to synchronize insertions for core and web. It also can be done , just need an agreement :). Btw David Kean wanted also to rename some public Async methods , we could combine 2 changes.

In reply to: 95038283 [](ancestors = 95038283)

- support automation (VSLangProj.References) by switching to IProjectItemTree instead of IProjectTree. This is needed since CPS uses IProjectItemTree nodes with Reference flag to implement VSLangProj.References. (Note: this still would need some matching CPS changes to make sure we work OK for regular references (assemblies and projects) and packages (for packages CPS needs to listen for package xaml rules and add e new reference type). Note: i reuse a Folder xaml type for custom dependency nodes (implemented outside of managed project system), however we might want to create a separate BrowseObject xaml rule "GenericDependency" to be precise and add there standard set of properties. - organize dependency nodes flags: all generic (implemented by us nodes) will have CPS's Reference flags to allow CPS default commands behavior and our own Dependencies flags to allow to figure out if nodes are ours in the future. All custom nodes implemented outside of managed project system, will have only Dependencies flags to avoid CPS logic be executed for them - they are not References in old meaning of them. - fixes issue #398 and does not show View In ObjectBrowser for dependencies sub nodes. Properties is still showing up though - fixes #864 and #708 to support Remove menu command and delete key for all standard top level dependencies (custom ones like web bower, should implement their own delete command) - fixes #403 to show View in Object Browser command for standard dependencies, however projects are not displayed in the Browser (still investigating why - could be CPS) and also need to hide this command for packages, (this is also pending). TODO: - projects are not displayed in the Browser (still investigating why - could be CPS) and also need to hide this command for packages, (this is also pending). - Properties is still showing up for Dependencies sub nodes. If it is controlled by some CPS command handler will override it, if by IVsUIHierarchy implementation would need CPS change. - see if we want to create a separate BrowseObject xaml rule "GenericDependency" for custom dependency nodes and add there standard set of properties.

- make searching for tree node safer and make sure it works with all kinds of nodes properly - add a formal node name to DependencyNode to avoid weird string parsing when need to remove node's project item

333fred

Aside from the missing bug link for the todo, looks good.

dnfclas added the cla-already-signed label Jan 6, 2017

jviau reviewed Jan 6, 2017

View reviewed changes

333fred reviewed Jan 6, 2017

View reviewed changes

Anton Piskunov added 3 commits January 6, 2017 15:53

Followup with review comments

483afcf

- make searching for tree node safer and make sure it works with all kinds of nodes properly - add a formal node name to DependencyNode to avoid weird string parsing when need to remove node's project item

Follow up with more comments

582fe39

333fred approved these changes Jan 7, 2017

View reviewed changes

Add more unit tests for DependencyNodes and GraphProvider

4b03737

abpiskunov merged commit ea525af into dotnet:master Jan 7, 2017

This change fixes several issues: #1091

This change fixes several issues: #1091

Uh oh!

Conversation

abpiskunov commented Jan 6, 2017 • edited by davkean Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abpiskunov commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jviau Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abpiskunov commented Jan 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

333fred Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

333fred Jan 6, 2017 • edited by abpiskunov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

abpiskunov commented Jan 6, 2017 •

edited by davkean

Loading

abpiskunov commented Jan 6, 2017 •

edited

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

jviau Jan 6, 2017 •

edited by abpiskunov

Loading

abpiskunov commented Jan 6, 2017 •

edited

Loading

333fred Jan 6, 2017 •

edited by abpiskunov

Loading

333fred Jan 6, 2017 •

edited by abpiskunov

Loading

333fred Jan 6, 2017 •

edited by abpiskunov

Loading

333fred Jan 6, 2017 •

edited by abpiskunov

Loading

333fred Jan 6, 2017 •

edited by abpiskunov

Loading

333fred Jan 6, 2017 •

edited by abpiskunov

Loading

333fred Jan 6, 2017 •

edited by abpiskunov

Loading

333fred Jan 6, 2017 •

edited by abpiskunov

Loading

333fred Jan 6, 2017 •

edited by abpiskunov

Loading