RevisionDiff: Move out non GUI to RevisionDiffController #4112

gerhardol · 2017-10-30T22:14:46Z

On top of #4087 (only last commit specific)

Requested as part of #4106 #4089, #4087, #4092 by @RussKie
The files are still tightly coupled with for instance _revisionGrid shared
Some calls like resetFileToToolStripMenuItem_DropDownOpening would just have resulted in code duplication
The Controller is not testable right now - but t is a start at least
For instance Module must be extracted from revisionGrid, or pass revisiongrid.Module at every call
Path.Combine is not swapped (4 out of 5 calls applicable) could have been replaced but I feel that this limits the readability and just increases code size.

Changes proposed in this pull request:

Refactoring

Screenshots before and after (if PR changes UI):

None

How did I test this code:

Manually

Has been tested on (remove any that don't apply):

GIT 2.14, 2.15
Windows 10

…-browse-artificial-stage Refactoring in master so changes in GitUI/CommandsDialogs/FormBrowse.Designer.cs and GitUI/CommandsDialogs/FormBrowse.cs reverted

Reimplementation after refactoring in master to RevisionDiffThe implementation can currently not refresh the RevisionGrid.

…-browse-artificial-stage

Requested as part of gitextensions#4106 and other The files are still tightly coupled with _revisionGrid shared For instance Module must be extracted from revisionGrid, or pass revisiongrid.Module at every call

This reverts commit 06abed5.

WIP

RussKie

It seems as though the purpose of the controller is unclear to you.
We need the controller to decouple the "thinking" (business logic) from the presentation (UI). This separation provides a number of benefits, such as:

testability - we can verify the implementation
documentation/assurance - this pretty much follows from the previous point - tests allow to capture the requirements and assumptions used during the implementation, and
UI portability or plugability - (in the ideal world) we could use the controller for a completely different UI implementation (e.g. web, wpf, etc). The UI layer provides necessary interaction elements and all the logic behind those elements is abstracted by the controller.

I hope it makes sense.

I've pushed a sample of what I envisaged when suggested moving the logic into the controller. It isn't a complete solution by any means (i.e. missing argument validations, tests etc), but is a good start.

gerhardol · 2017-10-31T19:43:24Z

GitUI/CommandsDialogs/RevisionDiff.cs

@@ -42,6 +43,10 @@ public RevisionDiff()
            DiffText.Font = AppSettings.DiffFont;

            GotFocus += (s, e) => DiffFiles.Focus();
+            Load += (s, e) =>
+            {
+                _revisionDiffController = new RevisionDiffController(Module);


Passing Module object does not make sense, it is not updated when the RevisionDiff Module is updated

Module is not required, I put it as an example.

it is not updated when the RevisionDiff Module is updated

that's a very good point

it is not updated when the RevisionDiff Module is updated

I've checked it, Module always contains the current information when switching between git repos.

The working for was not updated for me.

gerhardol · 2017-10-31T19:46:15Z

GitUI/CommandsDialogs/RevisionDiff.cs

@@ -633,16 +664,6 @@ private void saveAsToolStripMenuItem1_Click(object sender, EventArgs e)
            }
        }

-
-        private bool DeleteSelectedDiffFiles()


OK to remove, just a comment: This was purposely not removed as I want to have Keyboard shortcut here. A refrence from FormBrowse is needed

this PR is concentrating on moving logic out of the UI.
a following PR can concentrate on adding keyboard shortcuts

gerhardol · 2017-10-31T19:49:11Z

GitUI/CommandsDialogs/RevisionDiff.cs

            //Many options have no meaning for artificial commits or submodules
            //Hide the obviously no action options when single selected, handle them in actions if multi select

            // disable items that need exactly one selected item
            bool isExactlyOneItemSelected = DiffFiles.SelectedItems.Count() == 1;
-            var isCombinedDiff = isExactlyOneItemSelected &&
-                DiffFiles.CombinedDiff.Text == DiffFiles.SelectedItemParent;
+            var isCombinedDiff = isExactlyOneItemSelected && DiffFiles.CombinedDiff.Text == DiffFiles.SelectedItemParent;


I consider this to leave too much logic in the GUI layer. This will not separate the layers, just involve two files instead of one.

gerhardol · 2017-10-31T19:50:37Z

GitUI/CommandsDialogs/RevisionDiff.cs

@@ -706,7 +726,6 @@ private void diffEditFileToolStripMenuItem_Click(object sender, EventArgs e)
            //TBD RefreshRevisions();


Some solution to refresh the difflist and revisions is needed here.

gerhardol · 2017-10-31T19:59:29Z

@RussKie I believe the suggestion you implemented is just making the implementation more complicated:

The parameters to the functions are still calculated in the GUI layer, it still needs information about the parameters to pass.
None of the executing logic I moved to the controller is moved. (I only moved those without special parameters, could have done more.)
So two files need to be maintained instead of one. Tests can be added, but they are quite meaningless.

I can live with the changes (except for passing Module), will just not push for such changes.

RussKie · 2017-10-31T21:22:06Z

The parameters to the functions are still calculated in the GUI layer, it still needs information about the parameters to pass.

Ideally the CanShowMenuXxx methods would receive the list of selected revisions and the list of selected items (GitItemStatus). However DiffFiles.SelectedItem returns the last selected items from the list, and I am not totally comfortable with copying this piece of logic into the controller - if one day DiffFiles.SelectedItem changes and starts returning the first item from the list, the RevisionDiffController's implementations may likely become incorrect.
The whole range of things needs changing before the logic is properly decoupled from the UI, it is a long road. But it doesn't mean we shouldn't start moving in that direction.

None of the executing logic I moved to the controller is moved. (I only moved those without special parameters, could have done more.)

As per #4106 (review) I am expecting a refactor to increase maintainability of the new functionality.
The existing functionality can be moved across, but for me this is a secondary objective.

So two files need to be maintained instead of one.

In the current codebase we are maintaining only one file but the price of maintenance is staggering.
Your original proposal couples the controller to UI (by virtue of referencing UI components) which does not make it any more maintainable than the current implementation. And the proposed APIs are not very usable or aesthetic.

Tests can be added, but they are quite meaningless.

I respectfully disagree, tests are anything but meaningless. For example:

ShouldShowMenuCherryPick implementations has a cyclomatic complexity of 6,
ShouldShowMenuResetFile implementations has a cyclomatic complexity of 5,
ShouldShowMenuStage and ShouldShowMenuUnstage - complexity of 4.

This is complex for a simple one/two-line property. It is very hard to reason about these implementations and to make changes.

gerhardol · 2017-10-31T21:59:06Z

What is missing then? openWithDifftoolToolStripMenuItem_DropDownOpening? (Most logic will remain RevisionDiff though.)

I consider the test case meaningless because the logic is still in RevisionDiff. The test is independent of the functionality. I will look at some sample tests though.

RussKie · 2017-10-31T22:30:59Z

because the logic is still in RevisionDiff.

how so? RevisionDiff collects the data and passes it to RevisionDiffController to make decisions

My proposal isn't perfect nor complete, it is meant as a talking point. There is still a room for improvement (i.e. signatures look very similar - can be refactored further).

…how* to RevisionDiffController

gerhardol · 2017-11-04T22:57:07Z

@RussKie I want you to be happy, you (and current/previous maintainers) are doing a great job.
My second priority is getting my changes (especially gerhardol@7234d28#diff-320265b67fc16a7d7a11a99771f9e1a4L121, not even a PR yet). I have other things in my life too...

So I can do what you suggest, but my arguments against doing it the way it is done:

Splitting just CanShow* to Controller will leave dependencies between the CanShow and the "action" required (show blame etc) in two files. The CanShow* is only part of the problem. The implementation looks less complex but is even complexer.
My previous comment was for when all arguments were separate to CanShow* too. Then both the calculation of the arguments and the CanShow* declarations had to be updated at changes. After the change it is still likely that "just" implementation of CanShow*, GetSelectionInfo() and ContextMenuSelectionInfo will need to be updated.
Tests can be added, but they will just confirm the static implementation. Like "x=1; y=1: Assert(x==y)" which adds no value.

Adding tests will take a little longer (but will come), partly because I have not added tests here before, partly because I do not have a plan for good tests, partly because this does not engage me.

gerhardol · 2017-11-07T12:02:30Z

Side note: I have started to write tests, but that is slow...

RussKie · 2017-11-07T12:27:56Z

I think tests can be generialised to something like: - if selected a single revision and a single diff item I can see the following menus and can't see the others - if selected a single revision and a single submodule then... - if selected a single revision and multiple diff items some of which submodules then.... And so on....

…

On 07/11/2017 11:02 PM, "Gerhard Olsson" ***@***.***> wrote: Side note: I have started to write tests, but that is slow... — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#4112 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEMyXnb_spo680Qha4zFh87pEHsdd551ks5s0EbbgaJpZM4QL7qb> .

gerhardol · 2017-11-07T20:32:02Z

There are at least 2^7 combinations so all combinations cannot be tested...
I will think about it.
(So far I have been looking at how to write a test case than what to write. It slows be down some so I rather take this in the background otherwise everything will take like forever.)

RussKie · 2017-11-07T20:58:26Z

If we can cover few main cases it will still be a win

…

On 08/11/2017 7:32 AM, "Gerhard Olsson" ***@***.***> wrote: There are at least 2^7 combinations so all combinations cannot be tested... I will think about it. (So far I have been looking at how to write a test case than what to write. It slows be down some so I rather take this in the background otherwise everything will take like forever.) — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#4112 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AEMyXrSsnyJS7yAwPQJxqUriglFRA4-aks5s0L5EgaJpZM4QL7qb> .

gerhardol added 5 commits October 23, 2017 09:28

Stage/unstage in browse

738e11a

Merge remote-tracking branch 'origine/master' into feature-4087/n4031…

4026ddb

…-browse-artificial-stage Refactoring in master so changes in GitUI/CommandsDialogs/FormBrowse.Designer.cs and GitUI/CommandsDialogs/FormBrowse.cs reverted

Browse Diff: Stage/unstage for artificial commits

9cbf47d

Reimplementation after refactoring in master to RevisionDiffThe implementation can currently not refresh the RevisionGrid.

Merge remote-tracking branch 'origine/master' into feature-4087/n4031…

606975c

…-browse-artificial-stage

RevisionDiff: Move out non GUI to RevisionDiffController

06abed5

Requested as part of gitextensions#4106 and other The files are still tightly coupled with _revisionGrid shared For instance Module must be extracted from revisionGrid, or pass revisiongrid.Module at every call

RussKie self-requested a review October 30, 2017 22:19

RussKie assigned gerhardol and RussKie Oct 30, 2017

RussKie added 2 commits October 31, 2017 17:59

Revert "RevisionDiff: Move out non GUI to RevisionDiffController"

e8e6c22

This reverts commit 06abed5.

Extract show menu logic from RevisionDiff

32d0900

WIP

RussKie requested changes Oct 31, 2017

View reviewed changes

gerhardol commented Oct 31, 2017

View reviewed changes

RussKie and others added 3 commits November 3, 2017 17:14

WIP

e552ed3

Rewrote openWithDifftoolToolStripMenuItem_DropDownOpening, moved CanS…

ef2f607

…how* to RevisionDiffController

logic for diffToolStripSeparator13.Visible

a758687

This was referenced Nov 5, 2017

Various forms: limit menu options for artificial and submodule #4084

Merged

Enhanced view of uncommitted changes in Browse Repository #4031

Closed

gerhardol and others added 2 commits November 6, 2017 22:25

Merge branch 'master' into feature/n4031-refactor-revisiondiffcontroller

23c4aec

Merge branch 'master' into feature/n4031-refactor-revisiondiffcontroller

4ce91fb

RussKie merged commit 0aa2d9d into gitextensions:master Nov 7, 2017

gerhardol deleted the feature/n4031-refactor-revisiondiffcontroller branch November 19, 2017 22:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RevisionDiff: Move out non GUI to RevisionDiffController #4112

RevisionDiff: Move out non GUI to RevisionDiffController #4112

gerhardol commented Oct 30, 2017

RussKie left a comment •

edited

gerhardol Oct 31, 2017

RussKie Oct 31, 2017

RussKie Nov 3, 2017

gerhardol Nov 3, 2017

gerhardol Oct 31, 2017

RussKie Oct 31, 2017

gerhardol Oct 31, 2017

gerhardol Oct 31, 2017

gerhardol commented Oct 31, 2017

RussKie commented Oct 31, 2017

gerhardol commented Oct 31, 2017

RussKie commented Oct 31, 2017

gerhardol commented Nov 4, 2017

gerhardol commented Nov 7, 2017

RussKie commented Nov 7, 2017 via email

gerhardol commented Nov 7, 2017

RussKie commented Nov 7, 2017 via email

		@@ -706,7 +726,6 @@ private void diffEditFileToolStripMenuItem_Click(object sender, EventArgs e)
		//TBD RefreshRevisions();

RevisionDiff: Move out non GUI to RevisionDiffController #4112

RevisionDiff: Move out non GUI to RevisionDiffController #4112

Conversation

gerhardol commented Oct 30, 2017

RussKie left a comment • edited

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

gerhardol commented Oct 31, 2017

RussKie commented Oct 31, 2017

gerhardol commented Oct 31, 2017

RussKie commented Oct 31, 2017

gerhardol commented Nov 4, 2017

gerhardol commented Nov 7, 2017

RussKie commented Nov 7, 2017 via email

gerhardol commented Nov 7, 2017

RussKie commented Nov 7, 2017 via email

RussKie left a comment •

edited