diff --git a/specs/model-browser-toolwindow-restore.md b/specs/model-browser-toolwindow-restore.md new file mode 100644 index 00000000..b72ff005 --- /dev/null +++ b/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. diff --git a/src/Microsoft.Data.Entity.Design.Package/CustomCode/DiagramSurfaceContextMenuService.cs b/src/Microsoft.Data.Entity.Design.Package/CustomCode/DiagramSurfaceContextMenuService.cs index 543044cc..d19210a8 100644 --- a/src/Microsoft.Data.Entity.Design.Package/CustomCode/DiagramSurfaceContextMenuService.cs +++ b/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 diff --git a/src/Microsoft.Data.Entity.Design.Package/CustomCode/MicrosoftDataEntityDesignCommandSet.cs b/src/Microsoft.Data.Entity.Design.Package/CustomCode/MicrosoftDataEntityDesignCommandSet.cs index 85a406b6..252732f6 100644 --- a/src/Microsoft.Data.Entity.Design.Package/CustomCode/MicrosoftDataEntityDesignCommandSet.cs +++ b/src/Microsoft.Data.Entity.Design.Package/CustomCode/MicrosoftDataEntityDesignCommandSet.cs @@ -4285,56 +4285,159 @@ internal void OnMenuIncludeRelatedEntityType(object sender, EventArgs e) /// 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); + } + + /// + /// Determines whether the diagrams in an artifact can be moved out into a sibling .edmx.diagram file. + /// + /// The artifact whose diagrams would be moved. May be null. + /// When this returns false, a lower-case phrase describing what disqualified the artifact. + /// when the move is available, otherwise . + /// + /// 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. + /// + 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; + } + + /// + /// Moves every diagram out of an EDMX file and into a sibling .edmx.diagram file, after confirming with the user. + /// + /// Full path of the EDMX file whose diagrams should be moved. + /// + /// 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. + /// + 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 diff --git a/src/Microsoft.Data.Entity.Design.Package/CustomCode/MicrosoftDataEntityDesignPackage.cs b/src/Microsoft.Data.Entity.Design.Package/CustomCode/MicrosoftDataEntityDesignPackage.cs index c22bb846..45d57e4d 100644 --- a/src/Microsoft.Data.Entity.Design.Package/CustomCode/MicrosoftDataEntityDesignPackage.cs +++ b/src/Microsoft.Data.Entity.Design.Package/CustomCode/MicrosoftDataEntityDesignPackage.cs @@ -23,10 +23,19 @@ namespace Microsoft.Data.Entity.Design.Package { + // Transient because this package cannot service a shell-initiated restore of this window. The shell asks for a + // persisted frame through IVsPackage.CreateTool, which reaches ModelingPackage.CreateToolWindow - a non-virtual + // method that resolves persistence slots only against the registry AddToolWindow fills. That registry is typed to + // the DSL SDK's ToolWindow, and EntityDesignExplorerWindow is a plain shell ToolWindowPane, so the slot cannot be + // registered and the restore throws ArgumentNullException into the window's own frame. Marking the window + // transient keeps it out of the persisted layout, so that path is never taken; it is still opened on demand + // through FindToolWindow, which honours this attribute, and ProvideToolWindowVisibility below already ties it to + // the EDMX editor context. [ProvideToolWindow(typeof(EntityDesignExplorerWindow), MultiInstances = false, Style = VsDockStyle.Tabbed, Orientation = ToolWindowOrientation.Right, + Transient = true, Window = "{3AE79031-E1BC-11D0-8F78-00A0C9110057}")] [ProvideToolWindowVisibility(typeof(EntityDesignExplorerWindow), Constants.MicrosoftDataEntityDesignEditorFactoryId)] [ProvideToolWindow(typeof(MappingDetailsWindow), @@ -94,6 +103,10 @@ protected override void Initialize() ModelGenErrorCache = new ModelGenErrorCache(); ConnectionManager = new ConnectionManager(); + // Only MappingDetailsWindow is registered here: ModelingPackage's tool window registry, and the + // GetToolWindow lookup that reads it, are typed to the DSL SDK's ToolWindow. EntityDesignExplorerWindow + // is a plain shell ToolWindowPane, so it is resolved through FindToolWindow and its ProvideToolWindow + // attribute instead. AddToolWindow(typeof(MappingDetailsWindow)); // Register for VS Events @@ -402,7 +415,7 @@ public ExplorerWindow ExplorerWindow { get { - if (_explorerWindow == null) + if (_explorerWindow is null) { using (DpiAwareness.EnterDpiScope(DpiAwarenessContext.SystemAware)) { diff --git a/src/Microsoft.Data.Entity.Design/VisualStudio/Model/Commands/MigrateDiagramInformationCommand.cs b/src/Microsoft.Data.Entity.Design/VisualStudio/Model/Commands/MigrateDiagramInformationCommand.cs index 4c632632..bd9e37e9 100644 --- a/src/Microsoft.Data.Entity.Design/VisualStudio/Model/Commands/MigrateDiagramInformationCommand.cs +++ b/src/Microsoft.Data.Entity.Design/VisualStudio/Model/Commands/MigrateDiagramInformationCommand.cs @@ -4,6 +4,7 @@ using System.Diagnostics; using System.IO; using System.Linq; +using System.Runtime.InteropServices; using System.Xml.Linq; using EnvDTE; using Microsoft.Data.Entity.Design.Model; @@ -11,6 +12,7 @@ using Microsoft.Data.Entity.Design.VersioningFacade; using Microsoft.Data.Entity.Design.VisualStudio.Package; using Microsoft.Data.Tools.VSXmlDesignerBase.Model.VisualStudio; +using Microsoft.VisualStudio.Shell.Interop; using Command = Microsoft.Data.Entity.Design.Model.Commands.Command; namespace Microsoft.Data.Entity.Design.VisualStudio.Model.Commands @@ -96,8 +98,7 @@ internal static void DoMigrate(CommandProcessorContext cpc, EntityDesignArtifact if (artifact.DiagramArtifact != null) { // Ensure that diagram file is added to the project. - DTE service = PackageManager.Package.GetService(typeof(DTE)) as DTE; - service.ItemOperations.AddExistingItem(artifact.DiagramArtifact.Uri.LocalPath); + AddDiagramFileToProject(artifact); // Reload the artifacts. artifact.ReloadArtifact(); @@ -115,5 +116,73 @@ internal static void DoMigrate(CommandProcessorContext cpc, EntityDesignArtifact } } } + + // + // Adds the newly created diagram file to the project that owns the EDMX it was split out of. + // + // + // DTE's ItemOperations.AddExistingItem cannot be used here. It adds to whatever Solution Explorer has + // selected, but this command runs from the designer surface and the Model Browser, where the Solution + // Explorer selection is unrelated to the EDMX - and when nothing is selected it throws + // "Item can not be added to a project when multiple or no items are selected in the Solution Explorer." + // Resolving the project from the artifact's own path removes the dependency on the selection entirely. + // The diagram file is added under the EDMX's project item so it nests, which is how the shipping designer + // records it (a None item with DependentUpon pointing at the EDMX). + // + private static void AddDiagramFileToProject(EntityDesignArtifact artifact) + { + var diagramFilePath = artifact.DiagramArtifact.Uri.LocalPath; + + ProjectItem edmxProjectItem = VsUtils.GetProjectItemForDocument(artifact.Uri.LocalPath, Services.ServiceProvider); + if (edmxProjectItem is null) + { + // The EDMX is not part of a project - the Miscellaneous Files project, for example. The diagram file + // still exists on disk beside it and the model still loads; there is simply nothing to add it to. + VsUtils.LogToActivityLog( + $"MigrateDiagramInformation: '{artifact.Uri.LocalPath}' has no project item, so '{diagramFilePath}' was not added to a project.", + __ACTIVITYLOG_ENTRYTYPE.ALE_WARNING); + return; + } + + // SDK-style projects pick the file up by glob as soon as it lands on disk, and adding it again throws. + if (VsUtils.GetProjectItemForDocument(diagramFilePath, Services.ServiceProvider) is not null) + { + return; + } + + Project containingProject = edmxProjectItem.ContainingProject; + if (containingProject is null + || VsUtils.IsMiscellaneousProject(containingProject)) + { + // An EDMX opened on its own, with no solution or project loaded, lands in the Miscellaneous Files + // project, which does not support ProjectItems extensibility. The diagram file sits beside the EDMX on + // disk and the model loads from it either way; there is simply no project to record it in. + VsUtils.LogToActivityLog( + $"MigrateDiagramInformation: '{artifact.Uri.LocalPath}' does not belong to a real project, so '{diagramFilePath}' was not added to one."); + return; + } + + ProjectItems targetCollection = edmxProjectItem.ProjectItems ?? containingProject.ProjectItems; + if (targetCollection is null) + { + VsUtils.LogToActivityLog( + $"MigrateDiagramInformation: the project containing '{artifact.Uri.LocalPath}' does not support adding items, so '{diagramFilePath}' was not added to it.", + __ACTIVITYLOG_ENTRYTYPE.ALE_WARNING); + return; + } + + try + { + targetCollection.AddFromFile(diagramFilePath); + } + catch (COMException ex) + { + // The diagrams have already been written to disk and the artifact reloaded by this point. A project + // system that refuses the item should not make the migration itself look like it failed. + VsUtils.LogToActivityLog( + $"MigrateDiagramInformation: could not add '{diagramFilePath}' to the project: {ex.Message}", + __ACTIVITYLOG_ENTRYTYPE.ALE_ERROR); + } + } } } diff --git a/src/Microsoft.Data.Entity.Design/VisualStudio/VsUtils.cs b/src/Microsoft.Data.Entity.Design/VisualStudio/VsUtils.cs index 9248f3d8..af403abb 100644 --- a/src/Microsoft.Data.Entity.Design/VisualStudio/VsUtils.cs +++ b/src/Microsoft.Data.Entity.Design/VisualStudio/VsUtils.cs @@ -1356,6 +1356,14 @@ internal static bool IsLinkProjectItem(ProjectItem projectItem) // Check whether project item is a linked item var isLinkItem = false; + // SDK-style projects do not always expose the DTE automation property collection, in which case there is + // no IsLink property to read and the item is not a link. Without this the null dereference below throws + // out of command status handlers, where the shell swallows it and the command silently never appears. + if (projectItem?.Properties is null) + { + return false; + } + // immediately state false if this is a website project since websites don't // support this. The DTE calls after this will throw COM Exceptions if we continue. // As of build 20815.00 diff --git a/src/Microsoft.Data.Entity.Tests.Design.Package/CustomCode/MoveDiagramsToSeparateFileTests.cs b/src/Microsoft.Data.Entity.Tests.Design.Package/CustomCode/MoveDiagramsToSeparateFileTests.cs new file mode 100644 index 00000000..9d3b7c13 --- /dev/null +++ b/src/Microsoft.Data.Entity.Tests.Design.Package/CustomCode/MoveDiagramsToSeparateFileTests.cs @@ -0,0 +1,60 @@ +// Copyright (c) Microsoft. All Rights Reserved. Licensed under the MIT license. See License.txt in the project root for license information. + +using System; +using System.Linq; +using FluentAssertions; +using Microsoft.Data.Entity.Design.Package; +using Microsoft.Data.Entity.Design.VisualStudio; +using Microsoft.Data.Entity.Design.VisualStudio.Package; +using Microsoft.VisualStudio.Shell; +using Microsoft.VisualStudio.TestTools.UnitTesting; + +namespace Microsoft.Data.Entity.Tests.DesignPackage.CustomCode +{ + /// + /// Covers the parts of the Move Diagrams to Separate File command that do not need a running shell. + /// + /// + /// The migration itself, the status handler and the project-item checks all reach into VS services and are only + /// exercisable in the experimental hive. What is covered here is the guard behaviour that used to be missing: + /// the entry points now refuse bad input instead of throwing out of a command handler, where the shell swallows + /// the exception and the user sees nothing. + /// + [TestClass] + public class MoveDiagramsToSeparateFileTests + { + [TestMethod] + public void CanMoveDiagramsToSeparateFile_returns_false_for_a_null_artifact() + { + MicrosoftDataEntityDesignCommandSet.CanMoveDiagramsToSeparateFile(null, out var reason) + .Should().BeFalse(); + + reason.Should().NotBeNullOrWhiteSpace( + "the caller logs this phrase, so an empty one would produce a log entry that explains nothing"); + } + + [TestMethod] + public void IsLinkProjectItem_returns_false_when_there_is_no_project_item() + { + // SDK-style projects do not always expose the DTE automation property collection. Before this guard the + // null dereference threw out of the command status handler and the command silently never appeared. + VsUtils.IsLinkProjectItem(null).Should().BeFalse(); + } + + [TestMethod] + public void ExplorerWindow_is_declared_transient() + { + var explorerWindow = typeof(MicrosoftDataEntityDesignPackage) + .GetCustomAttributes(typeof(ProvideToolWindowAttribute), inherit: false) + .Cast() + .SingleOrDefault(attribute => attribute.ToolType == typeof(EntityDesignExplorerWindow)); + + explorerWindow.Should().NotBeNull(); + explorerWindow.Transient.Should().BeTrue( + "a persisted frame is restored through ModelingPackage.CreateToolWindow, which resolves slots only " + + "against the DSL tool window registry - EntityDesignExplorerWindow is a shell ToolWindowPane and " + + "cannot be registered there, so a non-transient declaration throws ArgumentNullException into the " + + "window's frame at startup"); + } + } +}