QNTM-5869: Search keywords split mechanism #9271

scottmitchell · 2018-11-27T22:02:08Z

Purpose

This PR addresses jira task QNTM-5869: Search keywords split mechanism.

This PR does:

Creates a new menu item under Settings > Experimental that toggles Experimental Search
Adds the full, unsplit search query to the list of queries in the search algorithm (when Experimental Search is ON)

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

@mjkkirschner @ColinDayOrg @Racel

…nto QNTM-5869

…-5869

scottmitchell · 2018-11-27T22:03:26Z

src/DynamoCoreWpf/Properties/Resources.en-US.resx

@@ -1855,9 +1855,6 @@ Want to publish a different package?</value>
  <data name="PublishPackageViewPackageVersion" xml:space="preserve">
    <value>Version (major minor build)</value>
  </data>
-  <data name="DynamoViewSettingsMenuIsolationMode" xml:space="preserve">


I'm not sure where these resource changes are coming from

mjkkirschner · 2018-11-27T22:08:00Z

src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

@@ -45,7 +45,7 @@
 // to distinguish one build from another. AssemblyFileVersion is specified
 // in AssemblyVersionInfo.cs so that it can be easily incremented by the
 // automated build process.
-[assembly: AssemblyVersion("2.1.0.4395")]
+[assembly: AssemblyVersion("2.1.0.6957")]


you should usually not commit this file.

Whoops, reverting this

mjkkirschner · 2018-11-27T22:08:32Z

src/DynamoCore/Search/SearchDictionary.cs

@@ -15,6 +15,8 @@ public class SearchDictionary<V>
        protected readonly Dictionary<V, Dictionary<string, double>> entryDictionary =
            new Dictionary<V, Dictionary<string, double>>();

+        public bool experimentalSearch = false;


can this be made private?

I don't think I can make it private here because I need to be able to access it from the DynamoViewModel. Maybe I'm missing something?

one option because this is an internal API is mark this internal and use [internalsVisibleAttribute] to make this property accessible from DynamoCoreWPF assembly.

mjkkirschner · 2018-11-27T22:09:33Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

@@ -291,6 +291,15 @@ public bool EnableTSpline
            }
        }

+        public bool ExperimentalSearch


should have a summary /// tag if this a public API - does it need to be public?

ColinDayOrg · 2018-11-27T23:08:07Z

src/AssemblySharedInfoGenerator/AssemblySharedInfo.cs

@@ -64,4 +64,4 @@
 // You can specify all the values or you can default the Build and Revision Numbers 
 // by using the '*' as shown below:
 // [assembly: AssemblyVersion("1.0.*")]
-[assembly: AssemblyFileVersion("2.1.0.4395")]
+[assembly: AssemblyFileVersion("2.1.0.6957")]


I don't think that this file is usually included in PRs.

mjkkirschner · 2018-11-28T16:32:55Z

src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs

+            get { return model.SearchModel.experimentalSearch; }
+            set
+            {
+                model.SearchModel.experimentalSearch = value;


I guess in this case it's not such a big deal, but you usually would not modify a field from another object - I think instead we usually use a property, in this case I think it makes sense to try and hide this field from external consumers - we don't want anyone to use it - in an extension for example.

mjkkirschner · 2018-11-28T16:34:45Z

@scottmitchell some other thoughts -
have you looked at the debug menu?(debug menu only shows up in debug builds) This might actually be better than the experimental menu because it will not be discoverable to users... I don't know if that was @Racel 's intention or not.

You could also use an ifdef compile time flag using the debug config.

Racel · 2018-11-28T16:40:57Z

@mjkkirschner @scottmitchell Either menu works. Just need it in some menu for testing.... if it looks ok, we will probably just make it the standard.

mjkkirschner · 2018-11-28T16:51:19Z

If thats the case and its not really for users, but for us, my feeling is that it should go into the debug menu - I would also like to hide the API since it doesn't seem like something we want people using from extensions or packages.

Racel · 2018-11-28T16:56:50Z

@mjkkirschner Sounds good to me. Let's put it in the debug menu and make the API private

scottmitchell · 2018-11-28T20:09:46Z

I think this is ready to go

ColinDayOrg · 2018-11-28T20:46:58Z

LGTM
Thanks for changing from Add to Insert(0) as well. I know it doesn't matter right now, but it may in the future.

mjkkirschner · 2018-12-05T22:34:27Z

@scottmitchell is this waiting on something?

QilongTang · 2018-12-06T05:28:00Z

@mjkkirschner This is waiting on @Racel 's review in R2020

ColinDayOrg · 2018-12-10T19:16:01Z

src/DynamoCore/Search/SearchDictionary.cs

@@ -310,6 +321,15 @@ internal IEnumerable<V> Search(string query, int minResultsForTolerantSearch = 0
            query = query.ToLower();

            var subPatterns = SplitOnWhiteSpace(query);
+


Just as a question; should this code be wrapped in #if DEBUG statements? Probably not necessary, but I am doing the same type of code and wanted to get the group thought about if it is needed/wanted or not.

I think because this code is already in the debug only menu it's fine not to use a compiler flag. On the other hand, for performance impacting code - like Console.WriteLine I would use a debug flag:
https://stackoverflow.com/questions/18464833/console-writeline-effect-on-performance

ColinDayOrg · 2018-12-10T19:17:49Z

src/DynamoCore/Search/SearchDictionary.cs

+        {
+            get { return experimentalSearch; }
+            set { experimentalSearch = value;  }
+        }


When implementing my own debug mode VS suggested;
internal bool ExperimentalSearch { get; set; } = false;

@ColinDayOrg Yep, that's cleaner. Fixed.

mjkkirschner · 2018-12-14T02:26:57Z

@scottmitchell there's now conflicts on this branch from your other PR I think - might making @Racel 's review harder.

scottmitchell · 2018-12-14T15:41:17Z

@mjkkirschner Look's like it was caused by @ColinDayOrg 's PR. I'll fix.

scottmitchell · 2018-12-14T16:54:06Z

src/DynamoCoreWpf/Views/Core/DynamoView.xaml

@@ -785,10 +785,15 @@
                                  Header="{x:Static p:Resources.DynamoViewDebugMenuShowDebugAST}"
                                  IsChecked="{Binding ShowDebugASTs}"></MenuItem>
                        <MenuItem Focusable="False"
-                                  Name="ExperimentalSearch"
+                                  Name="TruncateSearchResults"


@ColinDayOrg FYI I renamed your menu item name

Good catch, thanks. I am assuming that the "TruncateSearchResults" menu has been checked to test if it still works as expected after this change?

I just did a quick test to verify that the menu still works in the current master codebase after this specific change, so it seems ok to me.

ColinDayOrg · 2018-12-14T17:03:54Z

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QNTM-5869: Search keywords split mechanism #9271

QNTM-5869: Search keywords split mechanism #9271

scottmitchell commented Nov 27, 2018 •

edited

Loading

scottmitchell Nov 27, 2018

mjkkirschner Nov 27, 2018

scottmitchell Nov 27, 2018

mjkkirschner Nov 27, 2018

scottmitchell Nov 28, 2018

mjkkirschner Nov 28, 2018

mjkkirschner Nov 27, 2018

ColinDayOrg Nov 27, 2018

mjkkirschner Nov 28, 2018

mjkkirschner commented Nov 28, 2018 •

edited

Loading

Racel commented Nov 28, 2018

mjkkirschner commented Nov 28, 2018

Racel commented Nov 28, 2018

scottmitchell commented Nov 28, 2018

ColinDayOrg commented Nov 28, 2018

mjkkirschner commented Dec 5, 2018

QilongTang commented Dec 6, 2018

ColinDayOrg Dec 10, 2018

mjkkirschner Dec 10, 2018

ColinDayOrg Dec 10, 2018

scottmitchell Dec 10, 2018

mjkkirschner commented Dec 14, 2018

scottmitchell commented Dec 14, 2018

scottmitchell Dec 14, 2018 •

edited

Loading

ColinDayOrg Dec 14, 2018

ColinDayOrg Dec 14, 2018

ColinDayOrg commented Dec 14, 2018

		@@ -310,6 +321,15 @@ internal IEnumerable<V> Search(string query, int minResultsForTolerantSearch = 0
		query = query.ToLower();

		var subPatterns = SplitOnWhiteSpace(query);

QNTM-5869: Search keywords split mechanism #9271

QNTM-5869: Search keywords split mechanism #9271

Conversation

scottmitchell commented Nov 27, 2018 • edited Loading

Purpose

Declarations

Reviewers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Nov 28, 2018 • edited Loading

Racel commented Nov 28, 2018

mjkkirschner commented Nov 28, 2018

Racel commented Nov 28, 2018

scottmitchell commented Nov 28, 2018

ColinDayOrg commented Nov 28, 2018

mjkkirschner commented Dec 5, 2018

QilongTang commented Dec 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Dec 14, 2018

scottmitchell commented Dec 14, 2018

scottmitchell Dec 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ColinDayOrg commented Dec 14, 2018

scottmitchell commented Nov 27, 2018 •

edited

Loading

mjkkirschner commented Nov 28, 2018 •

edited

Loading

scottmitchell Dec 14, 2018 •

edited

Loading