-
Notifications
You must be signed in to change notification settings - Fork 635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DYN-6535 DynamoRevit Improve Search #14878
DYN-6535 DynamoRevit Improve Search #14878
Conversation
In a search criteria like "list replace" I've implemented a fix that will check on many nodes we have when the terms splitted by empty space (in this case list and replace) so if it reaches the limit then we discard the split search and execute a normal search.
Adding extra comments
else | ||
searchType = SearchType.Normal; | ||
|
||
var trimmedSearchTerm = searchType == SearchType.ByEmptySpace ? searchTerm.Replace(" ", "") : searchTerm; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the actual node name has spaces in it? Will those show up with this condition?
Also since the ByEmptySpace is just used to check if the search term has to be trimmed or not, I don't think it should be a value inside SearchType enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reddyashish for the first question I think the Tokenizer that we are using is removing the empty spaces, just to confirm I've tested in DynamoSandbox with several nodes that use empty spaces and I'm getting the expected node always in first place (see image below) so there should not be any problem with nodes having empty space.
For the second comment we are using SearchType.ByEmptySpace in more conditions in the code that's why I considered to add it in the SearchType enum.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the confirmation @RobertGlobant20. Can't we just use that as a property to check for empty spaces then? Because the search algorithm is not altered, just the search term. Might not be a big thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Converted to be a property in the next commit: ff1b100
Convert the ByEmptySpace to be a property and remove it from the enum.
UI Smoke TestsTest: success. 2 passed, 0 failed. |
@@ -82,6 +84,15 @@ public enum LuceneStorage | |||
FILE_SYSTEM | |||
} | |||
|
|||
public enum SearchType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comments for public fields please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added extra comments: 584f794
Adding extra comments
Purpose
Improving Dynamo search on Revit
In a search criteria like "list replace" I've implemented a fix that will check how many nodes are shown when using the terms splitted by empty space (in this case list and replace) so if it reaches the limit then we discard the split search and execute a normal search (without empty spaces).
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
Improving Dynamo search on Revit
Reviewers
@QilongTang @reddyashish
FYIs