improve node autocomplete results for custom node output ports #14218
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dont use type string, use name dont use name with spaces, it will never be correct.
mjkkirschner
changed the title
QilongTang
reviewed
Aug 3, 2023
| searchViewModel.PopulateAutoCompleteCandidates(); | ||
| Assert.AreEqual(7, searchViewModel.FilteredResults.Count()); | ||
| Assert.AreEqual(44, searchViewModel.FilteredResults.Count()); |
QilongTang
approved these changes
Aug 3, 2023
reddyashish
approved these changes
Aug 3, 2023
|
Regression: |
| @@ -760,6 +760,11 @@ internal IEnumerable<NodeSearchElement> GetMatchingSearchElements() | |||
| else if (PortViewModel.PortModel.PortType == PortType.Output) | |||
| { | |||
| portType = PortViewModel.PortModel.GetOutPortType(); | |||
| //if the custom node output name contains spaces, try using the first word. | |||
| if (PortViewModel.PortModel.Owner is Graph.Nodes.CustomNodes.Function && portType.Contains(" ")) | |||
There was a problem hiding this comment.
Should we generalize this check to include any white space?
There was a problem hiding this comment.
I can do that- I don't think any whitespace character is valid in a DS type name.
|
@QilongTang I don't think so - I'm going to merge this, if we see an increase in failures after this is merged, we can revert it. |
aparajit-pratap
approved these changes
Aug 3, 2023
|
I'm going to merge this - as @aparajit-pratap said the test failures also happen on master at this point. |
sm6srw
pushed a commit
to sm6srw/Dynamo
that referenced
this pull request
Mar 22, 2024
…oDS#14218) * update tests dont use type string, use name dont use name with spaces, it will never be correct. * add a new test * comments * review comments and new test
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
I could not reproduce the crash seen in #14192 in master (2.19).
I did notice however that checking customNode outputs frequently throws thousands of exceptions and also fails to find results even when the type name is a valid type!
I've made two changes I think will improve the situation a small bit.
Type.Nameinstead ofType.ToString()ToStringwill also include rank, likevar[]..[]- this is not a valid class name that can be found in the class table, and so we never make any matches with types that include rank. After this PR we'll better match different ranks of type, and also derived types.Note how more valid results are returned in the tests I have updated. Also less exceptions are thrown, so the results are computed a bit faster.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Return more results for object based node autocomplete when using custom node output ports.
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of