Node annotations in DocumentationBrowser #11153
Conversation
…nudsen/Dynamo into Node-annotations-doc-browser
| /// <returns>Returns true if any tags was removed from the content</returns> | ||
| internal static bool RemoveScriptTagsFromString(ref string content) | ||
| { | ||
| if (Regex.IsMatch(content, SCRIPT_TAG_REGEX, RegexOptions.IgnoreCase)) |
There was a problem hiding this comment.
Script tags aren't the only risk we need to sanitize, and we don't want to be doing our own HTML parsing -- there are a lot of edge cases. Consider a library like https://github.com/mganss/HtmlSanitizer instead.
There was a problem hiding this comment.
There was a problem hiding this comment.
changed this to use HtmlSanitizer it seems like this library has a lot of configuration options, my knowledge on what might be considered dangerous in this is limited, so for now im just using the default settings.
There was a problem hiding this comment.
this brings in a bunch of dependencies - @QilongTang @mmisol any thoughts?
There was a problem hiding this comment.
hmm actually I would rather not bring in more dependencies, is this a must have for locally hosted static web content? @Dewb
There was a problem hiding this comment.
@SHKnudsen - could you look into potential html sanitization options that have fewer dependencies?
There was a problem hiding this comment.
@mjkkirschner i looked into this and it seems like there arent a lot of good options, here is what i found so far:
AntiXss - looks like this hasent been update since 2014, also read a few negative articles about it. (https://www.nuget.org/packages/AntiXSS/)
HtmlRuleSanitizer - This seems to be the most used option after HtmlSanitizer, however this still has dependecies on an Html Parser, in this case that is HtmlAgilityPack. So this would probably bring in about the same amount of dependencies as HtmlSanitizer
HtmlSanitizer - As @Dewb suggested, this looks to be the most commonly used library. It depends on AngelSharp which also looks like its the most commonly use Html parser.
In addition to these three the only real option i found was to build our own sanitizer, however this would probably still bring in a dependency on a HtmlParser.
I dont have a lot of experience on this area tbh, so not entirely sure what we actually need the sanitation to do except for disabling <script> tags.
There was a problem hiding this comment.
thanks for looking into this @SHKnudsen - it looks like in the latest releases of HtmlSanitizer the system.encoding.codepages dependency was removed - perhaps we could use that newest release?
There was a problem hiding this comment.
| /// <summary> | ||
| /// Creates a lookup for node annotation markdown files associated with a package. | ||
| /// Also creates file watchers for the package node annotation paths so new files can be hot reloaded in Dynamo. | ||
| /// </summary> |
There was a problem hiding this comment.
Could these capabilities be moved to the DocumentationBrowserViewExtension? This is all triggered from the Package Manager's OnPackageLoaded handler; the doc browser extension could subscribe to that event as well. Also, this looks like we're monitoring all documentation for all packages at all times -- that seems excessive. We want to watch the minimum necessary to make hot reload work. It's okay to defer a lot of this work until the user requests help for a node.
There was a problem hiding this comment.
@Dewb we are watching all packages doc folder so we know if any documentation is added or removed, but we are only watching the current open documentation file for changes. I cant see the way to defer that to when the user press help, or am i missing something?
| <meta charset="UTF-8" /> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <style> | ||
| /* PrismJS 1.21.0 |
There was a problem hiding this comment.
was this css extracted from prisim.js?
src/DocumentationBrowserViewExtension/Docs/NodeAnnotationNotFound.md
Outdated
Show resolved
Hide resolved
src/DocumentationBrowserViewExtension/DocumentationBrowserViewExtension.cs
Show resolved
Hide resolved
src/DocumentationBrowserViewExtension/DocumentationBrowserViewExtension.cs
Show resolved
Hide resolved
| /// Creates a lookup for node annotation markdown files associated with a package. | ||
| /// Also creates file watchers for the package node annotation paths so new files can be hot reloaded in Dynamo. | ||
| /// </summary> | ||
| public class PackageDocumentationManager |
There was a problem hiding this comment.
curious why you did not use a static class?
There was a problem hiding this comment.
hm, yea good question, i guess i had other ideas at the start. Could certainly make this into a static class instead if you prefer that?
|
Hi @mjkkirschner got a bit caught up today but just pushed a fix for the failing tests and a few new tests that checks the content of node documentation. If there are some specific tests cases that will help you guys move forward, let me know and i will have a look at it Tuesday. |
|
FYI @sm6srw
|
|
Hi @SHKnudsen - we are going to look at isolating the markdown and html conversion / sanitization as a separate process, and potentially make commits to this branch, or another based on it. Have you had time to:
|
|
Hi @mjkkirschner, Didnt know there was a failing test, will have a look now. |
|
@SHKnudsen Sorry, I pushed to the wrong branch. I will revert my changes. They where supposed to go into my fork! |
This reverts commit 71509da.
This reverts commit bbefc36.
* add doc folder to PackageDirectoryBuilder * update tests * comment update
… way to not add any new 3rd party dependencies to Dynamo (#11) * Revert "Revert "Add MD2HTML Tool"" This reverts commit 99b42f2. * Revert "Revert "First pass - singleton version"" This reverts commit f655c39. * Implement HTML Sanitizer * Remove unneeded dependecies from the doc extension * Cleanup naming * More naming cleanups * Rename files * Rename folder * Add comments * Use literal strings * Hardening wrapper class * Simplify API * Make wrapper API internal * Add Tools project to Mono solution * Fix directory name (case) * Add missing internal * Use IDispose pattern * Fix literal string * Fix cross platform issues * Use CS.props file * Undo change to AssemblySharedInfo.cs * Use shared assembly info * Add more docs * Add ndesk support and implement help option * Rename class * Add missing change * Remove the use of ref * Fix typo * Fix toolpath and use nameof() * Improve error handling * Move strings to resources * Enhance error messages
* Fix sorting for number keys in List.SortByKey (DynamoDS#11241) * fix sorting for number keys in List.SortByKey * add unit test * Crash when accessing static hidden methods from zero touch nodes (DynamoDS#11238) * handle hidden static methods from zero touch nodes * add images (DynamoDS#11243) * Revert "Adding copy of missing file from LibG nuget package (DynamoDS#11218)" (DynamoDS#11254) This reverts commit a71b5c8. * Revert "Revert "Adding copy of missing file from LibG nuget package (DynamoDS#11218)" (DynamoDS#11254)" (DynamoDS#11255) This reverts commit 257ac14. * Undock/redock extension tabs to windows (DynamoDS#11246) * Somewhat working version * Pretty much working version * Finishing touches and test * Cleanup * Prevent both window and tab * Improve method name and comment * Forgot to include this in last commit * add analytics to extensions load event (DynamoDS#11258) * Revert Project Reference Changes (DynamoDS#11256) * Initial Commit * fix mono build * Update to latest libG nuget version * Cleanup entries VS Studio failed to update * Fix Mono build * Add the private flag back (modified by VS Studio) * Regressions * Add back missing resource * Change DefaultValue setter to public (DynamoDS#11251) * update load asm binaries from registry logic (DynamoDS#11261) * update load asm binaries from registry * Add coverage for docking (DynamoDS#11262) * Adding a public API on the View extension that gets triggered when the view extension is closed. (DynamoDS#11260) * Adding a public API on the View extension that gets triggered when the view extension is closed. * Update variable name * changing the name to ViewExtensionBaseClass * Addressing some comments. * Adding unit test. * Changing the name to Closed() * Some more comments. * Adding an additional test * Adding checks for the Menu items before setting its value. * Update Style (DynamoDS#11263) * CustomNodes that replicate do not access trace correctly. (DynamoDS#11244) * add failing test * this works, but seems like cheating. more tests * new tests, one failing, needs discussion * fix test - still fails but as expected * tests all pass with this change - but looking for better alternatives * add test use new field - cant find a better way currently add comments * revert internal update test * reset guid map before each test * failing test * reset * whitespace * Localize buttons (DynamoDS#11265) * update version num and tt file (DynamoDS#11269) * Fix merge error * Fix merge error * Revert "Fix merge error" This reverts commit 0a21ca4. Co-authored-by: aparajit-pratap <[email protected]> Co-authored-by: pinzart90 <[email protected]> Co-authored-by: Sylvester Knudsen <[email protected]> Co-authored-by: Aaron (Qilong) <[email protected]> Co-authored-by: Michael Kirschner <[email protected]> Co-authored-by: Martin Misol Monzo <[email protected]> Co-authored-by: Ashish Aggarwal <[email protected]> Co-authored-by: reddyashish <[email protected]>
| private static string CreateNodeInfo(OpenNodeAnnotationEventArgs e) | ||
| { | ||
| StringBuilder sb = new StringBuilder(); | ||
| sb.AppendLine("<h2>Node Info</h2>"); |
There was a problem hiding this comment.
@mjkkirschner Shouldn't these strings be resources?
There was a problem hiding this comment.
yes - but AFAIK we don't currently have a great way to localize web view content - I guess we could put tokens here in the html, and before displaying them, replace those with data from a resource dictionary in the extension itself?
There was a problem hiding this comment.
can we file this as an improvement?
There was a problem hiding this comment.
@mjkkirschner We have localized HTML documents for documentation browser so I guess the user expectation is that this content is localized too. Do you feel it's a big amount of work to externalize the strings here to resources?
There was a problem hiding this comment.
actually - I recall that @mmisol did some work to enable localization - I think the team may localize the html files directly.
There was a problem hiding this comment.
hah, thanks for chiming in Martin, can you remind me what the process is for html files currently?
There was a problem hiding this comment.
On our side, we should make sure HTML files go as embedded resources. That makes it a lot easier for the localization team. However, in this case it is just a string so it should go as a string resource.
|
|
||
| private string CreateNodeAnnotationContent(OpenNodeAnnotationEventArgs e) | ||
| { | ||
| var writer = new StringWriter(); |
There was a problem hiding this comment.
@sm6srw @SHKnudsen this writer implements IDisposable, it should probably be wrapped in a using, can it be fixed?
There was a problem hiding this comment.
Sorry, missed this. I will take a look.
* Fix failing tests * Revert "Fix failing tests" This reverts commit 824f785. * Fix failing tests * Properly dispose all string writers
* Fix for regression in multi-output node preview (DynamoDS#11266) * fix for multi-output node preview regression * update tests * add test * revert unwanted changes * update test * Life cycle refactoring: Get rid of singleton. Let the model create and own the MarkdownHandler on first use. * Fix crash in some document browser tests * Properly dispose all DocumentBrowserViewExtension objects * Revert "Fix crash in some document browser tests" This reverts commit ab146f9. * Display input port type specific default suggestions only (DynamoDS#11268) * input port type specific default suggestions * test fix * Test fixes * Use InTestMode flag * Delete empty lines * turn off hit testing visibility for DM related render packages (DynamoDS#11282) * Apply syntax highlighting to multi-line strings (DynamoDS#11289) * Also sanitize node documentation html * Save/Load view extension size and location (DynamoDS#11278) Information of where an extension control was last shown is saved and retrieved as preference settings. These are actually not directly visible for the user and are updating automatically, just like the main Dynamo window size. These settings are used when showing the extension control, to restore it to how it used to be before closing it. The available information for each view extension is: 1. How it is shown. For now this can be either as a floating window or docked to the right-side panel. 2. In case it is shown as a floating window, the following information about the window is saved: - Location by Top/Left coordinates - Size by Width/Height - Whether the window is maximized * add revit dlls to assembly conflict checker ignore list (DynamoDS#11285) * Update StartupUtils.cs Co-authored-by: pinzart <[email protected]> * Fix sanitizing log warnings * Node AutoComplete suggestions sorted alphabetically within the same group (DynamoDS#11280) * add/update tests * Address review comments * Add ILMerge * Fix typo * Fix mono build * Revert "Fix mono build" This reverts commit 9904525. * Refine mono solution * Remove task condition * Mono build fix * Add Readme Co-authored-by: aparajit-pratap <[email protected]> Co-authored-by: Ashish Aggarwal <[email protected]> Co-authored-by: Laurence Elsdon <[email protected]> Co-authored-by: Martin Misol Monzo <[email protected]> Co-authored-by: pinzart90 <[email protected]> Co-authored-by: pinzart <[email protected]>
|
@sm6srw should we move this out of draft PR state? |
|
@mjkkirschner Yes, I did that now. |
Purpose
This PR adds functionality to the
DocumentationBrowserso it can show Markdown files with node annotations.Markdown files gets associated with nodes by
specifying the folder where the doc are located in the *pkg jsonplacing the files in adocfolder in the package directory, each markdown doc should be named the same as the namespace of the node.FileWatchers are implemented on the annotation doc so they can be updated while loaded in Dynamo.
Node overloads is handled by specifying the input names in the markdown filename i.e.
TestPackage.TestCategory.TestCN(input1,input2)Node Annotation
Hot reload
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@Dewb
@mmisol
@QilongTang
@aparajit-pratap
FYIs
@Amoursol
For now this is implementation looks for a
docfolder in the package to find documentation files.