CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 658
DYN-8850: Reconfigure preference settings for node autocomplete #16230
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
Conversation
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.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-8850
/// <summary> | ||
/// Looks up a localized string similar to Node Type Match. | ||
/// </summary> | ||
public static string NodeTypeMatch { |
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.
Is our control auto-resizable horizontally ? This text will probably be bigger in certain languages.
I see it, it's auto.
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.
The whole flyout has a fixed size but the NodeTypeMatch textview is sized as Auto
, so it will compress the other views sized as *
(the buttons) to make space for itself while possible.
/// <summary> | ||
/// Event that is fired when a node is removed from the workspace. | ||
/// </summary> | ||
public event Action PreferencesChanged; |
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'm not sure about this one.
We are calling this from the onClose but there is no guarantee that the preferences have actually changed.
Plus the underlying object we care about is the PreferenceSettings instance and one can subscribe to that.
This can be modified from the API as well not only from the dialog.
However the Preferences dlg is modal so we should only do the update onClose when the dialog is present.
But I don't think we need to do the real time update so I'm ok with removing this.
So I would say to only consider the options on first occasion like the flyout show.
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.
Its weirder if its not realtime as the visual state gets desynchronized with the internal state. Its fine if we reload the view each time the user opens and closes the preferences, which shouldn't happen too often while the flyout is still visible.
We can have the best of both if we introduce an object to represent the latest copy of the preferences settings we're interested in and comparing with it, but it could introduce maintenance bugs when modifying the code and not updating that object.
So, I'd prefer it to load every time instead of get bugged when not refreshing. Plus, this may be useful for other parts of the application that would benefit from dynamically updating.
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.
Let's rename it then OnPreferencesClosed or something and maybe make it internal.
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.
Can't make it internal as we're using it from another assembly (the autocomplete view extension) but we can rename it
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.
We can access internals from our assembly since we have this [assembly: InternalsVisibleTo("NodeAutoCompleteViewExtension")].
But thinking more about it maybe we can avoid it all together and be more inline with the way other settings are handled.
So how about we define out DNA event in preferences for all the DNA settings and subscribe/react to it from the extension as usual.
The preferences is modal so we'll just need to postpone the action on Iddling - we do this in other contexts as well.
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.
Updated
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.
Very nice! Now we also have the api scenario covered and better handling of multiple req that could come from anywhere.
So in Node Type Match mode we are showing the total number of matches which is big. |
Oh and I guess we need to deprecate some of previous options like HideNodesBelowSpecificConfidenceLevel ? |
Those are not new, but the mockup no longer has that option. If we're sure it will stay this way in the future we can delete nonpublic members and obsolote public ones, but would need to confirm. |
Ok, we'll handle it later before 3.6. |
@@ -2870,6 +2870,7 @@ Dynamo.ViewModels.PreferencesViewModel.PackagePathsViewModel.get -> Dynamo.ViewM | |||
Dynamo.ViewModels.PreferencesViewModel.PackagePathsViewModel.set -> void | |||
Dynamo.ViewModels.PreferencesViewModel.PersistExtensionsIsChecked.get -> bool | |||
Dynamo.ViewModels.PreferencesViewModel.PersistExtensionsIsChecked.set -> void | |||
Dynamo.ViewModels.PreferencesViewModel.PreferencesChanged -> System.Action |
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.
to be removed
Let's remove that leftover api and we're good to go. |
Tests failed on unrelated |
Purpose
Reconfigure preference settings for node autocomplete
2025-05-16.21-07-29.mp4
Declarations
Check these if you believe they are true
*.resx
filesRelease Notes
Reconfigure preference settings for node autocomplete
Reviewers
@BogdanZavu
@johnpierson
FYIs
@QilongTang