Dyn 5497 markdown not loading #13703

RobertGlobant20 · 2023-01-23T22:46:25Z

Purpose

Fixing bug about markdown files not loading for node packages documentation.
There was a code change that was preventing for loading md files for packages node documentation, so after this fix was done in a different pull request (#13693), now it is loading the md file but the images were not loading due that WebView2 was using virtual folder fixed to fallback_docs folder then in this fix we are allowing to create the virtual folder dynamic depending if is a dynamo node or package node.
For some nodes of the Orchid package is working there is another bug with some specific nodes (like Math.Addition).

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

Release Notes

Fixing bug about markdown files not loading for node packages documentation.

Reviewers

@QilongTang @reddyashish @dnenov

FYIs

Adding code for dynamically create virtual directories and load images based in if is a dynamo node or is a package node.

Fix done by Deyan

Reverting changes back due that Deyan Nenov already did the changes in a different pull request.

RobertGlobant20 · 2023-01-23T22:49:32Z

Screenshot showing the documentation with images for a package node.

mjkkirschner · 2023-01-23T22:56:14Z

src/DocumentationBrowserViewExtension/DocumentationBrowserViewModel.cs

@@ -132,6 +140,8 @@ public bool ShowBrowser

        internal UIElement DynamoView { get; set; }

+        internal NodeInfo CurrentNodeInfo { get; set; }


I don't think carrying around this state is ideal - at the minimum it should have some comments...

I'm not sure if it's possible, but it seems simpler to keep track of where this data is used if it's passed in an event or binding (or something similar) to the View.

Just a thought.

@mjkkirschner removed the NodeInfo class and I just added a new internal property, check the next commit:
fdd337c

mjkkirschner · 2023-01-23T22:56:32Z

Can we write a test for this regression?

mjkkirschner · 2023-01-23T22:56:59Z

what does this mean For some nodes of the Orchid package is working there is another bug with some specific nodes (like Math.Addition). Is it tracked and in jira?

I've removed the NodeInfo class instead I just added the Name property and that one will be used for creating the virtual directory.

It was sending an exception when the md file was not found and the Link is null so I added a try catch (Link != null doesn't work) so the system won't crash and will show the default md file (when documentation is not found).

RobertGlobant20 · 2023-01-24T19:43:35Z

what does this mean For some nodes of the Orchid package is working there is another bug with some specific nodes (like Math.Addition). Is it tracked and in jira?

@mjkkirschner
I was debugging the code with @reddyashish some days ago and we found that there is something wrong with the Orchid assemblies because the info was not read for several Orchid nodes we can see that the packageInfo is null.

The next Jira task was just created for that purpose.
https://jira.autodesk.com/browse/DYN-5539

RobertGlobant20 · 2023-01-24T19:48:42Z

Can we write a test for this regression?

@mjkkirschner please confirm that this a list of steps for the test expected for validating this functionality.
With this test we will be only validating that the md file expected will be loaded (no validation on images loaded).

Open Dynamo
Install a package with documentation (note that most of the packages currently doesn't have documentation even the Orchid package in production doesn't have documentation).
Open the DocumentationBrowser
Validate that the md in the Package/doc folder match with the one loaded in the DocumentationBrowser.

mjkkirschner · 2023-01-24T23:08:14Z

Can we write a test for this regression?

@mjkkirschner please confirm that this a list of steps for the test expected for validating this functionality. With this test we will be only validating that the md file expected will be loaded (no validation on images loaded).

Open Dynamo

Install a package with documentation (note that most of the packages currently doesn't have documentation even the Orchid package in production doesn't have documentation).

Open the DocumentationBrowser

Validate that the md in the Package/doc folder match with the one loaded in the DocumentationBrowser.

what about mocking most of that - for example, just set the view model data with some fake data that should come from a package, open the docs browser, and then check that the virtual folder is setup correctly using that data - we just want to ensure it does not regress later.

filipeotero · 2023-01-26T14:45:50Z

src/DocumentationBrowserViewExtension/DocumentationBrowserViewModel.cs

+        /// <summary>
+        /// Package Name
+        /// </summary>
+        internal string Name


Maybe a more intuitive name would be PackageName... What do you think?

Or graph name?

@filipeotero yes, I think is a better idea to use PackageName, then updated in the next commit: 0e3c928

The Name property was named to packageName A new test was added that is validating that the virtual folder will be created in the packages/package/doc folder so when the documentation for a node is loaded the images are loaded successfully. Also the code for setting the VirtualFolderPath property was moved before WebView2.EnsureCoreWebView2Async() call because otherwise we were not reaching the code.

RobertGlobant20 · 2023-01-26T22:39:00Z

Can we write a test for this regression?

@mjkkirschner please confirm that this a list of steps for the test expected for validating this functionality. With this test we will be only validating that the md file expected will be loaded (no validation on images loaded).

Open Dynamo

Install a package with documentation (note that most of the packages currently doesn't have documentation even the Orchid package in production doesn't have documentation).

Open the DocumentationBrowser

Validate that the md in the Package/doc folder match with the one loaded in the DocumentationBrowser.

what about mocking most of that - for example, just set the view model data with some fake data that should come from a package, open the docs browser, and then check that the virtual folder is setup correctly using that data - we just want to ensure it does not regress later.

@mjkkirschner I've added a unit test for validating that the WebView2 Virtual Directory is created in the right place (so images will be loaded correctly) when displaying documentation for package nodes.

When implementing the unit test I found that the call to await WebView2.EnsureCoreWebView2Async() was returning the execution to the unit test (instead of executing the next code), then I had to move the Virtual Folder code creation before EnsureCoreWebView2Async() call also I've added a property for VirtualDirectory so it can be checked in the unit test., please let me know is the test should be enough
Thanks

Here is the commit with the changes:
0e3c928

pinzart90 · 2023-01-27T18:31:40Z

src/DocumentationBrowserViewExtension/DocumentationBrowserView.xaml.cs

+            {
+                if (viewModel.Link != null && !string.IsNullOrEmpty(viewModel.PackageName))
+                {
+                    VirtualFolderPath = HttpUtility.UrlDecode(viewModel.Link.AbsolutePath).Replace(viewModel.PackageName + ".md", "");


If this is about getting the package folder, might be better to call Path.GetDirectoryName(HttpUtility.UrlDecode(viewModel.Link.AbsolutePath)) instead of Replace(viewModel.PackageName + ".md", "")

thanks @pinzart90 I've done the change in the next commit: ad5209f

Fixed change for not using hardcoded ".md" when creating the virtual directory

There are some Orchid nodes in the Math category that are receiving a number of parameters variable and seems that node documentation (md files) is not supported for this kind of nodes (DSVarArgFunction), then a fix was applied for loading the package info and md files for this kind of nodes. Also I fixed a test that was replacing slashes but in some cases the slashes were already inverted.

mjkkirschner · 2023-01-31T18:52:17Z

src/DocumentationBrowserViewExtension/DocumentationBrowserView.xaml.cs

+                else
+                    VirtualFolderPath = FallbackDirectoryName;
+            }
+            catch (Exception ex)


what exception do you think will be caught here?

@mjkkirschner I found a case that viewModel.Link = "{}" so it was crashing when trying the next line viewModel.Link.AbsolutePath so I added this try catch because I didn't found any other way to validate the viewModel.Link content.
With the fix added related to DSVarArgFunction now I expect that viewModel.Link always will contain something valid but just in case I think we should keep this code or if you have any other idea in how to validate viewModel.Link = "{}" please let me know (Link is Uri type, Uri.IsWellFormedUriString returns true).

I think it's a good practice to log something including the exception if possible to the dynamo console - if some unanticipated exception starts showing up here atleast we'll know.

@mjkkirschner I've added some code for logging the exception in the dynamo console, see the next commit: 5980d15

mjkkirschner · 2023-01-31T18:54:37Z

src/DocumentationBrowserViewExtension/DocumentationBrowserViewModel.cs

+        /// <summary>
+        /// Package Name
+        /// </summary>
+        internal string PackageName


it still feels pretty odd to me that the DocumentationBrowserViewModel has a single piece of state like this that it has to hold on to.

If I am understanding correctly, can we at least name it something like:
CurrentPackageName or something like that and add a comment describing when it will be null and when it will be valid?

@mjkkirschner notice that the dependency on PackageName was added in the next PR:
#13414
Renamed to CurrentPackageName in the next commit: f00db5b

mjkkirschner · 2023-01-31T18:57:54Z

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

@@ -2120,9 +2120,9 @@ internal AssemblyName GetNameOfAssemblyReferencedByNode(NodeModel node)
        {
            AssemblyName assemblyName = null;
            // Get zerotouch assembly
-            if (node is DSFunction)
+            if (node is DSFunction || node is DSVarArgFunction)


~~this is unnecessary, is will check that the instance inherits from the type in question.~~

I am incorrect, DSVarArgFunction inherits from DSFunctionBase not DSFunction - apologies for the confusion.

mjkkirschner · 2023-01-31T19:01:14Z

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

            {
-                var descriptor = (node as DSFunction).Controller.Definition;
+                var descriptor = node is DSFunction function ? function.Controller.Definition :  (node as DSVarArgFunction).Controller.Definition;


I think you can get rid of all these cases and the if statement above - just check if the type is DSFunctionBase - it has a Controller.Definition property you can access and you don't need to check any types, it's safer an faster - unless you need special behavior for each type.

Checking if is DSFunctionBase type I've simplified the code in the next way:

Please let me know if it sounds good to you. Thanks
commit: f00db5b

mjkkirschner · 2023-01-31T19:02:34Z

test/DynamoCoreWpfTests/ViewExtensions/DocumentationBrowserViewExtensionTests.cs

+
+            docBrowserviewExtension.HandleRequestOpenDocumentationLink(nodeAnnotationEventArgs);
+            var htmlContent = GetSidebarDocsBrowserContents();
+            htmlContent = htmlContent.Replace(@"%5C", "/");


please add a comment

Added comment, please check the next commit: f00db5b

mjkkirschner

please see inline comments

Renaming PackageName to CurrentPackageName. Rewriting code for loading documentation correctly for DSVarArgFunction nodes Adding comments to the test

mjkkirschner · 2023-02-02T18:25:40Z

thanks @RobertGlobant20 !

Adding log in case we reach the exception when the Uri is empty.

zeusongit · 2023-02-02T22:18:31Z

Thanks for addressing the review comments @RobertGlobant20 I will merge the PR as soon as the self serve job completes

RobertGlobant20 · 2023-02-03T00:09:54Z

Thanks for addressing the review comments @RobertGlobant20 I will merge the PR as soon as the self serve job completes
@zeusongit
Just to comment that I keep receiving this emails after I push any commit in this PR (but all the checks seems that are passing):

mjkkirschner · 2023-02-06T17:34:35Z

@RobertGlobant20 seems there is a conflicted file.

RobertGlobant20 added 4 commits January 19, 2023 16:57

DYN-5497-Markdown-NotLoading

c0d6851

Adding code for dynamically create virtual directories and load images based in if is a dynamo node or is a package node.

DYN-5497-Markdown-NotLoading

5b1b00d

Fix done by Deyan

Reverting

b4645bc

Reverting changes back due that Deyan Nenov already did the changes in a different pull request.

Merge branch 'master' into DYN-5497-Markdown-NotLoading

8548e6f

RobertGlobant20 requested review from QilongTang and reddyashish January 23, 2023 22:46

mjkkirschner reviewed Jan 23, 2023

View reviewed changes

RobertGlobant20 added 2 commits January 24, 2023 13:00

DYN-5497-Markdown-NotLoading CodeReview 1

fdd337c

I've removed the NodeInfo class instead I just added the Name property and that one will be used for creating the virtual directory.

DYN-5497-Markdown-NotLoading Fix

0153528

It was sending an exception when the md file was not found and the Link is null so I added a try catch (Link != null doesn't work) so the system won't crash and will show the default md file (when documentation is not found).

filipeotero reviewed Jan 26, 2023

View reviewed changes

pinzart90 reviewed Jan 27, 2023

View reviewed changes

RobertGlobant20 added 2 commits January 27, 2023 14:06

DYN-5497-Markdown-NotLoading CodeReview3

ad5209f

Fixed change for not using hardcoded ".md" when creating the virtual directory

zeusongit requested a review from mjkkirschner January 31, 2023 15:47

mjkkirschner reviewed Jan 31, 2023

View reviewed changes

mjkkirschner requested changes Jan 31, 2023

View reviewed changes

DYN-5497-Markdown-NotLoading CodeReview 4

f00db5b

Renaming PackageName to CurrentPackageName. Rewriting code for loading documentation correctly for DSVarArgFunction nodes Adding comments to the test

zeusongit requested a review from mjkkirschner February 2, 2023 17:59

mjkkirschner approved these changes Feb 2, 2023

View reviewed changes

DYN-5497-Markdown-NotLoading CodeReview 4

5980d15

Adding log in case we reach the exception when the Uri is empty.

Merge branch 'master' into DYN-5497-Markdown-NotLoading

de57207

zeusongit merged commit cb05ea9 into DynamoDS:master Feb 6, 2023

avidit mentioned this pull request Feb 8, 2023

Markdown helping files fail in dynamo 2.16 #13648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dyn 5497 markdown not loading #13703

Dyn 5497 markdown not loading #13703

RobertGlobant20 commented Jan 23, 2023

RobertGlobant20 commented Jan 23, 2023

mjkkirschner Jan 23, 2023

RobertGlobant20 Jan 24, 2023

mjkkirschner commented Jan 23, 2023

mjkkirschner commented Jan 23, 2023

RobertGlobant20 commented Jan 24, 2023

RobertGlobant20 commented Jan 24, 2023

mjkkirschner commented Jan 24, 2023 •

edited

Loading

filipeotero Jan 26, 2023

zeusongit Jan 26, 2023

RobertGlobant20 Jan 26, 2023

RobertGlobant20 commented Jan 26, 2023

pinzart90 Jan 27, 2023

RobertGlobant20 Jan 27, 2023

mjkkirschner Jan 31, 2023

RobertGlobant20 Feb 1, 2023

mjkkirschner Feb 2, 2023

RobertGlobant20 Feb 2, 2023

mjkkirschner Jan 31, 2023

RobertGlobant20 Feb 1, 2023

mjkkirschner Jan 31, 2023 •

edited

Loading

mjkkirschner Jan 31, 2023

mjkkirschner Jan 31, 2023

RobertGlobant20 Feb 1, 2023

mjkkirschner Jan 31, 2023

RobertGlobant20 Feb 1, 2023

mjkkirschner left a comment

mjkkirschner commented Feb 2, 2023

zeusongit commented Feb 2, 2023

RobertGlobant20 commented Feb 3, 2023

mjkkirschner commented Feb 6, 2023

		@@ -132,6 +140,8 @@ public bool ShowBrowser

		internal UIElement DynamoView { get; set; }

		internal NodeInfo CurrentNodeInfo { get; set; }

Dyn 5497 markdown not loading #13703

Dyn 5497 markdown not loading #13703

Conversation

RobertGlobant20 commented Jan 23, 2023

Purpose

Declarations

Release Notes

Reviewers

FYIs

RobertGlobant20 commented Jan 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Jan 23, 2023

mjkkirschner commented Jan 23, 2023

RobertGlobant20 commented Jan 24, 2023

RobertGlobant20 commented Jan 24, 2023

mjkkirschner commented Jan 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobertGlobant20 commented Jan 26, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner Jan 31, 2023 • 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 left a comment

Choose a reason for hiding this comment

mjkkirschner commented Feb 2, 2023

zeusongit commented Feb 2, 2023

RobertGlobant20 commented Feb 3, 2023

mjkkirschner commented Feb 6, 2023

mjkkirschner commented Jan 24, 2023 •

edited

Loading

mjkkirschner Jan 31, 2023 •

edited

Loading