Skip to content

[DYN-8338] Move Auto-Complete Marker Triggering to ViewModel Instead of Codebehind#15969

Merged
QilongTang merged 13 commits intoDynamoDS:masterfrom
johnpierson:dyn8338_autoCompleteMarkerRevisions
Mar 24, 2025
Merged

[DYN-8338] Move Auto-Complete Marker Triggering to ViewModel Instead of Codebehind#15969
QilongTang merged 13 commits intoDynamoDS:masterfrom
johnpierson:dyn8338_autoCompleteMarkerRevisions

Conversation

@johnpierson
Copy link
Copy Markdown
Member

@johnpierson johnpierson commented Mar 24, 2025

Purpose

This PR builds upon a previous PR (#15947).

The Node AutoComplete marker now shows on ports with no wires when the node is selected.

20250324-dna

Additionally, I changed the PreviewMouseDown to PreviewMouseLeftButtonDown because middle click was firing off the DNA command. 🙈

Related to DYN-8338

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

  • Introduction of a new in canvas marker (outside the port). This new marker, the Node Autocomplete Marker, allows for launching of the node autocomplete service.

Reviewers

@QilongTang @BogdanZavu @chubakueno @Jingyi-Wen

FYIs

@achintyabhat @Amoursol

Copy link
Copy Markdown

@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-8338

@QilongTang QilongTang added this to the 3.5 milestone Mar 24, 2025
}
else
{
Dispatcher.DelayInvoke(autoCompleteMarkerDelay, TryHideAutoCompleteMaker);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think the delay would be something we can define on XML layer? @johnpierson @BogdanZavu Just curious

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am not too sure but can research. Would be keen to your thoughts @BogdanZavu

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the xaml perhaps we could have some animation that comes with a delay but I'm a little worried about introducing too much complexity and issues related to animation that we might not see right away.

            <EventTrigger RoutedEvent="MouseLeave">
                <BeginStoryboard>
                    <Storyboard>
                        <DoubleAnimation Storyboard.TargetName="InfoPanel"
                                         Storyboard.TargetProperty="Opacity"
                                         To="0"
                                         BeginTime="0:0:1"  <!-- delay -->
                                         Duration="0:0:0.2" />
                    </Storyboard>
                </BeginStoryboard>
            </EventTrigger>

Or maybe we can just improve what we have a little bit and that should be enough without using a delay ?!
We could have the marker ON based on focus or on focus and mouse enter.
So if the node if selected/blue - we don't hide the marker on mouse leave - we will hide it on lost focus event.
And we continue to hide it on mouse leave if the control does not have focus.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I was worried that if multiple nodes have focus it will try to show the markers for all of those nodes, but maybe that is ok?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I really like the animation, because it just feels so clean. But if that could be an issue, that does worry me

Copy link
Copy Markdown
Contributor

@BogdanZavu BogdanZavu Mar 24, 2025

Choose a reason for hiding this comment

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

Yeah, imo I think it's ok/expected to show the marker for what's in the selection. I think we're inline with other controls that appear on selection.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think that makes sense to me. So effectively tie it to the blue selection border almost?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should tie it to the focus event if possible. The blue border control behavior might change in the future.

Copy link
Copy Markdown
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

LGTM but I am curious if there can be a cleaner way per my comment

Comment thread src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs Outdated
@BogdanZavu
Copy link
Copy Markdown
Contributor

BogdanZavu commented Mar 24, 2025

LGTM but I am curious if there can be a cleaner way per my comment

Yes, same LGTM!

@johnpierson
Copy link
Copy Markdown
Member Author

johnpierson commented Mar 24, 2025 via email

This enables the Marker to only show while the node is selected.
@johnpierson
Copy link
Copy Markdown
Member Author

johnpierson commented Mar 24, 2025

Alrighty @BogdanZavu and @QilongTang - I made the changes to allow it to work with selection instead of hover or mouse events. It seems to be working real nice on my testing. So if you could re-review.

it working while selected actually mirrors more how I would want it to work as a user. (Eg. the connections on Miro board notes)

@johnpierson johnpierson changed the title [DYN-8338] Add time to allow for display of node auto-complete marker. [DYN-8338] Move Auto-Complete Marker Triggering to ViewModel Instead of Codebehind Mar 24, 2025
@QilongTang
Copy link
Copy Markdown
Contributor

Waiting for checks to finish before merging

@johnpierson
Copy link
Copy Markdown
Member Author

Once we get this all sorted we’re going to need to see about getting this in the RC. https://github.com/DynamoDS/Dynamo/tree/RC3.5.0_master

@QilongTang
Copy link
Copy Markdown
Contributor

Once we get this all sorted we’re going to need to see about getting this in the RC. https://github.com/DynamoDS/Dynamo/tree/RC3.5.0_master

yeah, cherry-pick shouldn't be a problem considering this is an improvement instead of new request

@QilongTang QilongTang merged commit 2453280 into DynamoDS:master Mar 24, 2025
25 of 26 checks passed
@johnpierson johnpierson deleted the dyn8338_autoCompleteMarkerRevisions branch March 25, 2025 13:37
johnpierson added a commit that referenced this pull request Mar 25, 2025
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.

3 participants