LibraryViewCustomization Service implementation by sharadkjaiswal · Pull Request #7963 · DynamoDS/Dynamo

sharadkjaiswal · 2017-06-14T09:00:04Z

Purpose

This PR implements ILibraryViewCustomization service and registers it when the LibraryViewExtension is loaded. Host applications can use this service to update the LayoutSpeficiation for the nodes loaded by the host application. For now, the host applications won't be able to modify the default layout specification as loaded by DynamoCore, the clients can only add new sections and add child elements or include info to a specific section.

For example, DynamoRevit may use the customization service to categorize all the Revit nodes under Revit Category in default section, otherwise, the Revit nodes will appear under Miscellaneous section.

DynamoModel model; //initialized before
var customization = model.ExtensionManager.Service<ILibraryViewCustomization>();
//Create a new category element for Revit items
var element = new LayoutElement("Revit") { elementType = LayoutElementType.category };
//Add some include path or child elements
element.include.Add(new LayoutIncludeInfo() { path = "Revit" });
element.include.Add(new LayoutIncludeInfo() { path = "Analyze" });
//finally add the new element to the customization.
customization.AddElements(new []{element});

This PR also implements LayoutSpecProvider that makes use of the LibraryViewCustomization service to provide the current specification. The custom nodes, as well as other nodes that are imported using ImportLibrary, are now added to "Add-ons" section.

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@benglin
@Randy-Ma

sharadkjaiswal · 2017-06-14T10:16:40Z

+        /// <returns>A cloned LayoutSpecification object</returns>
+        public LayoutSpecification Clone()
+        {
+            var s = ToJSONStream();


I must dispose this stream.

User-Zhaoyang · 2017-06-15T03:13:32Z

+        {
+            get
+            {
+                return root.Clone();


Why is a copy returned? Usually for property get methods, I have never seen cases where a copy is returned. If you really want to return a copy, it is better to add a specific method for that.

correct, I also didn't feel right about it. Hence the interface doesn't have Specification property rather it has a method GetSpecication(). The reason we don't want to pass the original specification is that a client can then modify the content of the specification with informing the customization service and we won't be able to refresh the library view. I have addressed this issue by removing this property and providing an explicit setter method.

aparajit-pratap · 2017-06-15T09:27:43Z

+        /// <summary>
+        /// List of LayoutSections
+        /// </summary>
+        public List<LayoutSection> sections { get; set; }


Property names begin with caps, no?

When this object is marshaled to JS then the property names starts with lowercase. I just wanted to keep it with the same convention, so confusion.

aparajit-pratap · 2017-06-15T09:40:47Z

+        /// <summary>
+        /// List of data types that should be included under this given library element
+        /// </summary>
+        public List<LayoutIncludeInfo> include { get; set; }


name can be Includes? Why not just make this IEnumerable?

I think it's convenient for serialization.

aparajit-pratap · 2017-06-15T09:51:36Z

+            var stream = assembly.GetManifestResourceStream(resource);
+
+            //Get the spec from the stream
+            var spec = LayoutSpecification.FromJSONStream(stream);


There's a possibility that the stream doesn't deserialize into a layout spec? Should we not handle this?

The stream used here is obtained from a static resource, so it shouldn't be a problem unless someone tampers that JSON file. If the stream is not valid then the returned spec is null.

aparajit-pratap · 2017-06-15T09:55:55Z

                        : string.Empty
            };

-            //If the node search element is part of a package, then we need to prefix pkg:// for it


why is this removed?

This is not needed, this case is already handled in GetFullyQualifiedName() method.

User-Zhaoyang · 2017-06-15T10:34:40Z

+        }
+
+        /// <summary>
+        /// Allows clients to add a list of include path to a given section. This 


"include path" is confusing me. I do not think the word "path" is proper.

User-Zhaoyang · 2017-06-15T10:46:21Z

+            var spec = LayoutSpecification.FromJSONStream(stream);
+            customization.AddSections(spec.sections);
+            this.customization = customization;
+            this.customization.SpecificationUpdated += OnSpecificationUpdate;


When this event is triggered, it won't automatically refresh UI. Right?

The controller also subscribes to this event and raises event for "libraryDataUpdated". UI refresh happens by subscribing to this event from library.html.

User-Zhaoyang · 2017-06-15T11:01:28Z

+        /// <summary>
+        /// 
+        /// </summary>
+        public bool inclusive { get; set; }


what does it mean if this is false?

If the include path is "A.B.C" under an element say "Root" and for two nodes fully qualified name is "A.B.C.D" and "A.B.C.E", then by default inclusive is true that means there we will get a category "C" under "Root" element as parent and "D" and "E" as children. If inclusive is false then both "D" and "E" will be direct children of "Root" and there won't be any intermediate element "C".

If inclusive is true, will it look like this?

Root |_ A |_ B |_ C |_ D |_ E

Actually, it would be Root -> C -> D and Root -> C -> E if inclusive is true otherwise it would be Root -> D & Root -> E

User-Zhaoyang · 2017-06-15T11:18:45Z

+            }
+            section.include.AddRange(includes);
+            int count = section.include.Count;
+            if (section.include.GroupBy(e => e.path).Count() < count) return false;


What does it mean when this happens? User error? If this is taken as a user error, then better error message should be there like which includes are duplicated.

Yes, it's a kind of user error, we can either throw an exception or do a console log.

User-Zhaoyang · 2017-06-15T11:42:35Z

+        /// of the given section conflicts with the path property of the given 
+        /// includes.
+        /// </summary>
+        public bool AddIncludeInfo(IEnumerable<LayoutIncludeInfo> includes, string sectionText = "")


Duplicated code between AddIncludeInfo and AddElements.

These are similar looking code, but operations are different.

sharadkjaiswal · 2017-06-15T13:34:02Z

-                            var name = NodeItemDataProvider.GetFullyQualifiedName(e);
-                            return string.IsNullOrEmpty(s) ? name : string.Format("{0}, {1}", s, name);
-                        }
+            customization.SpecificationUpdated += (o,e) => controller.RaiseEvent("libraryDataUpdated");


UI refresh happens because of 'libraryDataUpdated" event raised from the controller when SpecificationUpdated is handled.

I see, but if OnSpecificationUpdate is handled after the event handler here, then UI won't be updated.

User-Zhaoyang · 2017-06-16T05:59:11Z

+        /// Default constructor
+        /// </summary>
+        /// <param name="text">Text value of the element</param>
+        public LayoutSection(string text = "") : base(text)


Do we allow a section with text of ""?

* Recategorizing built-in methods (#7612) * Recategorize built-in methods * Added methods for List.cs, Math.cs and ImportExport.cs * Updated Math.MapTo * Updated List.cs * Icons and test cases - built-in methods are hidden from user's search - test cases moved to DSCoreNodesImages.resx - minor refinement to some methods - added test cases for List.cs and Math.cs * Set copy local in DynamoCoreWpfTests to false * Updated methods and test cases - Implementation for SetUnion, SetIntersection and SetDifference are the same as built-in methods - Edited ImportExport.ImportFromCSV to reuse the implementation from built-in - Added test cases for ImportExport.ImportFromCSV and fixed other test cases - Consolidated List.Contains with List.ContainsItem - Consolidated NormalizeDepth(list, rank) with NormalizeDepth(list) - Hid some of the built-in methods - Public methods and private helper methods are grouped using regions - Overload of List.Flatten is renamed as FlattenCompletely * Fixes for built in method test cases * Hide SortIndexByValue overload from builtin * Edited List, Math and ImportExport functions - consolidated CSV.ReadFromFile with ImportExport.ImportFromCSV - renamed ImportFromCSV to ImportCSV - moved CSV.WriteToFile to ImportExport.ExportCSV - fixed some of the list functions - reused builtin implememtations for Math.Map and Math.MapTo - moved test cases for CSV functions to ImportExport * migrations for csv.writetofile replacement of CSV.WriteToFile to ImportExport.ExportCSV * migrations for builtin nodes * Renamed FlattenCompletely and removed GetValues * updated icon for list.flatten * Update List.Flatten test case and revert migration for File.ExportToCSV * Consolidate Display nodes for re-categorization * Add one test case for migrating the very old ByGeometryColor node * LibraryViewExtension project for hosted library UI (#7756) * LibraryViewExtension project for hosted library UI * Serve library view with local data * Enable Node creation and console log (#7775) * Cleanup the solution file * Details view implementation over Dynamo canvas (#7780) * Details view implementation over Dynamo canvas * Fix visibility of details view * DetailsView integration (#7788) * Add missing resources. * Tabview to show library view and package list view * Provide Javascript hookup to update active package (#7791) * Fix a typo * Method to load installed packages in JSON format (#7789) * installed packages in json format * updated method to obtain installed pkg * removed getInstalledPackagesJSON() * reverted change to library.html * reverted previous changes * Re-fix a typo * Implement Install Package method on controller (#7793) * Update the bundle for latest updates in the UI. (#7794) * Consolidate some nodes from Logic and Formula to Math (#7757) * move some methods from Logic and Formula to Math * add entries to migration xml * add migration testcase * Remove List.Flatten overload (#7772) * remove list.flatten overload * Updated Dynamo.All.Sln * add migration test case * Update resources and js code from library.js and PMUI (#7796) * Recategorize "ColorRange2D" nodes into "ColorRange" (#7763) * Rename "ColorRange2D" to "ColorRange" * Add migration test case for ColorRange nodes * Install integration and style update (#7797) * Fix the issue that one json string may be invalid (#7802) * Library reorg (#7803) * Added 15 more packages to the list. * updated files to show packages. * Consolidate all file paths nodes under Import/Export category (#7776) * move nodes from file to importexport * add a migration testcase * change category name on ui to Import/Export * revert the slash * change namespace back to DSCore * rename namespace back to DSCore.IO * update loadedtypes.json * Cleanup LibraryReorg branch and remove PMUI code (#7804) * Update the library UI to recategorize two nodes * Consolidate all Directory related nodes to Import/Export Category (#7761) * consolidate directory nodes and rename importexport class * updated DirectoryFromPath and added migrations * change namespace from DSCore.IO to ImportExport * rename namespace and move directory methods to files.cs * add migration test cases * add directory.frompath and update nodemigrationtests * resolve conflicts * updated files * Update layoutSpecs and loadedTypes for ColorRange nodes * add nodes to loadedtype (#7806) * update layoutspecs and loadedtypes * Consolidate Excel nodes under Import/Export category (#7809) * recategorize excel methods * added migration and test case * migration for all excel.write node * move excel and csv methods to dsoffice * remove unnecessary attribute * add migration for DSCore.IO.File.ExportToCSV * Implement custom resource handlers (#7817) * add migration for csv.readfromfile * remove csv.readfromfile icons * Implement resource provider for runtime loadedTypes JSON data (#7826) * Implement resource provider for runtime loadedTypes JSON data * Add search keywords to LoadedTypes * Remove empty keywords * Tooltip implementation for library items (#7822) * Tooltip implementation for library items * Update librarie.min.js to include latest changes * Update LayoutSpecs.json and js to support sections (#7833) * Recategorize two GeometryColor nodes * Add ImportExport.CAD nodes under library (#7832) * Add ImportExport.CAD nodes under library * Restore lost changes * add test case for builtin migration (#7830) * Wrap left-over builtins into DS class for categorization and autocomplete (#7819) * wrap builtins into DS class for categorization and autocomplete * code cleanup * code cleanup * address review comments * addressed review comments * add checks for only static FFI classes to be derived from DS class * code cleanup * code cleanup * added migration, tests * revert changes to loadedtypes json file * updated layout spec json file * add test cases for static class and fix some tests * fixed prototest * Re-recategorize ColorRange nodes (#7840) * updated layoutSpecs.json (#7841) * Refactor the code so the LibraryViewController.cs can be tested (#7834) * Refactor the code so the LibraryViewController.cs can be tested Move methods to seperate class EventController, so this class can be used for mock testing. Derived class object LibraryViewController needs parameters which cannot be mocked. * Add moq test for LibraryViewController * To address the review comments from PR #7834 - Modify AssemblySharedInfo.cs to update CopyRight information of assmblies to 2017 - Remove constructor LibraryViewController() - Revert changes to Dynamo.All.sln realted to visual studio version - Add AssemblySharedInfo.cs to ViewExtensionLibraryTests so the assembly version is set to get the standard versioning * Modify the output path in ViewExtensionLibraryTests.csproj to match the properties from props * Add files related to commit -c03544f * Revert changs to AssemblySharedInfo.cs Modify AssemblySharedInfo.tt to update CopyRight information from 2016 to 2017 * Update librarie.js and other resources (#7842) * Revise category icons for library * add parameters for overloads * removed redundant path * merging new LibUI changes to Dynamo side * Implement IconResourceProvider to get icons for library items (#7869) * Implement IconResourceProvider to get icons for library items * remove methods and fields that are not required * Use category as read from customization xml * address review comments * Add unit tests for ResourceProviders (#7879) * Add unit tests for RespurceProviders * Rename ViewExtensionLibraryTests to LibraryResourceProviderTests - Update json file - Address review comments * Ensure that CopyLocal=False for Dynamo binaries (#7881) * Fix Icon resources for library items (#7880) - Rename BuiltIns.ds to BuiltIn.ds so that the icons for nodes from BuiltIn.ds could be resolved from BuiltIn.customization.dll * Update librarie.js and integrate the new API * Update library minimized javascript file to latest * Fix for DYN-870, removed Keys section from Organize group (#7895) * Remove unused LibraryViewExtension font resources (#7893) * Add description (#7904) * Prevent loading LibraryViewExtension in test mode to prevent NUnit crash (#7906) * wrap builtins into DS class for categorization and autocomplete * code cleanup * code cleanup * address review comments * addressed review comments * add checks for only static FFI classes to be derived from DS class * code cleanup * code cleanup * added migration, tests * revert changes to loadedtypes json file * updated layout spec json file * add test cases for static class and fix some tests * fixed prototest * disable library view extension in test mode * update core UI test to run when test mode is set to true and LibraryViewExtension is loaded * Integrate ImportLibrary UI (#7905) * Make it possible to use an external search function * Update library minimized javascript file to latest * Fix the build error in one test module * Disable right-click on LibraryView (#7913) * DYN-862: Implement event notification for libraryDataUpdated (#7896) * Implement event notification for libraryDataUpdated * update documentation * Event Observer * More testc ases * Don't use old results when throttle re-fires * Address review comments * Addressing review comments * Add node icon for Label.ByPointAndString * Addressing more review comments * fix failing excel tests (#7932) * Fixed failing tests on LibraryReorg branch (#7939) * fix failing excel tests * fixed tests * changed comments * Implement IServiceManager interface (#7949) * build fix, update csproj (#7950) * Add List.Equals overload to List BuiltIn class (#7954) * fix failing excel tests * fixed tests * changed comments * added builtin overload for List.Equals * Remove SearchView and LibrarySearchView WPF views and related tests (#7956) * fix failing excel tests * fixed tests * changed comments * added builtin overload for List.Equals * remove SearchView and LibrarySearchView WPF views * restore deleted comments * Remove v0.0.1 from resource path (#7961) * Fixed merge issues * Create DynamoVisualProgramming.DynamoCoreNodes.nuspec * Include assembly name in fully qualified name (#7971) * Include assembly name in fully qualified name * Create ZeroTouchSearchElement.cs * LibraryViewCustomization Service implementation (#7963) * LibraryViewCustomization Service implementation * Address review comments * Address more review comments * Provides support for default icon when resource not found (#7975) * Provides support for default icon when resource not found * Include concurrency test * Update librarie.js minified and resources (#8004) * Update library resources and categorization (#8010) * Update library resource and categorization * Remove ImportExportTests as its already moved to DSOfficeTests * update librarie.min.js * Implement Resource registration and app shutdown notification mechanism (#8014) * Implement Resource registration and app shutdown notification mechanism * Update LibraryViewExtension.Dispose implementation * Address review comments * Update DSCoreNodes.Migrations.xml removed unnecessary migration path for ExportCSV node

LibraryViewCustomization Service implementation

214665e

sharadkjaiswal commented Jun 14, 2017

View reviewed changes

sharadkjaiswal requested a review from aparajit-pratap June 15, 2017 02:34

User-Zhaoyang reviewed Jun 15, 2017

View reviewed changes

sharadkjaiswal mentioned this pull request Jun 15, 2017

Implement IServiceManager interface #7946

Merged

3 of 7 tasks

Address review comments

2662141

aparajit-pratap reviewed Jun 15, 2017

View reviewed changes

User-Zhaoyang reviewed Jun 15, 2017

View reviewed changes

sharadkjaiswal commented Jun 15, 2017

View reviewed changes

User-Zhaoyang reviewed Jun 16, 2017

View reviewed changes

Address more review comments

60b2f1e

sharadkjaiswal merged commit 0b0d1c0 into DynamoDS:LibraryReorg Jun 16, 2017

sharadkjaiswal deleted the specprovider branch June 16, 2017 09:30

Conversation

sharadkjaiswal commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

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!

sharadkjaiswal Jun 15, 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!

aparajit-pratap Jun 16, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sharadkjaiswal commented Jun 14, 2017 •

edited

Loading

sharadkjaiswal Jun 15, 2017 •

edited

Loading

aparajit-pratap Jun 16, 2017 •

edited

Loading