Lazy Load Node Context Menu #12074

OliverEGreen · 2021-09-28T10:56:47Z

Purpose

This PR improves the way in which all nodes' context menus are loaded, so that they are constructed when required by the user, and disposed immediately after closing. This lazy-loaded approach is designed to improve graph opening speed and reduce Dynamo's memory usage.

The new approach is handled by a new static class: NodeContextMenuBuilder.cs. This flexible class can be adapted in the future, should new MenuItems be required.

Any MenuItems created during a node's NodeViewCustomization process are stashed per-node, and dynamically injected into the ContextMenu at a consistent location when the menu is built.

Please Note:

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.

Reviewers

@saintentropy
@mjkkirschner
@QilongTang

FYIs

@Amoursol

…_menu

Amoursol · 2021-09-28T14:35:53Z

@OliverEGreen is it possible to include the benchmarking numbers you have in the description here? Before and After 😄

OliverEGreen · 2021-09-28T14:37:15Z

@OliverEGreen is it possible to include the benchmarking numbers you have in the description here? Before and After 😄

src/DynamoCoreWpf/Properties/Resources.en-US.resx

mjkkirschner · 2021-09-28T20:12:28Z

src/DynamoCoreWpf/Views/Core/NodeView.xaml

+            <ContextMenu Name="MainContextMenu"
+                         x:FieldModifier="public"
+                         Closed="MainContextMenu_OnClosed"
+                         Style="{StaticResource NodeContextMenuStyle}">


I have two questions here:

I think including this style for menuItems in contextMenu.Resources can be optimized - can it also be moved to the sharedResource dictionary and only applied in the nodeContextMenuBuilder and applied there?

Why does this style include references to the dismissed badge, is that in the context menu at certain times?

As far as I can tell this is a dumb WPF thing:

We can style the ContextMenu from a ResourceDictionary fine, but styling the items is a little harder. ContextMenus can contain both MenuItems and Separators (we have both of these in the mockup).
Telling the ContextMenu that its ItemContainerStyle should be a style specifically targeting MenuItems won't work and this results in an exception. The Separators don't like being given a style that's not for them.

Therefore, I put both the style for Separators and MenuItems within a local resources section, so they get picked up as is appropriate and with no exceptions.

Yes it is! It's there at all times.

Am investigating whether we can use an ItemContainerStyleSelector to get past # 2.

I've had no luck with implementing the ItemContainerStyleSelector. It seems like a sensible approach, but I've not seen it being used inside of a ResourceDictionary.

mjkkirschner · 2021-09-28T20:13:22Z

src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs

+        /// <summary>
+        /// A dictionary of MenuItems which are added during certain nodes' NodeViewCustomization process.
+        /// </summary>
+        private Dictionary<string, object> NodeViewCustomizationMenuItems { get; } = new Dictionary<string, object>();


why are the values of type object and not MenuItem?

@OliverEGreen a few other issues with this...

Do you need the order of keys / values in this dictionary to stay stable? If that is the case you need to use an ordered dictionary like:
https://docs.microsoft.com/en-us/dotnet/api/system.collections.specialized.ordereddictionary?view=netframework-4.8

also I was curious why you did not just use a hashset - it looks like MenuItem's base class ItemControl implements Equals
https://referencesource.microsoft.com/#PresentationFramework/src/Framework/System/windows/Controls/ItemsControl.cs,3808

though - if you need order, go with the ordered dictionary, hashset does not preserve insertion order either.

"why are the values of type object and not MenuItem?"

As above, they can be both Separators and MenuItems.

I'd say order stability does matter yeah, will switch out to an OrderedDictionary!

I considered HashSet, but I wasn't sure about using Equals to compare one MenuItem to another - would that work? Hence, I'm using the 'Header' values to indicate uniqueness.

src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs

mjkkirschner · 2021-09-28T20:21:00Z

src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs

+
+        private void MainContextMenu_OnClosed(object sender, RoutedEventArgs e)
+        {
+            grid.ContextMenu.Items.Clear();


if you've already gone this far, can't we get rid of the ContextMenu as well, why instantiate one contextMenu per nodeView when we only need 1 at a time in the entire graph.

⚠️

If it's possible to style this centrally then it should be possible. However, we're relying on a lot of locally-defined XAML styles currently.

mjkkirschner · 2021-09-28T20:21:43Z

src/DynamoCoreWpf/Utilities/NodeContextMenuBuilder.cs

+            Properties.Resources.NodeContextMenuShowLabels,
+            Properties.Resources.NodeContextMenuRenameNode,
+            Properties.Resources.ContextMenuLacing,
+            //Wpf.Properties.Resources.NodeInformationalStateDismissedAlerts,


This is intentionally commented out, to be tied together with the node info states once that work has been merged.

src/DynamoCoreWpf/Utilities/NodeContextMenuBuilder.cs

mjkkirschner · 2021-09-28T20:22:48Z

src/DynamoCoreWpf/Utilities/NodeContextMenuBuilder.cs

+
+        private static void AddInjectedNodeViewCustomizationMenuItems(Dictionary<string, object> nodeViewCustomizationMenuItems)
+        {
+            foreach (KeyValuePair<string, object> keyValuePair in nodeViewCustomizationMenuItems)


var doesn't let me grab the Key or Value, which is needed here.
I've swapped for:

foreach (DictionaryEntry keyValuePair in nodeViewCustomizationMenuItems) { ContextMenu.Items.Add(keyValuePair.Value); }

var doesn't change the type, the type of item being iterated over is already inferred from the type of nodeViewCustomizationMenuItems - it's a KeyValuePair no matter what.

I don't think it will let me access the Value property if it's 'var' though:

~~try compiling that - I'm guessing resharper is just getting confused- for example:~~

Dynamo/test/DynamoCoreTests/SerializationTests.cs

Line 238 in b17e984

foreach (var kvp in a.InportCountMap)

~~and an example:~~
https://dotnetfiddle.net/nekXuD

ah my mistake, OrderedDictionary does not return KeyValuePairs when iterating over them - it returns DictionaryEntries https://docs.microsoft.com/en-us/dotnet/api/system.collections.dictionaryentry?view=net-5.0

instead of var examples on MSDN show using a foreach look like this:

foreach (DictionaryEntry de in myOrderedDictionary) { //... }

mjkkirschner · 2021-09-28T20:25:30Z

src/DynamoCoreWpf/Utilities/NodeContextMenuBuilder.cs

+    /// <summary>
+    /// A static class containing methods for dynamically building a Node's Context Menu.
+    /// </summary>
+    internal static class NodeContextMenuBuilder


please use nameof where you can, when these strings change, everything will break.

There is a lot of work in here to construct the views and bindings with code - thats fine - but I think the xaml could also have been reused by extracting it to a resource and instantiating it - perhaps that would not play well with the dynamic menu items - but I think if we can avoid re-writing all of our xaml we should. Consider if this is useful: https://docs.microsoft.com/en-us/windows/apps/design/style/xaml-resource-dictionary#look-up-resources-in-code

I've inserted nameof everywhere. The only hard-coded strings in the class are now equivalents of those that were previously hard-coded in XAML.

I'm afraid I don't understand this approach. Please feel free to reach out on Slack/Zoom if you want to go over this.

mjkkirschner · 2021-09-28T20:25:56Z

This PR should have tests for the new builder class if you decide to go that way.

OliverEGreen · 2021-09-29T15:58:58Z

This PR should have tests for the new builder class if you decide to go that way.

Added 5 tests to cover the new functionality.

QilongTang · 2021-10-01T13:53:07Z

The regressions reported for this PR should be unrelated

Test Result (3 failures / +3)
DynamoCoreWpfTests.PackageManagerUITests.CannotCreateDuplicatePackageSearchDialogs
DynamoCoreWpfTests.PackageManagerUITests.CanOpenPackageSearchDialogAndWindowIsOwned
DynamoCoreWpfTests.PackageManagerUITests.PackageSearchDialogClosesWithDynamo

OliverEGreen · 2021-10-01T14:42:40Z

src/DynamoCoreWpf/Utilities/NodeContextMenuBuilder.cs

+        internal static void AddInjectedNodeViewCustomizationMenuItems(OrderedDictionary nodeViewCustomizationMenuItems)
+        {
+            foreach (DictionaryEntry keyValuePair in nodeViewCustomizationMenuItems)
+            {
+                ContextMenu.Items.Add(keyValuePair.Value);
+            }
+        }


@mjkkirschner I think in the latest version of the files there's a DictionaryEntry in this foreach loop. Not entirely sure why the PR isn't showing this on the main page! Must be outdated.

mjkkirschner · 2021-10-06T20:20:19Z

@OliverEGreen your PR still changes ContextMenuStyle to NodeContextMenuStyle causing test failures because that style is used in other places. I believe that change was just reverted by @QilongTang - is there a reason you are bringing it back in?

mjkkirschner

please see above comment regarding: ContextMenuStyle

OliverEGreen · 2021-10-06T22:06:11Z

@OliverEGreen your PR still changes ContextMenuStyle to NodeContextMenuStyle causing test failures because that style is used in other places. I believe that change was just reverted by @QilongTang - is there a reason you are bringing it back in?

Ah I hadn't spotted it was being used elsewhere. No reason to change the name then! Can undo this on Monday. Thanks for raising!

QilongTang · 2021-10-07T03:02:32Z

The following regression reported but I think those were fixed on master in a previous PR. Seems you noticed the styling change already

DynamoCoreWpfTests.PackageManagerUITests.CannotCreateDuplicatePackageSearchDialogs
DynamoCoreWpfTests.PackageManagerUITests.CanOpenPackageSearchDialogAndWindowIsOwned
DynamoCoreWpfTests.PackageManagerUITests.PackageSearchDialogClosesWithDynamo

src/DynamoCoreWpf/UI/Themes/Modern/DynamoModern.xaml

…_menu

OliverEGreen added 17 commits September 23, 2021 13:48

Lazy-loads node context menu

99048de

Centralises ContextMenu style

ecfa38b

Labels

a433e4c

Allows for NodeViewCustomizations to add their own MenuItems

c6378e2

Disables dismissed alerts for now

a4fbed3

Comments methods and removes redundant methods.

d6b82e1

Undoes some extra work

c4c9f6f

Updates

1cc38bb

Undoes irrelevant changes

74092b5

Disposes MenuItems whenever the node ContextMenu is closed.

7f5c04b

Update NodeView.xaml.cs

3d38de6

Tidies up Node Context Menu construction code.

4ab00a2

Update NodeView.xaml.cs

a43ec02

Creates static NodeContextMenuBuilder class.

708123d

Update Watch.cs

c08c218

Merge remote-tracking branch 'upstream/master' into node_lazy_context…

935ed99

…_menu

Merge branch 'master' into node_lazy_context_menu

e17c4fb

mjkkirschner reviewed Sep 28, 2021

View reviewed changes

src/DynamoCoreWpf/Properties/Resources.en-US.resx Outdated Show resolved Hide resolved

mjkkirschner reviewed Sep 28, 2021

View reviewed changes

src/DynamoCoreWpf/Views/Core/NodeView.xaml.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Sep 28, 2021

View reviewed changes

src/DynamoCoreWpf/Utilities/NodeContextMenuBuilder.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Sep 28, 2021

View reviewed changes

src/DynamoCoreWpf/Utilities/NodeContextMenuBuilder.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Sep 28, 2021

View reviewed changes

OliverEGreen added 3 commits September 29, 2021 11:28

Responds to comments

5604732

Adds tests

e9e3333

Update NodeContextMenuBuilderTests.cs

3765a04

Merge branch 'DynamoDS:master' into node_lazy_context_menu

6085dcb

QilongTang requested review from zeusongit and reddyashish October 1, 2021 13:59

OliverEGreen commented Oct 1, 2021

View reviewed changes

Merge branch 'master' into node_lazy_context_menu

30d9cd0

reddyashish approved these changes Oct 6, 2021

View reviewed changes

mjkkirschner requested changes Oct 6, 2021

View reviewed changes

QilongTang reviewed Oct 7, 2021

View reviewed changes

src/DynamoCoreWpf/UI/Themes/Modern/DynamoModern.xaml Outdated Show resolved Hide resolved

OliverEGreen added 2 commits October 11, 2021 13:54

Merge remote-tracking branch 'upstream/master' into node_lazy_context…

aaf8b2b

…_menu

Reverts style name

b46e7bf

QilongTang approved these changes Oct 12, 2021

View reviewed changes

QilongTang merged commit 2212221 into DynamoDS:master Oct 12, 2021

OliverEGreen deleted the node_lazy_context_menu branch November 9, 2021 09:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lazy Load Node Context Menu #12074

Lazy Load Node Context Menu #12074

OliverEGreen commented Sep 28, 2021

Amoursol commented Sep 28, 2021

OliverEGreen commented Sep 28, 2021

mjkkirschner Sep 28, 2021

OliverEGreen Sep 29, 2021

OliverEGreen Sep 29, 2021 •

edited

Loading

OliverEGreen Sep 29, 2021

mjkkirschner Sep 28, 2021

mjkkirschner Sep 28, 2021

OliverEGreen Sep 29, 2021

OliverEGreen Sep 29, 2021

mjkkirschner Sep 28, 2021 •

edited

Loading

OliverEGreen Sep 29, 2021

mjkkirschner Sep 28, 2021

OliverEGreen Sep 29, 2021

mjkkirschner Sep 28, 2021

OliverEGreen Sep 29, 2021

mjkkirschner Sep 29, 2021 •

edited

Loading

OliverEGreen Sep 29, 2021

mjkkirschner Oct 1, 2021 •

edited

Loading

mjkkirschner Sep 28, 2021 •

edited

Loading

OliverEGreen Sep 29, 2021

mjkkirschner commented Sep 28, 2021

OliverEGreen commented Sep 29, 2021

QilongTang commented Oct 1, 2021

OliverEGreen Oct 1, 2021

mjkkirschner commented Oct 6, 2021

mjkkirschner left a comment

OliverEGreen commented Oct 6, 2021

QilongTang commented Oct 7, 2021 •

edited

Loading

Lazy Load Node Context Menu #12074

Lazy Load Node Context Menu #12074

Conversation

OliverEGreen commented Sep 28, 2021

Purpose

Declarations

Reviewers

FYIs

Amoursol commented Sep 28, 2021

OliverEGreen commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverEGreen Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Oct 1, 2021 • edited Loading

Choose a reason for hiding this comment

mjkkirschner Sep 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Sep 28, 2021

OliverEGreen commented Sep 29, 2021

QilongTang commented Oct 1, 2021

Choose a reason for hiding this comment

mjkkirschner commented Oct 6, 2021

mjkkirschner left a comment

Choose a reason for hiding this comment

OliverEGreen commented Oct 6, 2021

QilongTang commented Oct 7, 2021 • edited Loading

OliverEGreen Sep 29, 2021 •

edited

Loading

mjkkirschner Sep 28, 2021 •

edited

Loading

mjkkirschner Sep 29, 2021 •

edited

Loading

mjkkirschner Oct 1, 2021 •

edited

Loading

mjkkirschner Sep 28, 2021 •

edited

Loading

QilongTang commented Oct 7, 2021 •

edited

Loading