DYN-4361: enabled clipboard copy for preview bubbles and watch nodes by dnenov · Pull Request #15844 · DynamoDS/Dynamo

dnenov · 2025-02-20T14:18:00Z

Purpose

Addresses the following Jira issue - Enable Ctrl + C to copy selected data out of preview bubbles/watch nodes

The solution was not as straightforward as the text nodes to copy from were children of a TreeView element.

The successful solution was to integrate the key capture on the level of the TreeViewItem manipulation, which was already working to enable the up/down navigation.
Finally, since the TreeViewItem's KeyDown event handler did not correctly capture a Ctrl+Key combination outright, we had to listen for the combination and string it manually together.
To enable this workflow, a stop-watch routine was also implemented to reset the timer between Ctrl occurrences.
Added test coverage for the new functionality.

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

enabled clipboard copy on Ctrl+C for info bubbles and watch nodes
added test coverage

Reviewers

@achintyabhat
@reddyashish
@zeusongit

FYIs

@QilongTang
@johnpierson

- enabled clipboard copy on Ctrl+C for info bubbles and watch nodes - added test coverage

github-actions

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

johnpierson · 2025-02-20T15:03:36Z

So far testing locally with copying various data and arrow key navigation, this looks pretty good to me.

zeusongit · 2025-02-20T16:19:57Z

src/DynamoCoreWpf/Views/Preview/WatchTree.xaml.cs

+        private void treeviewItem_KeyDown(object sender, System.Windows.Input.KeyEventArgs e)
        {
-            if (prevWatchViewModel != null)
+            if (prevWatchViewModel != null || Models.DynamoModel.IsTestMode)


Is prevWatchViewModel value null when in TestMode?

no it shouldn't be null.

then I am confused, why add the IsTestMode condition?

It was null, yes. At least when reusing the design pattern of the existing watchnode/info-bubble tests. I didn't feel I should go out of my way to look into creating the view model, as this solution worked and I have used it (bypassing checks based on Dynamo.IsTestMode) in the past in similar circumstances.

All tests of this test suite passed locally as well.

@dnenov Thank you for the new changes, looks clean and better.

Is there any unit test present for this functionality? Which tests are being affected, causing the need for IsTestMode check.

I will remove the IsTestMode , you are right! Apologies.

johnpierson · 2025-02-20T16:50:23Z

Is this going to be in 3.5?

zeusongit · 2025-02-20T17:11:58Z

Is this going to be in 3.5?

Yes

dnenov · 2025-02-20T19:49:34Z

src/DynamoCoreWpf/Views/Preview/WatchTree.xaml.cs

            this.Loaded += WatchTree_Loaded;
            this.Unloaded += WatchTree_Unloaded;
+
+            _ctrlResetTimer.Tick += _ctrlResetTimer_Tick;


We are using this pattern inside the DynamoView (_workspaceResizeTimer.Tick += _resizeTimer_Tick;) without unsubscribing, I assumed this is safe.

Can we unsubscribe to it? If possible? Probably add a Dispose method?

Of course. The Watchnode does not have a Dispose, but a similar thing is happening inside the Loaded/Unloaded handlers.

@zeusongit I removed the timer altogether. Sporadic tests were failing, I don't know if that was due to a leak, but I was able to remove the timer while tinkering. Sadly, I cannot figure out a way to test for the Keyboard.Modifiers combination, as far as I can see it's impossible to set that programmatically. So I don't have a way of Unit testing that functionality. I don't know if we have a different framework for Integration tests that we use to simulate clicks and user interactions, but probably that would be the way to do it.

Thanks @dnenov, I was iffy with the timer approach myself, so thanks for looking into it. I will ask if this is something AGT tests can cover.

Definitely ^

- moved subscribe/unsubscribe from timer events inside the loaded/unloaded parts of the watchnode

zeusongit · 2025-02-21T21:31:24Z

@dnenov One failed test that looks new, Dynamo.Tests.SerializationTests.InPortDescriptionDeserilizationTest

…d-copy-watchnode

- tooltip text change for the color node was causing a test to fail, fixing that although it's not part of the scope of this PR

dnenov · 2025-02-23T09:09:56Z

@dnenov One failed test that looks new, Dynamo.Tests.SerializationTests.InPortDescriptionDeserilizationTest

I fixed the test, although it was not related to this PR. Now a different test is failing, which passes locally RemovesScriptTagsFromLoadedHtml:

Could it be that changes to the master are interfering? This PR should not be affecting these tests.

- now uses `Keyboard.Modifiers ` to detect Ctrl+C combination, which eliminates the need for a timer altogether - no longer able to do a wpf test, I found it impossible to simulate the hardware keyboard.modifiers necessary for the test to run

reddyashish · 2025-02-24T14:52:43Z

RemovesScriptTagsFromLoadedHtml has been flaky on CI jobs.

reddyashish · 2025-02-24T15:48:39Z

test/DynamoCoreTests/SerializationTests.cs


            var nodeModelNode = this.CurrentDynamoModel.CurrentWorkspace.Nodes.Where(x => x.GUID == new Guid("c848cc3cb24a477f8248e53fc9304cc1")).First();
-            Assert.AreEqual(nodeModelNode.InPorts.First().ToolTip, "List of colors to include in the range");
+            Assert.AreEqual(nodeModelNode.InPorts.First().ToolTip, "List of colors to include in the range (disabled)");


Why is this changed?

Is this for fixing the unrelated failing test? https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/17216/testReport/Dynamo.Tests/SerializationTests/InPortDescriptionDeserilizationTest/

I don't think this is the expected fix here @dnenov , need to dig why and when was the tooltip changed. Also I think the expected and actual variables are inverted in the test.

Yes, it looks to me that this tooltip recently changed - I think it's on the Color node, or similar. And it's hard-coded inside the test, so that's the fix of it.

- remove unnecessary code

enabled clipboard copy

3d6f512

- enabled clipboard copy on Ctrl+C for info bubbles and watch nodes - added test coverage

github-actions bot reviewed Feb 20, 2025

View reviewed changes

QilongTang requested review from reddyashish and zeusongit February 20, 2025 15:20

zeusongit reviewed Feb 20, 2025

View reviewed changes

zeusongit added this to the 3.5 milestone Feb 20, 2025

dnenov commented Feb 20, 2025

View reviewed changes

unsubscribe from timer click

d67fe37

- moved subscribe/unsubscribe from timer events inside the loaded/unloaded parts of the watchnode

dnenov added 2 commits February 22, 2025 19:39

Merge remote-tracking branch 'upstream/master' into DYN-4361-clipboar…

b984795

…d-copy-watchnode

unrelated test failure fix

cf5689a

- tooltip text change for the color node was causing a test to fail, fixing that although it's not part of the scope of this PR

dnenov added 2 commits February 23, 2025 09:11

Trigger CI pipeline

a72200d

code refactor

7c347b4

- now uses `Keyboard.Modifiers ` to detect Ctrl+C combination, which eliminates the need for a timer altogether - no longer able to do a wpf test, I found it impossible to simulate the hardware keyboard.modifiers necessary for the test to run

reddyashish reviewed Feb 24, 2025

View reviewed changes

remove redundant code

4f72e64

- remove unnecessary code

zeusongit approved these changes Feb 24, 2025

View reviewed changes

zeusongit merged commit a727eae into DynamoDS:master Feb 24, 2025
22 of 24 checks passed

Conversation

dnenov commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Release Notes

Reviewers

FYIs

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

johnpierson commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusongit Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnenov Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

johnpierson commented Feb 20, 2025

Uh oh!

zeusongit commented Feb 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zeusongit commented Feb 21, 2025

Uh oh!

dnenov commented Feb 23, 2025

Uh oh!

reddyashish commented Feb 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dnenov commented Feb 20, 2025 •

edited

Loading

zeusongit Feb 20, 2025 •

edited

Loading

dnenov Feb 20, 2025 •

edited

Loading