DYN-10717 Fix the Save/Save As menu items hardcoded - #17255
DYN-10717 Fix the Save/Save As menu items hardcoded#17255edwin-vasquez-ucaldas wants to merge 20 commits into
Conversation
![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-10717
There was a problem hiding this comment.
Pull request overview
This PR addresses a WPF UI bug in DynamoCoreWpf where the Save / Save As menu items were hardcoded as disabled in DynamoView.xaml, leaving them unusable for newly created (unsaved) workspaces (while keyboard shortcuts still worked via CanExecute).
Changes:
- Removed hardcoded
IsEnabled="False"from the Save / Save AsMenuItems inDynamoView.xaml. - Removed code-behind logic that imperatively re-enabled those menu items when certain events fired.
- Trimmed trailing whitespace in a generated XML documentation file.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs | Removes imperative enablement of Save/Save As menu items from event handlers (but currently leaves them out of the broader enable/disable flow). |
| src/DynamoCoreWpf/Views/Core/DynamoView.xaml | Removes hardcoded disabled state for Save/Save As menu items so WPF can derive enablement from other mechanisms. |
| doc/distrib/xml/en-US/DSCoreNodes.xml | Whitespace-only cleanup in XML documentation output. |
Comments suppressed due to low confidence (1)
src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs:472
OnWorkspaceOpenedstill re-enables Export and the shortcut bar Save button, but it no longer re-enables the Save/Save As menu items. If those menu items were disabled viaOnEnableShortcutBarItems(false)(e.g., when Start Page is shown), opening a workspace may not restore them. Re-enabling them here keeps behavior consistent with the other UI elements this handler restores.
private void OnWorkspaceOpened(WorkspaceModel workspace)
{
if (!(exportMenu is null))
{
exportMenu.IsEnabled = true;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
test/DynamoCoreWpfTests/DynamoViewTests.cs:162
- This test flips the global static
DynamoModel.IsTestModebut doesn’t guarantee it’s restored if an assertion fails, which can cascade into unrelated failures in later tests (TearDown doesn’t resetIsTestMode). Capture the original value and restore it in afinallyblock.
DynamoModel.IsTestMode = false;
ViewModel.CloseHomeWorkspaceCommand.Execute(null);
DynamoModel.IsTestMode = true;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/DynamoCoreWpf/Views/Core/DynamoView.xaml.cs:440
- Save/Save As MenuItems are no longer disabled when DynamoViewModel raises RequestEnableShortcutBarItems(false) (the handler now only disables export/shortcut bar). Since the Save commands' CanExecute predicates are currently hard-coded to always return true (DynamoViewModel.cs:3248, 3372) and Dynamo.UI.Commands.DelegateCommand does not auto-requery, the Save menu items will remain enabled even in UI-lock states like Guided Tour start (GuidesManager.cs:166). Consider moving the enable/disable logic into the commands' CanExecute (e.g., return !GuideFlowEvents.IsAnyGuideActive) and calling RaiseCanExecuteChanged when that state toggles, so both menu items and keybindings behave consistently.
private void DynamoViewModel_RequestEnableShortcutBarItems(bool enable)
{
if (!(exportMenu is null))
{
exportMenu.IsEnabled = enable;
src/DynamoCoreWpf/Views/Core/DynamoView.xaml:358
- The hardcoded IsEnabled="False" removal makes Save/Save As enabled by default, but the PR description/release note says enablement should be driven by a real CanExecute predicate. Currently both CanShowSaveDialogIfNeededAndSaveResultCommand and CanShowSaveDialogAndSaveResult always return true (DynamoViewModel.cs:3248, 3372), so enablement is effectively unconditional and won’t respect any intentional UI-disable state (e.g., Guided Tour). Implementing a state-based CanExecute (and raising CanExecuteChanged when it changes) would align behavior with the PR intent.
<MenuItem Name="saveThisButton"
Command="{Binding ShowSaveDialogIfNeededAndSaveResultCommand}"
Header="{x:Static p:Resources.DynamoViewFileMenuSave}"
InputGestureText="Ctrl + S" />
<MenuItem Name="saveButton"
…m AddHomeWorkspace() and the tab-close-creates-new-workspace path too
…red CanExecute, gated on guided tour, Start Page, and workspace dirty state
There was a problem hiding this comment.
🟢 Ready to approve
The changes consistently centralize Save/Save As enablement in CanExecute, remove conflicting imperative UI toggles, and include targeted regression tests for the reported scenarios.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
… of the whitespace
There was a problem hiding this comment.
🟡 Not ready to approve
New code introduces a couple of avoidable null-reference risks by invoking RaiseCanExecuteChanged() without null-checks on public settable command properties.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (2)
src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs:3302
SetGuidedTourActivecallsRaiseCanExecuteChanged()on the Save/Save As commands without null-checks. These commands are public settable properties, so a null assignment (by external code or future changes) would cause a crash when a guided tour starts/ends. Use null-conditional invocations to keep this gating method robust.
internal void SetGuidedTourActive(bool isActive)
{
isGuidedTourActive = isActive;
ShowSaveDialogIfNeededAndSaveResultCommand.RaiseCanExecuteChanged();
ShowSaveDialogAndSaveResultCommand.RaiseCanExecuteChanged();
src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs:1338
SaveCommandsTrackedWorkspace_PropertyChangedcallsShowSaveDialogIfNeededAndSaveResultCommand.RaiseCanExecuteChanged()without a null-check, butShowSaveDialogIfNeededAndSaveResultCommandis a public settable property (PublicAPI.Shipped). If it is ever unset (e.g., by external consumers or during future refactors), this handler will throw and potentially break workspace switching / dirty tracking. Use a null-conditional invocation here to keep the handler resilient.
This issue also appears on line 3298 of the same file.
private void SaveCommandsTrackedWorkspace_PropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == nameof(WorkspaceModel.HasUnsavedChanges))
ShowSaveDialogIfNeededAndSaveResultCommand.RaiseCanExecuteChanged();
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
@edwin-vasquez-ucaldas seems that there are two regresssions also update the branch |
Remove or correct this assertion. Refactor this method to reduce its Cognitive Complexity from 17 to the 15 allowed. Check failure: Offload the code that's conditional on this type test to the appropriate subclass and remove the condition.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔵 Human review recommended
It changes core Undo/Redo and dirty-state semantics as well as WPF command enablement behavior, which is subtle and warrants final human validation despite strong regression test coverage.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Change return type to 'void'; not a single caller uses the returned value. Refactor this method to reduce its Cognitive Complexity from 31 to the 15 allowed.
There was a problem hiding this comment.
🟡 Changes recommended
One of the newly added selection/undo tests sets HasUnsavedChanges = false without updating the saved undo depth, which can cause Undo/Redo to re-mark the workspace dirty and fail the intended regression scenario.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (2)
test/DynamoCoreTests/Models/DynamoModelCommandsTest.cs:241
- Setting HasUnsavedChanges to false here does not update the workspace's saved undo depth (savedUndoDepth), so subsequent Undo/Redo operations can re-mark the workspace as dirty even though the test intends a clean/saved baseline. Use MarkAsSaved() to keep the dirty flag and saved undo depth consistent.
CurrentDynamoModel.CurrentWorkspace.HasUnsavedChanges = false;
test/DynamoCoreTests/Models/DynamoModelCommandsTest.cs:210
- This test manually clears HasUnsavedChanges, but with the new savedUndoDepth tracking, a plain assignment can leave the workspace in an inconsistent 'clean' state relative to the undo history. Prefer MarkAsSaved() so later Undo/Redo behavior in this test (or future edits) stays consistent with real save flows.
This issue also appears on line 241 of the same file.
CurrentDynamoModel.CurrentWorkspace.HasUnsavedChanges = false;
- Files reviewed: 17/17 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟡 Changes recommended
Dirty-state recalculation on Undo/Redo can clear HasUnsavedChanges for persisted changes that are not represented in the saved-state-affecting undo depth, risking incorrect Save gating.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
… HasUnsavedChanges purely from SavedStateAffectingUndoDepth.
There was a problem hiding this comment.
🟡 Changes recommended
The new independentDirtyFlag wiring can cause undo-tracked edits to permanently latch the dirty state, preventing Undo back-to-saved from ever returning the workspace to “clean” in some common flows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🟢 Ready to approve
The changes consistently align menu/toolbar/hotkey enablement with real CanExecute + corrected dirty tracking, and the PR includes targeted regression tests for the key failure modes.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 25/25 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…led_Save_and_Save_As_options
|
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed edge cases where save/dirty-state synchronization may not update correctly (guided-tour active state changes without events and untracked dirty setters that can be cleared by undo-depth logic).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs:906
- Save enablement now relies on GuidedTourStart/GuidedTourFinish to trigger NotifySaveCommandsChanged(), but GuideFlowEvents.IsAnyGuideActive can be toggled without raising those events (e.g. GuidesManager.ContinueTourButton_Click sets GuideFlowEvents.IsAnyGuideActive = true at src/DynamoCoreWpf/UI/GuidedTour/GuidesManager.cs:288). In that flow, CanExecute will return false but menu/toolbar enable state may not refresh, leaving Save/Save As appearing enabled while a tour is active.
Consider centralizing guided-tour active state changes behind an event that is always raised (e.g. make the IsAnyGuideActive setter raise a state-changed event, or update GuidesManager to call GuideFlowEvents.OnGuidedTourStart/Finish instead of setting IsAnyGuideActive directly) and have DynamoViewModel subscribe to that signal.
TrackWorkspaceForSaveCommands(model.CurrentWorkspace);
GuideFlowEvents.GuidedTourStart += OnGuidedTourStateChanged;
GuideFlowEvents.GuidedTourFinish += OnGuidedTourStateChanged;
- Files reviewed: 25/25 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| private void UpdateHasUnsavedChangesFromSavedStateAffectingDepth() | ||
| { | ||
| // OR in independentDirtyFlag rather than overwriting: the undo stack being back | ||
| // at its saved position only means undo-tracked content is clean again | ||
| bool undoDepthIndicatesUnsaved = undoRecorder.SavedStateAffectingUndoDepth != savedUndoDepth; | ||
| HasUnsavedChanges = undoDepthIndicatesUnsaved || independentDirtyFlag; |
Purpose
The Save/Save As menu items are hardcoded
IsEnabled="False"in XAML, and no code path re-enables them for freshly created (as opposed to opened) workspaces.The MenuItems don't use
CanExecuteat all in DynamoView.xaml.IsEnabled="False"is a literal, not a binding — WPF never re-derives it from the command'sCanExecute(which, per DynamoViewModel.cs:3248 and :3372, always returns true anyway).The only two things that flip
IsEnabledback totrueare event handlers in DynamoView.xaml.cs:OnWorkspaceOpened— wired to Model.WorkspaceOpened += OnWorkspaceOpenedDynamoViewModel_RequestEnableShortcutBarItems— wired to RequestEnableShortcutBarItemsWorkspaceOpenedonly fires fromOpenWorkspace()(DynamoModel.cs) for example opening an existing file. It does not fire fromAddHomeWorkspace(), which is what creates the blank/default workspace at Dynamo startup, and is also the pattern used when a tab is closed and a fresh empty workspace takes its place.Ctrl+S bypasses the bug because
KeyBindingin XAML only checksCommand.CanExecute(hardcoded true), neverMenuItem.IsEnabled, completely independent mechanisms bound to the same command.Before fix
After fix
Declarations
Check these if you believe they are true
Release Notes
Remove the
hardcoded IsEnabled="False"and drive enablement through a realCanExecutepredicate, removing the redundant imperative toggle mechanism entirely. It fixes the whole class of bug rather than patching another missed call site.Reviewers
@jasonstratton @RobertGlobant20
@jnealb