Use the host search function by User-Zhaoyang · Pull Request #7912 · DynamoDS/Dynamo

User-Zhaoyang · 2017-05-31T08:28:13Z

Purpose

This is to implement to use the old search function.

Here is the old search result when "list.s" is input in the search box:

Here is the search result now:

The order and content(only part of the result is in the view) are the same.

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

@sharadkjaiswal

FYIs

@benglin @riteshchandawar @Racel

…o LibraryReorg # Conflicts: # src/LibraryViewExtension/Handlers/NodeItemDataProvider.cs # src/LibraryViewExtension/LibraryViewController.cs

sharadkjaiswal · 2017-05-31T15:39:06Z

        {
-            supportedSchemes.Add(provider.Scheme);
-            resourceProviders.Add(baseurl, provider);
+            if (!resourceProviders.ContainsKey(baseurl))


I think we should not be changing resource provider for the same url. Url should have additional data to differentiate the different scenarios.

Again, I don't think we should allow updating the old provider.

sharadkjaiswal · 2017-05-31T15:40:34Z

    /// Provides json resource data for all the loaded nodes
    /// </summary>
    class NodeItemDataProvider : ResourceProviderBase
    {


I would rather subclass this class to implement node search data provider.

sharadkjaiswal · 2017-05-31T15:45:54Z

        refreshLibraryView(libController);

+        libController.searchLibraryItemsHandler = function (text, callback) {
+            let url = csharpController.searchLibraryItems(text);


We can directly have a url to provide a search service as http://localhost/nodesearch/{searchText} and register a provider for http://localhost/nodesearch/

…o LibraryReorg # Conflicts: # src/LibraryViewExtension/Handlers/NodeItemDataProvider.cs

sharadkjaiswal · 2017-06-01T11:16:15Z

@@ -86,7 +100,6 @@ public static string GetFullyQualifiedName(NodeSearchElement element)
        /// <returns></returns>
        internal LoadedTypeItem CreateLoadedTypeItem(NodeSearchElement element)


You can use Generics to implement this method so that you don't have to use the same code again for LoadedTypeItemExtended class.
internal T CreateLoadedTypeItem<T>(NodeSearchElement element) where T : LoadedTypeItem, new()

You mean the constructor of LoadedTypeItem?

sharadkjaiswal · 2017-06-01T11:21:33Z

+        /// </summary>
+        /// <param name="element"></param>
+        /// <param name="item"></param>
+        protected void InitializeLoadedTypeItem(NodeSearchElement element, LoadedTypeItem item)


Not sure why do we need this method?

This is the one that after an instance of LoadedTypeItem or its derived class is created, the data need to be initialized to set iconUrl or update fully qualified name. This will be the same in the base and the derived classes.

sharadkjaiswal · 2017-06-01T11:22:14Z

        {
-            supportedSchemes.Add(provider.Scheme);
-            resourceProviders.Add(baseurl, provider);
+            if (!resourceProviders.ContainsKey(baseurl))


Again, I don't think we should allow updating the old provider.

sharadkjaiswal · 2017-06-01T11:26:13Z

+            var uri = new Uri(url);
+            var pathAndQuery = uri.PathAndQuery;
+            var index = url.IndexOf(serviceIdentifier);
+            var text = url.Substring(index + serviceIdentifier.Length + 1);


what if the passed url is /search/point/bycoordinates, the text becomes point/bycoordinates?

you might consider having a method IEnumerable<LoadedTypeItem> GetLoadedTypeDataForRequest(Uri requestUri) then the base class implementation doesn't need to be overridden. Here is how the base class implementation would look like.

public override Stream GetResource(IRequest request, out string extension) { var uri = new Uri(request.Url); var items = GetLoadedTypeDataForRequest(uri); extension = "json"; return GetLoadedTypeDataStream(items); }

The old search behavior allows input text like "point/bycoordinates".

For the second point, then you need to override GetLoadedTypeDataForRequest. But as GetResource is only a function with few lines, why not override it directly?

sharadkjaiswal · 2017-06-01T11:28:35Z

+        /// <param name="element"></param>
+        /// <param name="w"></param>
+        /// <returns></returns>
+        private LoadedTypeItem CreateLoadedTypeItemExtended(NodeSearchElement element, int w = 0)


why is this method not returning LoadedTypeItemExtended instead of LoadedTypeItem?

sharadkjaiswal · 2017-06-01T12:43:31Z

            var sw = new StreamWriter(ms);
            var serializer = new JsonSerializer();
+
+            var data = CreateObjectForSerialization(searchEntries);


What if this method accepts list of LoadedTypeItem instead of searchEntities, then you won't need an additional method CreateObjectForSerialization?
protected Stream GetLoadedTypeDataStream(IEnumerable<LoadedTypeItem> items) {...}

CreateObjectForSerialization basically creates different instance of LoadedTypeData in the base class and the derived class.

sharadkjaiswal · 2017-06-01T12:44:28Z

    }

-    class LoadedTypeData
+    class LoadedTypeData<T>


you might want to put a condition for T to be derived from LoadedTypeItem

I am wondering if it would have worked as it is when loadedTypes internally contains the list of LoadedTypeItemExtended?

That is how template means? If it does not work, I doubt the reason why this language feature exists.

But if you mean LoadedTypeData<A> can not be casted to LoadedTypeData<B> directly, then you are right. But this is not the intention here.

User-Zhaoyang · 2017-06-08T03:53:28Z

@sharadkjaiswal
As discussed, I am merging this.

User-Zhaoyang added 3 commits May 31, 2017 16:11

Make it possible to use an external search function

1b24e69

Merge branch 'LibraryReorg' of https://github.com/DynamoDS/Dynamo int…

cc0e041

…o LibraryReorg # Conflicts: # src/LibraryViewExtension/Handlers/NodeItemDataProvider.cs # src/LibraryViewExtension/LibraryViewController.cs

Update library minimized javascript file to latest

271b1bb

User-Zhaoyang requested a review from sharadkjaiswal May 31, 2017 08:28

User-Zhaoyang assigned sharadkjaiswal May 31, 2017

Fix the build error in one test module

66a1f30

sharadkjaiswal reviewed May 31, 2017

View reviewed changes

User-Zhaoyang added 2 commits June 1, 2017 17:20

Addressing review comments

ffd6f87

Merge branch 'LibraryReorg' of https://github.com/DynamoDS/Dynamo int…

ecb0847

…o LibraryReorg # Conflicts: # src/LibraryViewExtension/Handlers/NodeItemDataProvider.cs

sharadkjaiswal reviewed Jun 1, 2017

View reviewed changes

User-Zhaoyang force-pushed the LibraryReorg branch from 78664b0 to c18d69d Compare June 5, 2017 02:40

Addressing more review comments

73eb9e0

User-Zhaoyang force-pushed the LibraryReorg branch from c18d69d to 73eb9e0 Compare June 5, 2017 02:51

User-Zhaoyang merged commit a56a7bb into DynamoDS:LibraryReorg Jun 8, 2017

		@@ -86,7 +100,6 @@ public static string GetFullyQualifiedName(NodeSearchElement element)
		/// <returns></returns>
		internal LoadedTypeItem CreateLoadedTypeItem(NodeSearchElement element)

Conversation

User-Zhaoyang commented May 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

FYIs

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!

User-Zhaoyang Jun 2, 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!

User-Zhaoyang Jun 2, 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!

User-Zhaoyang commented Jun 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

User-Zhaoyang commented May 31, 2017 •

edited

Loading

User-Zhaoyang Jun 2, 2017 •

edited

Loading

User-Zhaoyang Jun 2, 2017 •

edited

Loading