Skip to content
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-7273 Search Issue Civil3D #15420

Merged

Conversation

RobertGlobant20
Copy link
Contributor

@RobertGlobant20 RobertGlobant20 commented Aug 6, 2024

Purpose

When a user in Civil3D was searching for a node like "choose text style" no results were not found in the top ten nodes.

This problem was happening due that when the SearchTerm contains empty space the algorithm parse the term in tokens so matches all the nodes which contain "choose", "text" and "style" in the node name and due that there is a restriction that the if the number of matching nodes > 20 then discard that case so is not showing results for that searchTerm.

Also there was a missing condition that when the term has spaces we should try to match the name with spaces, this was the code added in this commit and this is why now we are getting results when searching for "choose text style".

I realized that with my fix searching for "set parameter" in Revit was moving the SetParameterByName node from the 1st position to the 5th position then for fixing that problem I've modified the code in a way that will be using the default weight for Wildcard Query (instead of using the once calculated by the CalculateFieldWeight() method),

Also was reported that when searching for "file path" was not showing the File Path node in the list, it was happening due that the word "file" is also a Category so the results were showing the nodes under the File Category that contain the word "path" in the Name. For fixing this problem I removed the MUST for the category so it will show also extra nodes in the results (including the File Path node).

Declarations

Check these if you believe they are true

  • The codebase 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.
  • This PR modifies some build requirements and the readme is updated
  • This PR contains no files larger than 50 MB

Release Notes

When a user in Civil3D was searching for a node like "choose text style" no results were not found in the top ten nodes.

Reviewers

@QilongTang

FYIs

@reddyashish

When a user in Civil3D was searching for a node like "choose text style" no results were not found in the top ten nodes.

This problem was happening due that when the SearchTerm contains empty space the algorithm parse the term in tokens so matches all the nodes which contain "choose", "text" and "style" in the node name and due that there is a restriction that the if the number of matching nodes > 20 then discard that case so is not showing results for that searchTerm.

Also there was a missing condition that when the term has spaces we should try to match the name with spaces, this was the code added in this commit and this is why now we are getting results when searching for "choose text style".
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7273

@RobertGlobant20
Copy link
Contributor Author

This is a GIF of the expected behavior in Revit 2025.
Revit_F5aKSXf4oz

@RobertGlobant20
Copy link
Contributor Author

This is a GIF of the expected behavior in Civil3D 2025 Update 2025.1.
acad_AfiOIKtfJq

Copy link

github-actions bot commented Aug 6, 2024

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests

@QilongTang QilongTang added this to the 3.3 milestone Aug 12, 2024
I realized that with my fix searching for "set parameter" in Revit was moving the SetParameterByName node from the 1st position to the 5th position then for fixing that problem I've modified the code in a way that will be using the default weight for Wildcard Query (instead of using the once calculated by the CalculateFieldWeight() method),

Also was reported that when searching for "file path" was not showing the File Path node in the list, it was happening due that the word "file" is a Category so the results were showing the nodes under the File Category that contain the word "path" in the Name. For fixing this problem I removed the MUST for the category so it will show also extra nodes in the results (including the File Path node).
@RobertGlobant20
Copy link
Contributor Author

GIF showing the results of the search criteria "file path","set parameter", "element set parameter", "setparameter" on Revit.
Revit_QL9Hal8Uae

@RobertGlobant20
Copy link
Contributor Author

GIF showing the result of searching for "choose text style" in Civil3D.
acad_YAW8v7BFOW

Added a unit test that validates that the "File Path" node is found at the top 5 results when searching for the criteria "file path".
@QilongTang
Copy link
Contributor

Looks good but cant really merge right now with the builds failing. The failure is not related to this code though.

@QilongTang
Copy link
Contributor

Hi @RobertGlobant20 Please update your fork/branch with #15428

@@ -379,7 +379,6 @@ internal string CreateSearchQuery(string[] fields, string SearchTerm, bool IsPac
{
//Means that the first term is a category when we will be using the FullCategoryName for making a specific search based in the category
trimmedSearchTerm = matchingCategory?.FullCategoryName;
occurQuery = Occur.MUST;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reason behind removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when searching for criteria like "file path" with MUST we were creating a query like this one (consider that the word 'file' is a Category):

SELECT .... FROM ..... WHERE CATEGORY = 'file' AND (NAME LIKE 'path' or NAME LIKE ''path*)

so it was returning the two nodes in which Category = 'Core.File' that match the criteria related with the Name (the Name should contain 'path'),
But the user is expecting the "file path" node - this one didn't match due that belongs to the Category = "Core.Input" so removing the MUST makes the query more flexible showing also the nodes that matches the Name = 'file path' OR the Category = "Core.File".
Please let me know if this is clear enough.

@@ -409,6 +408,22 @@ internal string CreateSearchQuery(string[] fields, string SearchTerm, bool IsPac

if (searchTerm.Contains(' '))
Copy link
Contributor

@zeusongit zeusongit Aug 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we somehow combine this and the hasEmptySpaces flag, so that we do not have to check this twice, and probably avoid some confusion in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will suggest to consider this refactoring for a different Jira task since I remember it generated other problems also the hasEmptySpaces is under a condition that you added probably we will need to think more about it.
image

https://github.com/DynamoDS/Dynamo/pull/15338/files

@RobertGlobant20
Copy link
Contributor Author

@QilongTang any idea why the Dynamo.Tests.ExcelTests.ReadChangeFilename() test is failing, I was checking the test code and I don't see why it should be related.
image

@QilongTang
Copy link
Contributor

@QilongTang QilongTang merged commit 5dc7213 into DynamoDS:master Aug 16, 2024
23 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants