Move to Folder Refactoring #5438

MDoerner · 2020-03-21T23:15:37Z

Closes #3332
Closes #5159

This PR introduces MoveToFolderRefactoring, MoveContainingFolderRefactoring and CodeExplorerMoveToFolderCommand allowing to move components and folders around in the folder hierarchy. The target is derived from the selected component for the refactorings and from the selected folder or component view model for the CE command. The target folder is selected in a dialog I borrowed from Rename.

This PR also extracts the user interaction part from the InteractiveRefactoringBase into a different class then injected back. This allows to reuse the interaction elsewhere, e.g. in CE commands. (Sorry for any conflicts.)

The backing refactoring actions and interactions for the refactorings allow to process multiple modules or folders at once. However, I had to realize that the CE does not allow multi-selection. Thus, the CE command does not deal with multiple selected nodes so far. Should we change our stand on multi-select in the CE, adapting the command to this, should be comparatively easy.

We have drag and drop now as well.

Component move via dialog:

Folder move via dialog:

Component move via drag and drop:

Folder move via drag and drop:

Also correctly converts folder annotation arguments to the string content and back. This also removes the parentheses around folders.

It did not consider that there could be a getter in another module with the same name.

It moves the containing folder into another folder. Also fixes ToVbaStringLiteral. (It was missing the surrounding quotes.)

Also adds members to get the initial model to InteractiveRefactoringTestsBase.

There are already tests for the involved refactoring actions.

This moves interactive refactorings from using inheritance to deal with the interaction to composition. Consequently, the interaction can be reused elsewhere.

This combines MoveToFolder and MoveFolder, selecting based on the selected node of the CE. In principle, the backing refactoring actions are capable of dealing with multiple nodes at once. However, the CE only allows selecting single items at the moment and the command reflects that when deriving the models to pass to the refactoring actions.

codecov · 2020-03-21T23:15:40Z

Codecov Report

Merging #5438 into next will decrease coverage by 0.35%.
The diff coverage is 32.30%.

@@            Coverage Diff             @@
##             next    #5438      +/-   ##
==========================================
- Coverage   61.49%   61.14%   -0.35%     
==========================================
  Files        1194     1222      +28     
  Lines       40595    41003     +408     
  Branches     8666     8749      +83     
==========================================
+ Hits        24961    25070     +109     
- Misses      13790    14080     +290     
- Partials     1844     1853       +9

Impacted Files	Coverage Δ
...e/Navigation/CodeExplorer/CodeExplorerViewModel.cs	`58.97% <0.00%> (-0.51%)`	⬇️
...UI/CodeExplorer/CodeExplorerAddComponentService.cs	`22.22% <0.00%> (-2.34%)`	⬇️
...duck.Core/UI/CodeExplorer/CodeExplorerControl.xaml	`0.00% <0.00%> (ø)`
...k.Core/UI/CodeExplorer/CodeExplorerControl.xaml.cs	`0.00% <0.00%> (ø)`
...ds/Abstract/CodeExplorerMoveToFolderCommandBase.cs	`0.00% <0.00%> (ø)`
...plorer/Commands/CodeExplorerMoveToFolderCommand.cs	`0.00% <0.00%> (ø)`
...Drop/CodeExplorerMoveToFolderDragAndDropCommand.cs	`0.00% <0.00%> (ø)`
...PaneRefactorMoveContainingFolderCommandMenuItem.cs	`0.00% <0.00%> (ø)`
...ems/CodePaneRefactorMoveToFolderCommandMenuItem.cs	`0.00% <0.00%> (ø)`
...nd/MenuItems/ParentMenus/RefactoringsParentMenu.cs	`0.00% <ø> (ø)`
... and 70 more

The exception is moving into the project folder. This simply adds the explicit folder annotation.

# Conflicts: # Rubberduck.Resources/RubberduckUI.resx

retailcoder · 2020-03-28T20:00:08Z

Rubberduck.Core/UI/Command/Refactorings/CodePaneRefactorMoveContainingFolderCommand.cs

+            return target != null
+                   && target is ModuleDeclaration
+                   && !_state.IsNewOrModified(target.QualifiedModuleName);


Wouldn't the null-check be redundant with target is ModuleDeclaration module?

e.g.

return target is ModuleDeclaration module && !_state.IsNewOrModified(module.QualifiedModuleName);

Actually the null-check is redundant even without a module variable.

I will remove the null check.

Rubberduck.Core/UI/Command/Refactorings/CodePaneRefactorMoveToFolderCommand.cs

retailcoder · 2020-03-28T20:13:48Z

Rubberduck.Core/UI/Refactorings/MoveFolder/MoveMultipleFoldersView.xaml.cs

+            Loaded += (o, e) =>
+                {
+                    MoveToFolderTextBox.Focus();
+                    MoveToFolderTextBox.SelectAll();
+                };


How does the inline EventHandler delegate get de-registered?

I have to admit that I simply copied this from the Rename dialog. Maybe, whoever wrote that can answer.

I think it simply gets unregistered when the view gets GCed. Remember, it is the event that keeps the handler alive, not the other way around.

I will add explicit de-registration, nonetheless.

retailcoder · 2020-03-28T20:18:37Z

Rubberduck.Core/UI/Refactorings/MoveToFolder/MoveMultipleToFolderView.xaml.cs

+            Loaded += (o, e) =>
+                {
+                    MoveToFolderTextBox.Focus();
+                    MoveToFolderTextBox.SelectAll();
+                };


Worrying about handler holding up GC here too

Could we simply unregister the handler in the handler itself?

retailcoder · 2020-03-28T20:31:52Z

Rubberduck.Refactorings/MoveFolder/IMoveMultipleFoldersPresenter.cs

+    public interface IMoveMultipleFoldersPresenter : IRefactoringPresenter<MoveMultipleFoldersModel>
+    {}


Why do we need this marker interface?

Because we use it as the type parameter in the presenter and dialog setup.

retailcoder · 2020-03-28T20:35:38Z

Rubberduck.Refactorings/MoveToFolder/IMoveMultipleToFolderPresenter.cs

+    public interface IMoveMultipleToFolderPresenter : IRefactoringPresenter<MoveMultipleToFolderModel>
+    {}


Marker interface?

Same as above.

retailcoder · 2020-03-28T20:40:43Z

RubberduckTests/Inspections/WriteOnlyPropertyInspectionTests.cs

@@ -74,6 +74,25 @@ End Property
            Assert.AreEqual(0, InspectionResultsForModules(("MyClass", inputCode, ComponentType.ClassModule)).Count());
        }

+        [Test]
+        [Category("Inspections")]
+        public void WriteOnlyProperty_ReturnsResult_GetInOtherModule()


retailcoder

LGTM - just curious about the marker interfaces for the generic presenters, and I think the inline EventHandler delegates in .xaml code-behinds would be better off not-inlined and explicitly de-registered, but if it's not causing any problems it's probably a non-issue.

retailcoder · 2020-03-28T20:46:17Z

This is awesome BTW, well done!

A test was there, but the assert was wrong.

This is done in Rename-, MoveMultipleToFolder- and Move MultipleFoldersView. Also addresses one more review comment to PR rubberduck-vba#5438.

MDoerner added 7 commits March 20, 2020 21:58

Add MoveToFolderRefactoring

8a992ee

Also correctly converts folder annotation arguments to the string content and back. This also removes the parentheses around folders.

Fix WriteOnlyPropertyInspection

213822f

It did not consider that there could be a getter in another module with the same name.

Add MoveContainingFolderRefactoring

7b2b173

It moves the containing folder into another folder. Also fixes ToVbaStringLiteral. (It was missing the surrounding quotes.)

Add tests for MoveToFolderRefactoring itself

50299d8

Also adds members to get the initial model to InteractiveRefactoringTestsBase.

Add tests for MoveContainingFolderRefactoring itself

f17b1f7

There are already tests for the involved refactoring actions.

Introduce IRefactoringUserInteraction

b9e6e70

This moves interactive refactorings from using inheritance to deal with the interaction to composition. Consequently, the interaction can be reused elsewhere.

MDoerner added the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Mar 21, 2020

MDoerner added 2 commits March 22, 2020 22:35

Add move to folder DnD in the CE

6b77f0a

Stop moving components into folders they are already in

9b2fc3b

The exception is moving into the project folder. This simply adds the explicit folder annotation.

MDoerner removed the PR-Status: WIP Pull request is work-in-progress, more commits should be expected. label Mar 22, 2020

MDoerner marked this pull request as ready for review March 22, 2020 22:06

MDoerner added 2 commits March 24, 2020 19:49

Merge branch 'next' into MoveToFolderRefactoring

16dad99

# Conflicts: # Rubberduck.Resources/RubberduckUI.resx

Fix missing usings after merge

68c9f55

retailcoder reviewed Mar 28, 2020

View reviewed changes

Rubberduck.Core/UI/Command/Refactorings/CodePaneRefactorMoveToFolderCommand.cs Show resolved Hide resolved

retailcoder reviewed Mar 28, 2020

View reviewed changes

MDoerner added 2 commits March 29, 2020 11:54

Only report Let or Set in WriteOnlyPropertyInspection

ff1c5f6

A test was there, but the assert was wrong.

Unregister load event handlers in view code behinds

6f0a1d7

This is done in Rename-, MoveMultipleToFolder- and Move MultipleFoldersView. Also addresses one more review comment to PR rubberduck-vba#5438.

retailcoder merged commit 5fec4ad into rubberduck-vba:next Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to Folder Refactoring #5438

Move to Folder Refactoring #5438

MDoerner commented Mar 21, 2020 •

edited

Loading

codecov bot commented Mar 21, 2020 •

edited

Loading

retailcoder Mar 28, 2020

retailcoder Mar 28, 2020

MDoerner Mar 28, 2020

retailcoder Mar 28, 2020

MDoerner Mar 28, 2020

MDoerner Mar 29, 2020

retailcoder Mar 28, 2020

MDoerner Mar 28, 2020

retailcoder Mar 28, 2020

MDoerner Mar 28, 2020

retailcoder Mar 28, 2020

MDoerner Mar 28, 2020

retailcoder Mar 28, 2020

retailcoder left a comment

retailcoder commented Mar 28, 2020

		public interface IMoveMultipleFoldersPresenter : IRefactoringPresenter<MoveMultipleFoldersModel>
		{}

		public interface IMoveMultipleToFolderPresenter : IRefactoringPresenter<MoveMultipleToFolderModel>
		{}

Move to Folder Refactoring #5438

Move to Folder Refactoring #5438

Conversation

MDoerner commented Mar 21, 2020 • edited Loading

codecov bot commented Mar 21, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

retailcoder left a comment

Choose a reason for hiding this comment

retailcoder commented Mar 28, 2020

MDoerner commented Mar 21, 2020 •

edited

Loading

codecov bot commented Mar 21, 2020 •

edited

Loading