Skip to content

DYN-8338 Enable a new Marker on Input and Output Port for Launching Node AutoComplete#15947

Merged
BogdanZavu merged 30 commits intoDynamoDS:masterfrom
johnpierson:DYN8338_AddAutoCompleteMarker
Mar 21, 2025
Merged

DYN-8338 Enable a new Marker on Input and Output Port for Launching Node AutoComplete#15947
BogdanZavu merged 30 commits intoDynamoDS:masterfrom
johnpierson:DYN8338_AddAutoCompleteMarker

Conversation

@johnpierson
Copy link
Copy Markdown
Member

@johnpierson johnpierson commented Mar 19, 2025

Purpose

This PR adds a new marker that appears on output ports during hover for node autocomplete. If approved, this would replace the double click paradigm and decouple autocomplete from wire connection interference.

20250319-dnaProposal

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

@Amoursol @mjkkirschner

@johnpierson
Copy link
Copy Markdown
Member Author

Related to DYN-8338 and DYN-8398

@johnpierson johnpierson changed the title [DYN-8338] Enable a new Marker on Output Port for Launching Node AutoComplete DYN-8338 Enable a new Marker on Output Port for Launching Node AutoComplete Mar 19, 2025
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

Comment thread src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs Outdated
Comment thread src/DynamoCoreWpf/UI/Themes/Modern/OutPorts.xaml Outdated
Comment thread src/DynamoCoreWpf/UI/Themes/Modern/OutPorts.xaml Outdated
@johnpierson
Copy link
Copy Markdown
Member Author

johnpierson commented Mar 19, 2025 via email

@johnpierson johnpierson requested a review from BogdanZavu March 20, 2025 13:07
Comment thread src/DynamoCoreWpf/ViewModels/Core/PortViewModel.cs Outdated

private void TryShowAutoCompleteMarker()
{
//show the node autocomplete marker if available, skip codeblocks and watch nodes
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.

👍🏻

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

BogdanZavu commented Mar 20, 2025

In NodeAutoCompleteSearchControl.xaml
EventSetter Event="PreviewMouseLeftButtonUp"Handler="OnMouseLeftButtonUp"
Let's replace this with MouseLeftButtonDown - the reason is ButtonUp escapes sometimes (we are only preventing LeftButtonDown from propagation ) and it can trigger node placement accidentally from the NDA popup list.

@BogdanZavu
Copy link
Copy Markdown
Contributor

Let's add this to the inports as well.

<!-- Bind NodeAutoComplete to double left click -->
<Grid.InputBindings>
<MouseBinding Command="{Binding Path=NodeAutoCompleteCommand}" MouseAction="LeftDoubleClick" />
</Grid.InputBindings>
Copy link
Copy Markdown
Contributor

@QilongTang QilongTang Mar 20, 2025

Choose a reason for hiding this comment

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

@BogdanZavu @johnpierson Should we hold on touching the behavior for regular autocomplete yet? Only apply to cluster autocomplete for now? We can use a dedicated command for cluster autocomplete if needed

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.

Not really, I think the change is solid. So let's go ahead with the general improvement. We can ask for some help with testing from @jnealb once it's in.

@johnpierson johnpierson changed the title DYN-8338 Enable a new Marker on Output Port for Launching Node AutoComplete DYN-8338 Enable a new Marker on Input and Output Port for Launching Node AutoComplete Mar 20, 2025
@QilongTang
Copy link
Copy Markdown
Contributor

Buildtime errors:
D:\a\Dynamo\Dynamo\Dynamo\src\DynamoCoreWpf\ViewModels\Core\PortViewModel.cs(110,13): error RS0016: Symbol 'NodeAutoCompleteMarkerVisible.get' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md) [D:\a\Dynamo\Dynamo\Dynamo\src\DynamoCoreWpf\DynamoCoreWpf.csproj]
D:\a\Dynamo\Dynamo\Dynamo\src\DynamoCoreWpf\ViewModels\Core\PortViewModel.cs(111,13): error RS0016: Symbol 'NodeAutoCompleteMarkerVisible.set' is not part of the declared public API (https://github.com/dotnet/roslyn-analyzers/blob/main/src/PublicApiAnalyzers/PublicApiAnalyzers.Help.md) [D:\a\Dynamo\Dynamo\Dynamo\src\DynamoCoreWpf\DynamoCoreWpf.csproj]

@johnpierson
Copy link
Copy Markdown
Member Author

johnpierson commented Mar 21, 2025

I moved that method to internal. So hopefully that works @QilongTang
That didn't work, so I might have to add this new method to PublicApi?

Comment thread src/DynamoCoreWpf/UI/Themes/Modern/InPorts.xaml Outdated
Comment thread src/DynamoCoreWpf/UI/Themes/Modern/OutPorts.xaml Outdated
<interactivity:InvokeCommandAction Command="{Binding NodeAutoCompleteCommand}" PassEventArgsToCommand="True"/>
</interactivity:EventTrigger>
</interactivity:Interaction.Triggers>
</Border>
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.

@BogdanZavu @johnpierson Would you remind me this would not make this part of every port at the view initialization right? Just thinking about performance impact

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.

So we only added one border - while collapsed this will not participate in the rendering , sizing recalculation etc so performance should remain the same. Same thing for the highlight.

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.

Other than two comments, LGTM

@BogdanZavu
Copy link
Copy Markdown
Contributor

Cool! LGTM! Let's add those color defines and I think we're good! *✧

- Add marker to output port
- Hook marker to node autocomplete command (single click)
- Enable visibility when no connection exists

(Apologies for this all being one commit as I had to refork, rebranch and all sorts of mess because of a merge conflict with a previous PR)
Tie Node AutoComplete Marker to be visible only when it is enabled in preferences
@johnpierson johnpierson force-pushed the DYN8338_AddAutoCompleteMarker branch from ef27dc1 to bd4b02f Compare March 21, 2025 17:23
private const double autocompletePopupSpacing = 2.5;
private const double proxyPortContextMenuOffset = 20;
internal bool inputPortDisconnectedByConnectCommand = false;
private bool nodeAutoCompleteMarkerVisible;
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.

@johnpierson guessing the failure reason - let's initialize this with false. I suspect during tests is true and the tests record the control no

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.

It is because the control is a child element of the port. So now it reports back 9 for this node when it was previously 6.
image

@QilongTang
Copy link
Copy Markdown
Contributor

QilongTang commented Mar 21, 2025

Sorry @johnpierson I introduced merge conflicts.. Addressed for you!

@johnpierson
Copy link
Copy Markdown
Member Author

party-dead

@johnpierson
Copy link
Copy Markdown
Member Author

I tested the 1 failure now locally and it seems to be passing. This should be good to go @QilongTang
image

@BogdanZavu
Copy link
Copy Markdown
Contributor

There is 1 failed test but I think it's not related to this change and it passes for me locally.
So I think we're good to go with this one @johnpierson @QilongTang

@johnpierson
Copy link
Copy Markdown
Member Author

@BogdanZavu - Last time I merged a PR, I did it early by accident so if you guys want to do it. :)

@BogdanZavu BogdanZavu merged commit 68ae401 into DynamoDS:master Mar 21, 2025
25 of 26 checks passed
@johnpierson johnpierson deleted the DYN8338_AddAutoCompleteMarker branch March 21, 2025 21:11
johnpierson added a commit to johnpierson/Dynamo that referenced this pull request Mar 27, 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