Fix Move Diagrams to Separate File and Model Browser restore

specs/model-browser-toolwindow-restore.md

@@ -0,0 +1,75 @@
+# Model Browser Tool Window Restore
+
+## Symptom
+
+With the Model Browser docked and visible when VS launches, and no solution or `.edmx` loaded, the tool window frame renders:
+
+```
+System.ArgumentNullException: Value cannot be null.
+Parameter name: persistenceSlot
+   at Microsoft.VisualStudio.Modeling.Shell.ModelingPackage.CreateToolWindow(Guid& persistenceSlot)
+```
+
+It clears as soon as an `.edmx` is opened, and the window works normally for the rest of the session.
+
+## Evidence
+
+Exp hive activity log (`$env:APPDATA/Microsoft/VisualStudio/18.0_93d9f2a2Exp/ActivityLog.xml`, UTF-16):
+
+```
+Frame identifier: ST:0:0:{a34b1c5d-6d37-4a0c-a8b0-99f8e8158b48}
+Frame caption:    Model Browser
+```
+
+That GUID is `PackageConstants.guidExplorerWindowString`, the `[Guid]` on `EntityDesignExplorerWindow`.
+
+## Root cause
+
+The package owns two tool windows with different base classes:
+
+| | `MappingDetailsWindow` | `EntityDesignExplorerWindow` |
+|---|---|---|
+| Base class | `TreeGridDesignerToolWindow` → DSL `ToolWindow` | `ExplorerWindow` → shell `ToolWindowPane` |
+| `[ProvideToolWindow]` | yes | yes |
+| `AddToolWindow` in `Initialize` | yes | **no — cannot be** |
+| Retrieved by | `GetToolWindow` (DSL) | `FindToolWindow` (shell) |
+
+On startup the shell asks for each persisted frame through `IVsPackage.CreateTool(ref Guid)`. That reaches `ModelingPackage.CreateToolWindow`, which resolves persistence slots **only** against the registry `AddToolWindow` fills, and never falls back to attribute-based resolution — so `[ProvideToolWindow]` alone is not enough for a `ModelingPackage`. The Explorer's slot isn't in that registry, the lookup yields nothing, and the DSL code throws naming its own `persistenceSlot` parameter.
+
+It self-heals because opening an `.edmx` reaches the `ExplorerWindow` property, which calls `FindToolWindow` — the base `Package` path, which *does* honour the attribute.
+
+## Approaches that do not work
+
+Verified against the compiler, not assumed:
+
+1. **`AddToolWindow(typeof(EntityDesignExplorerWindow))`.** Compiles, but `GetToolWindow` returns DSL `ToolWindow` and `ExplorerWindow` derives from shell `ToolWindowPane` — `CS0039`, no conversion. The DSL registry is typed end-to-end to `ToolWindow`, so a `ToolWindowPane` can never live in it.
+2. **Overriding `CreateToolWindow(ref Guid, uint)`.** `CS0506` — inherited from `ModelingPackage` but not virtual.
+3. **Overriding `CreateToolWindow(ref Guid)`.** `CS0115` — the one-argument form in the stack trace is private.
+4. **Overriding `InstantiateToolWindow(Type)`.** Overridable, but it sits downstream of the GUID→Type lookup that fails, so it is never reached.
+
+## Fix applied
+
+`Transient = true` on the Explorer's `[ProvideToolWindow]` in `MicrosoftDataEntityDesignPackage.cs`.
+
+Transient windows are kept out of the persisted layout, so the shell never attempts the restore that this package cannot service. The window is still created on demand through `FindToolWindow`, which honours the attribute, and `[ProvideToolWindowVisibility(..., MicrosoftDataEntityDesignEditorFactoryId)]` already ties it to the EDMX editor context — so tying its lifetime to a document rather than to the window layout matches the existing declaration.
+
+**Behaviour change:** the Model Browser no longer reappears docked when VS starts. It appears when an `.edmx` is opened. Previously it did reappear, but as an exception box that was non-functional until a model loaded.
+
+## Verification
+
+Not yet run. Steps:
+
+1. Build, launch the Exp hive: `devenv.exe /RootSuffix Exp /log`
+2. Open an `.edmx`, open the Model Browser, dock it, close the document and solution, exit VS.
+3. Relaunch with no solution. The Model Browser should be **absent**, with no exception in any frame.
+4. Open an `.edmx`. The Model Browser should appear and populate.
+5. Confirm the log is clean:
+   `iconv -f UTF-16 -t UTF-8 "$APPDATA/Microsoft/VisualStudio/18.0_93d9f2a2Exp/ActivityLog.xml" | grep -i persistenceSlot`
+
+**Caveat:** a layout persisted by a previous build still contains the frame. The first launch after this change may still attempt one restore before the layout is rewritten. If the exception appears once and never again, that is the stale layout, not a failure of the fix — confirm with Window → Reset Window Layout or a fresh Exp hive.
+
+## If restore-at-startup must be preserved
+
+Explicitly re-implementing `IVsPackage.CreateTool(ref Guid)` on the package compiles cleanly and is the only viable interception point: handle the Explorer's slot via `FindToolWindow(typeof(EntityDesignExplorerWindow), 0, true)` and return `S_OK`, delegating every other slot to `CreateToolWindow(ref slot, 0)`. Re-implementing the interface re-maps all of `IVsPackage`, with unimplemented members binding to `ModelingPackage`'s public methods. This was not taken because it trades a one-attribute change for shell-interop that only the Exp hive can validate, to restore a window that is empty without a document.
+
+`MappingDetailsWindow` is unaffected — it is in the DSL registry — and should be confirmed as such during verification. If it shows the same exception, this analysis is wrong and should be redone rather than patched.

src/Microsoft.Data.Entity.Design.Package/CustomCode/DiagramSurfaceContextMenuService.cs

@@ -6,6 +6,7 @@
 using Microsoft.Data.Entity.Design.EntityDesigner.View.ContextMenu;
 using Microsoft.Data.Entity.Design.EntityDesigner.View.Export;
 using Microsoft.Data.Entity.Design.EntityDesigner.ViewModel;
+using Microsoft.Data.Entity.Design.Model;
 using Microsoft.Data.Entity.Design.VisualStudio;
 using Microsoft.Data.Entity.Design.VisualStudio.Package;
 using Microsoft.VisualStudio;
@@ -82,6 +83,7 @@ internal sealed class DiagramSurfaceContextMenuService : IDisposable
 
         // Shared command references for diagram surface menu
         private MenuCommandDefinition _showDataTypesCommand;
+        private MenuCommandDefinition _moveDiagramsToSeparateFileCommand;
 
         private bool _isDisposed;
 
@@ -405,6 +407,19 @@ private void PopulateDiagramSurfaceMenu()
             };
             _diagramSurfaceMenu.MenuItems.Add(_showDataTypesCommand);
 
+            // Move Diagrams to Separate File. This shares the group above rather than getting its own: the item is
+            // hidden once the diagrams already live in a .edmx.diagram file, and a dedicated group would leave two
+            // adjacent separators behind whenever that happens.
+            _moveDiagramsToSeparateFileCommand = new MenuCommandDefinition(
+                "MoveDiagramsToSeparateFile",
+                "Move Diagrams to Separate File",
+                KnownMonikers.MoveToFolder,
+                "Move the diagram layout out of the EDMX and into a separate .edmx.diagram file")
+            {
+                IsVisible = false
+            };
+            _diagramSurfaceMenu.MenuItems.Add(_moveDiagramsToSeparateFileCommand);
+
             // Separator
             _diagramSurfaceMenu.MenuItems.Add(MenuSeparatorDefinition.Instance);
 
@@ -501,6 +516,15 @@ private void UpdateMenuItemStates(System.Collections.ObjectModel.ObservableColle
             {
                 _showDataTypesCommand.IsChecked = diagram.DisplayNameAndType;
             }
+
+            // Shares its eligibility rules with the Model Browser command so the two never disagree about whether the
+            // move is on offer.
+            if (_moveDiagramsToSeparateFileCommand != null)
+            {
+                var artifact = diagram.GetModel()?.EditingContext?.GetEFArtifactService()?.Artifact as EntityDesignArtifact;
+                _moveDiagramsToSeparateFileCommand.IsVisible =
+                    MicrosoftDataEntityDesignCommandSet.CanMoveDiagramsToSeparateFile(artifact, out _);
+            }
         }
 
         private void OnDiagramSurfaceMenuActionExecuted(object sender, MenuActionEventArgs e)
@@ -566,6 +590,10 @@ private void OnDiagramSurfaceMenuActionExecuted(object sender, MenuActionEventAr
                     ExecuteToggleShowDataTypes(diagram);
                     break;
 
+                case "MoveDiagramsToSeparateFile":
+                    ExecuteMoveDiagramsToSeparateFile(diagram);
+                    break;
+
                 // Select All
                 case "SelectAll":
                     ExecuteSelectAll(diagram);
@@ -1610,6 +1638,20 @@ private void ExecuteSelectAll(EntityDesignerDiagram diagram)
             }
         }
 
+        private void ExecuteMoveDiagramsToSeparateFile(EntityDesignerDiagram diagram)
+        {
+            var artifact = diagram.GetModel()?.EditingContext?.GetEFArtifactService()?.Artifact;
+            if (artifact is null)
+            {
+                return;
+            }
+
+            // Calls the command set directly rather than going through the shell command table: the OLE command's
+            // status handler is scoped to the Diagrams node in the Model Browser, so it reports the command hidden
+            // when the click came from the designer surface.
+            MicrosoftDataEntityDesignCommandSet.MoveDiagramsToSeparateFile(artifact.Uri.LocalPath);
+        }
+
         private void ExecuteShowMappingDetails()
         {
             // Execute the View Mapping Details command via VS command

src/Microsoft.Data.Entity.Design.Package/CustomCode/MicrosoftDataEntityDesignCommandSet.cs

@@ -4285,56 +4285,159 @@ internal void OnMenuIncludeRelatedEntityType(object sender, EventArgs e)
         /// </summary>
         internal void OnStatusMoveDiagramsToSeparateFile(object sender, EventArgs e)
         {
-            if (sender is MenuCommand cmd)
+            if (sender is not MenuCommand cmd)
             {
-                cmd.Enabled = cmd.Visible = false;
+                return;
+            }
 
-                // check whether the EDMX Project Item is a link.
-                var uri = Utils.FileName2Uri(CurrentDocData.FileName);
-                var artifactProjectItem = VsUtils.GetProjectItemForDocument(uri.LocalPath, Services.ServiceProvider);
+            cmd.Enabled = cmd.Visible = false;
 
-                if (artifactProjectItem != null
-                    && VsUtils.IsLinkProjectItem(artifactProjectItem) == false)
-                {
-                    // Only show if the diagram artifact is not null
-                    var modelManager = PackageManager.Package.ModelManager;
-                    EntityDesignArtifact artifact = modelManager.GetArtifact(uri) as EntityDesignArtifact;
-                    Debug.Assert(artifact != null, "There is no EntityDesignArtifact with URI:" + uri.LocalPath + " in modelmanager.");
+            // Offered on the Diagrams container node only. The command moves every diagram in the model, so offering
+            // it on an individual diagram would imply it moves just that one. This is the common case for the command
+            // being hidden and is not logged below.
+            if (SelectedExplorerItem is not ExplorerDiagrams)
+            {
+                return;
+            }
 
-                    if (artifact != null
-                        && artifact.DiagramArtifact == null)
-                    {
-                        cmd.Enabled = cmd.Visible = true;
-                    }
-                }
+            // Past this point the user is on the node where the command belongs, so each reason it stays hidden is
+            // recorded rather than left for someone to work out from an absent menu item.
+            if (CurrentDocData is null)
+            {
+                VsUtils.LogToActivityLog("MoveDiagramsToSeparateFile hidden: there is no current doc data.");
+                return;
             }
+
+            var uri = Utils.FileName2Uri(CurrentDocData.FileName);
+            var artifact = PackageManager.Package.ModelManager.GetArtifact(uri) as EntityDesignArtifact;
+
+            if (!CanMoveDiagramsToSeparateFile(artifact, out var reason))
+            {
+                VsUtils.LogToActivityLog($"MoveDiagramsToSeparateFile hidden: {reason}.");
+                return;
+            }
+
+            cmd.Enabled = cmd.Visible = true;
+
+            // The registered status handler chain runs IsArtifactDesignerSafeAndEditSafeHandler after this method and
+            // can still hide the command, so record what that check will decide.
+            VsUtils.LogToActivityLog(
+                $"MoveDiagramsToSeparateFile shown; designerSafeAndEditSafe={IsArtifactDesignerSafeAndEditSafe()}");
         }
 
         internal void OnMenuMoveDiagramsToSeparateFile(object sender, EventArgs e)
         {
+            // Take the file name once, before anything else. DoMigrate reloads the artifact and resets the current
+            // context, and CurrentDocData resolves through the shell's selection service every time it is read - so
+            // reading it again afterwards can come back null and take the whole command down with it.
+            if (CurrentDocData is null)
+            {
+                VsUtils.LogToActivityLog(
+                    "MoveDiagramsToSeparateFile: there is no current doc data.", __ACTIVITYLOG_ENTRYTYPE.ALE_WARNING);
+                return;
+            }
+
+            MoveDiagramsToSeparateFile(CurrentDocData.FileName);
+        }
+
+        /// <summary>
+        ///     Determines whether the diagrams in an artifact can be moved out into a sibling .edmx.diagram file.
+        /// </summary>
+        /// <param name="artifact">The artifact whose diagrams would be moved. May be null.</param>
+        /// <param name="reason">When this returns false, a lower-case phrase describing what disqualified the artifact.</param>
+        /// <returns><see langword="true" /> when the move is available, otherwise <see langword="false" />.</returns>
+        /// <remarks>
+        ///     Shared by the Model Browser command's status handler and the designer surface context menu so both offer
+        ///     the command under identical conditions. Callers add their own context checks on top - the Model Browser
+        ///     only offers it on the Diagrams node, for instance - but the rules about the artifact itself live here.
+        /// </remarks>
+        internal static bool CanMoveDiagramsToSeparateFile(EntityDesignArtifact artifact, out string reason)
+        {
+            if (artifact is null)
+            {
+                reason = "there is no EntityDesignArtifact for the document";
+                return false;
+            }
+
+            if (artifact.DiagramArtifact is not null)
+            {
+                reason = "the diagrams are already in a separate file";
+                return false;
+            }
+
+            // A linked EDMX cannot have a sibling diagram file added alongside it.
+            var artifactProjectItem = VsUtils.GetProjectItemForDocument(artifact.Uri.LocalPath, Services.ServiceProvider);
+            if (artifactProjectItem is null)
+            {
+                reason = $"no project item was found for '{artifact.Uri.LocalPath}'";
+                return false;
+            }
+
+            if (VsUtils.IsLinkProjectItem(artifactProjectItem))
+            {
+                reason = "the EDMX is a linked project item";
+                return false;
+            }
+
+            reason = null;
+
+            return true;
+        }
+
+        /// <summary>
+        ///     Moves every diagram out of an EDMX file and into a sibling .edmx.diagram file, after confirming with the user.
+        /// </summary>
+        /// <param name="edmxFileName">Full path of the EDMX file whose diagrams should be moved.</param>
+        /// <remarks>
+        ///     Shared by the Model Browser command and the designer surface context menu. It takes a file name rather than
+        ///     reading the current document itself because the migration reloads the artifact and resets the current
+        ///     context, after which any ambient "current document" lookup can come back null.
+        /// </remarks>
+        internal static void MoveDiagramsToSeparateFile(string edmxFileName)
+        {
+            if (string.IsNullOrWhiteSpace(edmxFileName))
+            {
+                VsUtils.LogToActivityLog(
+                    "MoveDiagramsToSeparateFile: no EDMX file name was supplied.", __ACTIVITYLOG_ENTRYTYPE.ALE_WARNING);
+                return;
+            }
+
             var result = VsUtils.ShowMessageBox(
                 Services.ServiceProvider, Resources.MoveDiagramNodesWarning
                 , OLEMSGBUTTON.OLEMSGBUTTON_YESNO, OLEMSGDEFBUTTON.OLEMSGDEFBUTTON_SECOND, OLEMSGICON.OLEMSGICON_WARNING);
 
-            if (result == DialogResult.Yes)
+            if (result != DialogResult.Yes)
             {
-                var uri = Utils.FileName2Uri(CurrentDocData.FileName);
-                var editingContext = PackageManager.Package.DocumentFrameMgr.EditingContextManager.GetNewOrExistingContext(uri);
-                Debug.Assert(editingContext != null, "EditingContext for artifact with uri: " + uri + " is not available.");
-                if (editingContext != null)
-                {
-                    var efArtifactService = editingContext.GetEFArtifactService();
-                    EntityDesignArtifact entityDesignArtifact = efArtifactService.Artifact as EntityDesignArtifact;
+                return;
+            }
 
-                    // Don't need to put the transaction name in resource string table since we clear the VS undo stack after the command is executed.
-                    CommandProcessorContext cpc = new CommandProcessorContext(
-                        editingContext, EfiTransactionOriginator.EntityDesignerOriginatorId, "MoveDiagrams");
-                    MigrateDiagramInformationCommand.DoMigrate(cpc, entityDesignArtifact);
-                    // Save the EDMX file.
-                    RunningDocumentTable rdt = new RunningDocumentTable(Services.ServiceProvider);
-                    rdt.SaveFileIfDirty(CurrentDocData.FileName);
-                }
+            var uri = Utils.FileName2Uri(edmxFileName);
+
+            var editingContext = PackageManager.Package.DocumentFrameMgr?.EditingContextManager.GetNewOrExistingContext(uri);
+            if (editingContext is null)
+            {
+                VsUtils.LogToActivityLog(
+                    $"MoveDiagramsToSeparateFile: no editing context is available for '{uri}'.",
+                    __ACTIVITYLOG_ENTRYTYPE.ALE_ERROR);
+                return;
             }
+
+            if (editingContext.GetEFArtifactService()?.Artifact is not EntityDesignArtifact entityDesignArtifact)
+            {
+                VsUtils.LogToActivityLog(
+                    $"MoveDiagramsToSeparateFile: the editing context for '{uri}' holds no EntityDesignArtifact.",
+                    __ACTIVITYLOG_ENTRYTYPE.ALE_ERROR);
+                return;
+            }
+
+            // Don't need to put the transaction name in resource string table since we clear the VS undo stack after the command is executed.
+            CommandProcessorContext cpc = new CommandProcessorContext(
+                editingContext, EfiTransactionOriginator.EntityDesignerOriginatorId, "MoveDiagrams");
+            MigrateDiagramInformationCommand.DoMigrate(cpc, entityDesignArtifact);
+
+            // Save the EDMX file.
+            RunningDocumentTable rdt = new RunningDocumentTable(Services.ServiceProvider);
+            rdt.SaveFileIfDirty(edmxFileName);
         }
 
         #endregion