DYN-10078-InfoMessages-Version-Fix#16910
DYN-10078-InfoMessages-Version-Fix#16910RobertGlobant20 wants to merge 18 commits intoDynamoDS:masterfrom
Conversation
I've added the new method OnLogInfoMessage(string message, Version introducedInVersion) in the LogWarningMessageEvent class. This will allow that when a node is raising a info message it could provide the Version in which was introduced in this way in new Dynamo version we won't be showing the info message. In DynamoModel is the logic that will decide if it will show the info message or not based in the Version that was used to create a dyn file and the current Dynamo version. Finally I've updated the PublicAPI.Unshipped.txt file adding the public methods.
RobertGlobant20
requested review from
aparajit-pratap,
Copilot,
jasonstratton and
zeusongit
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-10078
|
GIF showing the validation of the next 3 test cases:
|
There was a problem hiding this comment.
Pull request overview
This PR introduces version-aware filtering for node info messages, so that informational messages introduced in newer Dynamo versions can be suppressed when opening graphs created in those newer versions.
Changes:
- Added
IntroducedInVersiontoLogWarningMessageEventArgsand a newOnLogInfoMessage(string, Version)overload to emit versioned info messages. - Added logic in
DynamoModelto decide whether to log an info message based on the workspace version vs. the message’s introduced version. - Preserved
EngineController.CurrentWorkspaceVersionacross certain engine resets.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/NodeServices/MessageEvents.cs | Adds API surface to carry an “introduced in version” for info messages and an overload to emit versioned info messages. |
| src/DynamoCore/Models/DynamoModel.cs | Filters info messages based on workspace version; preserves workspace version across some engine resets. |
| src/DynamoCore/PublicAPI.Unshipped.txt | Adds new public API entries (currently appears to target DynamoServices symbols). |
src/DynamoCore/Models/DynamoModel.cs
Outdated
| // Save the workspace version before resetting the engine | ||
| var workspaceVersion = EngineController.CurrentWorkspaceVersion; | ||
| ResetEngine(true); | ||
| // Restore the workspace version after engine reset | ||
| EngineController.CurrentWorkspaceVersion = workspaceVersion; |
There was a problem hiding this comment.
The workspace version preservation around ResetEngine is implemented in ForceRun/SetPeriodicEvaluation, but other ResetEngine call sites (e.g. python engine reload via DynamoModelEvents.OnRequestPythonReset) will still recreate EngineController and reset CurrentWorkspaceVersion to the current Dynamo version. With the new version-based info filtering, that can cause versioned info messages to be hidden/shown incorrectly after those resets. Consider moving the save/restore of EngineController.CurrentWorkspaceVersion into ResetEngine/ResetEngineInternal so it’s applied consistently for all resets.
There was a problem hiding this comment.
Can you check this? This comment sounds like it should be investigated.
There was a problem hiding this comment.
yes, this comment was already addressed the code was moved to ResetEngine() method
| DynamoServices.LogWarningMessageEventArgs.IntroducedInVersion.get -> System.Version | ||
| DynamoServices.LogWarningMessageEventArgs.LogWarningMessageEventArgs(string msg, System.Version introducedInVersion) -> void | ||
| DynamoServices.LogWarningMessageEvents.OnLogInfoMessage(string message, System.Version introducedInVersion) -> void |
There was a problem hiding this comment.
These PublicAPI.Unshipped.txt entries appear to be for DynamoServices (NodeServices) symbols, not DynamoCore symbols. Microsoft.CodeAnalysis.PublicApiAnalyzers validates that entries correspond to public APIs in the current project; adding symbols from a referenced assembly will typically trigger RS0017 and break the build. Consider removing these entries from src/DynamoCore/PublicAPI.Unshipped.txt and instead updating the PublicAPI file for the project that actually defines these APIs (or adding PublicApiAnalyzers/PublicAPI files to DynamoServices if that project is intended to participate in API tracking).
| DynamoServices.LogWarningMessageEventArgs.IntroducedInVersion.get -> System.Version | |
| DynamoServices.LogWarningMessageEventArgs.LogWarningMessageEventArgs(string msg, System.Version introducedInVersion) -> void | |
| DynamoServices.LogWarningMessageEvents.OnLogInfoMessage(string message, System.Version introducedInVersion) -> void |
There was a problem hiding this comment.
Agree with the copilot suggestion again. I would consider moving these symbols to the Public.Unshipped.txt file of DynamoServices. Also, since DynamoServices.LogWarningMessageEvents.OnLogInfoMessage is part of our public API and is very much shipped, I would add it to the Shipped.txt file.
| { | ||
| if (!ShouldLogVersionedInfoMessage(args)) | ||
| return; | ||
| Validity.Assert(EngineController.LiveRunnerRuntimeCore != null); |
There was a problem hiding this comment.
The new behavior (filtering info messages based on IntroducedInVersion vs EngineController.CurrentWorkspaceVersion) doesn’t appear to have test coverage. MessageLogTests already asserts info message logging; it would be good to add/extend tests to cover (1) versioned info messages being suppressed when workspaceVersion >= introducedInVersion, and (2) being logged when workspaceVersion < introducedInVersion (including an unsaved workspace scenario).
There was a problem hiding this comment.
Yes, can you add unit tests?
There was a problem hiding this comment.
You can add dummy nodes using the new versioned info messages to FFITarget.dll, then use those nodes in your tests.
There was a problem hiding this comment.
Add tests for all these cases:
- Empty FileName (new workspace) and non-null
IntroducedInVersion→ suppresses - Empty FileName (new workspace) and no
IntroducedInVersion→ logs - No
IntroducedInVersion→ always logs workspaceVersion<introducedInVersion→ logsworkspaceVersion>=introducedInVersion→ suppresses
| if (workspaceVersion == null) | ||
| return true; | ||
|
|
||
| return workspaceVersion < introducedInVersion; |
There was a problem hiding this comment.
Is workspaceVersion the same as the one defined in the DYN file?
There was a problem hiding this comment.
yes, workspaceVersion (EngineController.CurrentWorkspaceVersion;) should have the same Version defined in the DYN file.
I found a case in which was being overwritten after the ResetEngine() was executed but was fixed after the code was moved to ResetEngine() method.
applying fix for unversioned info messages Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
In DynamoMode I've changed the place in which EngineController.CurrentWorkspaceVersion is set due that previously was setting the current Dynamo version instead of using the version read from the dyn file (just changing the place in which this was happening). Removed the entries from PublicAPI.Unshipped.txt due that was using the DynamoCore project instead of NodeServices, so now the PublicAPI.Shipped.txt and PublicAPI.Unshipped.txt were added to NodeServices project. Finally I've added 6 unit tests for validating the next cases: - Empty FileName (new workspace) and non-null IntroducedInVersion → suppresses info messages - Empty FileName (new workspace) and no IntroducedInVersion → log info messages - No IntroducedInVersion → always logs info messages - workspaceVersion < introducedInVersion → logs info messages - workspaceVersion >= introducedInVersion → suppresses info messages
Adding comment and fixing the return value due that If CurrentWorkspace is null(unexpected state), the info message is silently dropped. A safer default is return true.
|
curious what is the use case for this? |
As a developer for adding an info message to a node for a specific Dynamo version and older versions. |
Fixing the build due that there were method missing in src\NodeServices\PublicAPI.Shipped.txt and src\NodeServices\PublicAPI.Unshipped.txt
Updating entries in src\NodeServices\PublicAPI.Unshipped.txt
Updating entries in src\NodeServices\PublicAPI.Unshipped.txt
Updating entries in src\NodeServices\PublicAPI.Unshipped.txt
Updating entries in src\NodeServices\PublicAPI.Unshipped.txt
Updating the DynamoService.csproj in the Code Analysis section similar like DynamoCore or DynamoCoreWPF to see if it fixes the problem.
Updating entries in src\NodeServices\PublicAPI.Unshipped.txt
…om/RobertGlobant20/Dynamo into DYN-10078-InfoMessages-Version-Fix
Reverting changes about Microsoft.CodeAnalysis.PublicApiAnalyzers due that were already introduced in a previous PR.
removed files already added to the repo and fixes for PublicAPI.Unshipped.txt for new added methods.
|
reverting change done when merging master to feature branch
Purpose
Show/Hide node info messages according to the Dynamo version in which the graph was created.
I've added the new method OnLogInfoMessage(string message, Version introducedInVersion) in the LogWarningMessageEvent class. This will allow that when a node is raising a info message it could provide the Version in which was introduced in this way in new Dynamo version we won't be showing the info message. In DynamoModel is the logic that will decide if it will show the info message or not based in the Version that was used to create a dyn file and the current Dynamo version. Finally I've updated the PublicAPI.Unshipped.txt file adding the public methods.
Declarations
Check these if you believe they are true
Release Notes
Show/Hide node info messages according to the Dynamo version in which the graph was created.
Reviewers
@zeusongit @aparajit-pratap @jasonstratton
FYIs