Adding a view element to let users expand the watch node. by reddyashish · Pull Request #11353 · DynamoDS/Dynamo

reddyashish · 2020-12-09T21:29:38Z

Purpose

This PR adds a functionality to let the users expand the watch node so that they can view the whole content.

Task: https://jira.autodesk.com/browse/DYN-2457

After discussing with @Amoursol and @QilongTang, we thought it would be useful to add a thumb control to expand the watch node to a desirable size. This thumb control to similar to the one, that is present in watch3D node.

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

Reviewers

@DynamoDS/dynamo

Amoursol · 2020-12-09T21:34:51Z

@reddyashish looking great as a first pass!

Can you please switch the location of the item count and resize box in these? So that the box is justified to the lower right, and that the count is justified to the bottom, but to the left hand side of the box?

mjkkirschner · 2020-12-09T21:40:35Z

@reddyashish - I think you'll have to consider opening a graph and how to serialize/deserialize the width and height now. This should probably be in the view data for the watch node.

reddyashish · 2020-12-09T21:53:34Z

Looks like this now.

QilongTang · 2020-12-10T05:54:41Z

@reddyashish - I think you'll have to consider opening a graph and how to serialize/deserialize the width and height now. This should probably be in the view data for the watch node.

Yes, I believe this is part of the AC as well.

src/DynamoCoreWpf/Views/Preview/WatchTree.xaml

QilongTang · 2020-12-15T21:30:55Z

src/DynamoCoreWpf/Views/Preview/WatchTree.xaml

-             d:DesignHeight="161"
-             d:DesignWidth="410"
+             Width="300"
+             Height="300"


Would this change affect default watch node size?

Yes, before this change we would have a maximum height of 300 but now we don't want to put any limitation on that.
So when we remove that, the height of the watch node would be dependent on the number of elements in the list and it would become much longer. So now we will have a default width and height same as the size of watch3d node and it can be expanded with the thumb icon.

hmm - I think it would be nice if the default stayed the same as it was - so that existing graphs do not change in appearance when opened.

So whats happening is, previously there was no height and width set at runtime for the watch node. Only the maximum height of 300 was set. So the height of the node would depend on the number of elements but once the max height is reached, the scroll bar appears. Now we don't have any max height set to the watch node and if no default height or width is set explicitly, then the watch node will be too long in the case where input has a lot of elements. To avoid it, I thought we could make it consistent with the watch3d node. What do you think?

QilongTang · 2020-12-15T21:47:48Z

src/DynamoCoreWpf/Views/Preview/WatchTree.xaml

    </UserControl.Resources>
-    <Grid>

+    <Grid Name ="inputGrid" MinHeight="100" MinWidth="100">


Would this change the current behavior?

No, this will only set the min height and width so that when resizing the node, the thumb icon doesn't get collapsed.

reddyashish · 2020-12-16T14:47:26Z

@mjkkirschner As discussed with @QilongTang, I will be opening up a new PR to address the deserialization of the watch node height and width because there seems to be some missing pieces and the team can have a separate discussion on that draft PR.

QilongTang · 2020-12-16T16:31:36Z

Can you check Regression:

DynamoCoreWpfTests.PreviewBubbleTests.PreviewBubble_HiddenDummyVerticalBoundaries

Thomas84 · 2021-01-04T16:48:12Z

It would be nice to have the same functionality at the node preview:

Amoursol · 2021-01-04T18:17:49Z

It would be nice to have the same functionality at the node preview:

Hello @Thomas84 - We have to be careful of the Node Preview as this state is often 'working' and/or left on and costs system resources, versus a Watch node is a user-driven action. We see them as two separable elements.

Is there a use case that you would prefer to use an expandable preview bubble over an expandable watch node?

Thomas84 · 2021-01-04T22:13:11Z

It would be nice to have the same functionality at the node preview:

Hello @Thomas84 - We have to be careful of the Node Preview as this state is often 'working' and/or left on and costs system resources, versus a Watch node is a user-driven action. We see them as two separable elements.

Is there a use case that you would prefer to use an expandable preview bubble over an expandable watch node?

Normally I use the node preview to explore a script or when i build a new script. I use watch nodes rarely, just if I want to shift the focus of someone to a specific result, to extract a dictionary item or to show the result in the dynamo player. I place all my nodes in a way, that the node preview bubble will not obstruct anything, if it will be expanded/pinned by someone (maximum standard height of the preview bubble is useful!). It is just time consuming to place extra watch nodes. In my opinion there should be an explorer mode, which expands all node preview bubbles at once and pin it until i exit the explorer mode. The height of the node preview is at the moment not the real problem, for me most often the width is the problem. And the best option would be, if the width adjust automatically (with a bigger standard width value - but it should not be unlimited large), so I don't have to use in most cases the horizontal scroll bar or the drag handle (of the new watch node). If there will be the new drag handle at the preview bubble as an addition, it would be a welcome functionality if I can expand the preview bubble to unlimited size.

And instead of watch nodes, I use Code Blocks with a variable, as description of the data. Thus if I need to, I can show the result in the node preview bubble. Especially if the script is very large it is a good method, to know what data is coming (without scrolling to another area of the script). In the last screenshot at the bottom, the width of the preview bubble can be expanded (there is a large maximum width of the preview bubble at the Code Block, thus a long variable name can expand the width (trick :P), and at the screenshot at the top of my post, the width is too small.

Amoursol · 2021-01-06T14:51:27Z

..., I use Code Blocks with a variable, as description of the data. Thus if I need to, I can show the result in the node preview bubble. Especially if the script is very large it is a good method, to know what data is coming (without scrolling to another area of the script). In the last screenshot at the bottom, the width of the preview bubble can be expanded (there is a large maximum width of the preview bubble at the Code Block, thus a long variable name can expand the width (trick :P), and at the screenshot at the top of my post, the width is too small.

@Thomas84 - Gotcha! We have noted this down for future works as we are starting to look at the UX of Dynamo again in a fresh pass. For now, this task will be limited to Watch Nodes as we try to keep our improvements small, but we will file a task into our backlog for the preview bubble state as our UX designers can take a look at it.

@reddyashish please make sure to file a Jira task for the 'Preview Bubble' with a [UX] prefix and ping me.

…e watch node. This was a better option to avoid issues with preview control.

reddyashish · 2021-01-21T15:30:15Z

The self-service job looks good. Only the excel tests are failing. https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/764/

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

QilongTang · 2021-01-22T00:50:18Z

src/Libraries/CoreNodeModelsWpf/NodeViewCustomizations/Watch.cs

            rootWatchViewModel = new WatchViewModel(dynamoViewModel.BackgroundPreviewViewModel.AddLabelForPath);

            // Fix the maximum width/height of watch node.
-            nodeView.PresentationGrid.MaxWidth = Configurations.MaxWatchNodeWidth;


Should we also obsolete these two properties? Seems the only references are in this file here which will be removed.

Or we can still set a much bigger maximum height and width, like half window size or something. This might be a UX question. FYI: @Amoursol If we choose to give user total control, then I prefer these two obsolete now and removed in Dynamo 3.0

I discussed this with UX-@Jingyi-Wen about if we should make a max size of watch node still. @Jingyi-Wen did not feel that we need. So let's obsolete MaxWatchNodeWidth and MaxWatchNodeHeight and move on.

@QilongTang I'm cool with what you and Jingyi came up with!

I think the "watch node" from Grasshopper can be a very good reference.
Imagine in Dynamo, after double click, we got a Panel, you can write number, string, or any code you want. If you don't write anything, it is a "watch node". With a default size that can display correctly for most of the time. For the special case, you can resize it very easily. Also, you can change the color of the panel.

QilongTang

LGTM with one question

Adding a view element to let users expand the watch node.

0227bc1

Change the location of the thumb element.

ca5293a

QilongTang reviewed Dec 15, 2020

View reviewed changes

src/DynamoCoreWpf/Views/Preview/WatchTree.xaml Outdated Show resolved Hide resolved

QilongTang reviewed Dec 15, 2020

View reviewed changes

Removing the BoolToVisibilityConverter refernece that is not being used

0a0e32b

reddyashish added 4 commits January 20, 2021 16:29

Hide the expansion thumb element in the preview content of the node

cd52400

Uncommiting previous commit and showing the thumb element only for th…

06a5adc

…e watch node. This was a better option to avoid issues with preview control.

Set the min height and min width dynamically for the watch node.

c141784

Remove unnecessary changes

46f9cd3

QilongTang reviewed Jan 21, 2021

View reviewed changes

src/DynamoCoreWpf/Views/Preview/WatchTree.xaml.cs Outdated Show resolved Hide resolved

Use const to set the size of the node.

0b19046

QilongTang reviewed Jan 22, 2021

View reviewed changes

QilongTang approved these changes Jan 22, 2021

View reviewed changes

reddyashish added 2 commits January 22, 2021 16:11

Obsoleting the properties that are not being used anymore.

cf3e8e7

Update Watch.cs

94fa493

reddyashish merged commit c90364b into DynamoDS:master Jan 23, 2021

Conversation

reddyashish commented Dec 9, 2020

Purpose

Declarations

Reviewers

Uh oh!

Amoursol commented Dec 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Dec 9, 2020

Uh oh!

reddyashish commented Dec 9, 2020

Uh oh!

QilongTang commented Dec 10, 2020

Uh oh!

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!

reddyashish Dec 16, 2020 • 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!

reddyashish commented Dec 16, 2020

Uh oh!

QilongTang commented Dec 16, 2020

Uh oh!

Thomas84 commented Jan 4, 2021

Uh oh!

Amoursol commented Jan 4, 2021

Uh oh!

Thomas84 commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amoursol commented Jan 6, 2021

Uh oh!

reddyashish commented Jan 21, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QilongTang Jan 22, 2021 • 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!

QilongTang left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Amoursol commented Dec 9, 2020 •

edited

Loading

reddyashish Dec 16, 2020 •

edited

Loading

Thomas84 commented Jan 4, 2021 •

edited

Loading

QilongTang Jan 22, 2021 •

edited

Loading