Dependency injection #11142

RussKie · 2023-08-12T13:46:07Z

Proposed changes

Introduce the concept of a service container, which gets initialised at the very start of the app. IGitUICommands implements IServiceProvider, which means the service container is available anywhere where an instance of GitUICommands is.
The service container is plumbed in, and MEF removed. However, nothing else has changed at this stage, and further enhancements will be coming as follow up work.

Plumb service provider
Refactor tests
Partial revert of Start using MEF #9107

Merge strategy

❗ Don't squash.

✒️ I contribute this code under The Developer Certificate of Origin.

RussKie · 2023-09-20T12:41:22Z

I'd like to have this merged. If no objections/feedback received, I'll merge this by the end of the week.

GitExtensions/Program.cs

mstv

It seems to work. I do not have principle objections.
The review of the first commit and of Program.cs have raised questions which should be clarified before the complete review.

GitUI/CommandsDialogs/FormBrowse.cs

GitUI/GitUICommands.cs

GitUI/CommandsDialogs/FormClone.cs

GitUI/CommandsDialogs/FormCommit.cs

GitUI/CommandsDialogs/FormMergeSubmodule.cs

GitUI/CommandsDialogs/FormCommit.cs

GitUI/CommandsDialogs/RevisionDiffControl.cs

GitUI/HelperDialogs/FormStatus.cs

GitUI/Script/ScriptRunner.cs

GitExtensions/Program.cs

gerhardol

Not sure what the benefit is with the change.
But I trust the change and have no objections to merge.
Have only review by "pattern matching", i.e. uniform changes. Not run

RussKie · 2023-09-21T01:17:48Z

Not sure what the benefit is with the change.

This is the first step to using dependency injection throughout the application - i.e., avoid newing up instances inplace, and rather create a dependency tree upfront. This will have perf benefits and will make testing much easier.
This will also allow us to make the implementations much more decoupled and modular.

mstv

I took the liberty to fix some comments myself. Could do for ScriptOptionsParser, too.

What do you think about the strict compostion in 09d143c?
I think it is clearer without loss of convenience.

GitUI/Script/ScriptOptionsParser.cs

IntegrationTests/UI.IntegrationTests/Script/ScriptRunnerTests.cs

RussKie · 2023-09-24T07:30:34Z

What do you think about the strict compostion in 09d143c?
I think it is clearer without loss of convenience.

I've gone through several iterations of the API, and I didn't want to expose the SP publicly. To me it feels like an implementation detail which would unnecessarily burden the API surface. That said, I don't consider the API locked, and it's possible/likely a number of API will change as we utilise the DI more.

GitExtensions/Program.cs

GitUI/GitUICommands.cs

GitUI/CommitInfo/CommitInfo.cs

GitUI/CommandsDialogs/FormBrowse.cs

GitUI/Script/ScriptOptionsParser.cs

mstv · 2023-09-24T10:52:01Z

I've gone through several iterations of the API, and I didn't want to expose the SP publicly.

The SP interface is publicly exposed by public interface IGitUICommands : IServiceProvider.
public IServiceProvider ServiceProvider { get; init; } does not expose more than the direct interface implementation.

Public inheritance / interface implementation reads as "UICommands is a/the SP". UICommands is not the SP in the strict sense, it just holds it and makes it accessible.

(Since serviceProvider: UICommands is being eliminated by the factory methods With...(), this discussion is getting minor relevance.)

RussKie · 2023-09-25T08:41:11Z

I started with passing IServiceProvider to every form, since it kind of felt it was a responsibility of a UI component to know how to resolve its dependencies. However, it quickly spiralled out of control, and even though I saw that change through to a compilable state, there were way too many changes. Also, each UI control (well, almost) had a reference to IGitUICommands, which in a way behaves as a "controller" for UI components. So, to minimise the change surface I decided to extend that interface with IServiceProvider.

Exposing IServiceProvider as a property of IGitUICommands doesn't feel right to me, it feels as a leaky abstraction.

I dislike the current implementation with the inheritance and (re-)assignment of IGitUICommandsSource. In the long term I'd like to replace that with IServiceProvider, from which a current instance of IGitUICommands or IGitModule can be resolved. That would allow us to modularise and componentise the app.

mstv · 2023-09-25T18:41:11Z

IntegrationTests/UI.IntegrationTests/CommandsDialogs/FormBrowse.LeftPanelTests.Remotes.cs

+            serviceContainer.AddService(Substitute.For<IAppTitleGenerator>());
+            serviceContainer.AddService(Substitute.For<IWindowsJumpListManager>());
+            serviceContainer.AddService(Substitute.For<ResourceManager.ILinkFactory>());


Indeed, this works without specifying the interface type .AddService<IType>(...).

RussKie · 2023-09-26T05:35:33Z

GitUI/GitUICommands.cs

-        ///  Creates a new instance of <see cref="GitUICommands"/> for a git repository specified by <paramref name="module"/>.
+        /// Creates a new instance of <see cref="GitUICommands"/> for a git repository specified by <paramref name="module"/>.


I've done this on purpose. The double space makes it easier to read comments. We use this convention in a few .NET repos, notably in dotnet/winforms.

The double space makes it easier to read comments.

This is pretty new - and done inconsistently in this PR (at least in Plugins/GitUIPluginInterfaces/ServiceProviderExtensions.cs). I do not have strong opinion on it.

RussKie · 2023-09-26T05:38:57Z

GitUI/CommitInfo/CommitInfo.cs

-                return _commitDataBodyRenderer?.Render(data, showRevisionsAsLinks: false) ?? string.Empty;
+                return _commitDataBodyRenderer.Render(data, showRevisionsAsLinks: false);


I am not 100% convinced that this is safe. The data loading in this control is async, and without any cancelations. So, it's quite possible to get in a state where data is being loaded, and at the same time a repo is being unloaded which sets these variables to null.

You are right, I stopped in the middle of a right movement.

mstv · 2023-09-26T17:38:31Z

GitUI/CommitInfo/CommitInfo.cs

@@ -420,11 +427,9 @@ async Task LoadLinksForRevisionAsync(GitRevision revision, DistributedSettings s

                    string GetLinksForRevision(DistributedSettings settings)
                    {
+                        IEnumerable<ExternalLink> links = _gitRevisionExternalLinksParser.Parse(revision, settings);
                        cancellationToken.ThrowIfCancellationRequested();


Moved down on purpose because the caller has checked for cancellation right before calling this function.

RussKie · 2023-09-28T07:50:40Z

Thank you for the reviews

ghost assigned RussKie Aug 12, 2023

RussKie force-pushed the dependency_injection branch 9 times, most recently from 42ab47f to bf88dfa Compare August 30, 2023 01:55

RussKie closed this Aug 30, 2023

RussKie reopened this Aug 30, 2023

RussKie force-pushed the dependency_injection branch from bf88dfa to 8d78eed Compare August 30, 2023 08:07

RussKie mentioned this pull request Sep 1, 2023

Allow configuring editor for local files #11183

Open

RussKie closed this Sep 4, 2023

RussKie reopened this Sep 4, 2023

RussKie force-pushed the dependency_injection branch from e28381b to aae8ef3 Compare September 4, 2023 10:34

RussKie mentioned this pull request Sep 6, 2023

Make ExecuteAsync cancellable and kill canceled processes #11194

Merged

RussKie force-pushed the dependency_injection branch from 3a4edff to 6f881e8 Compare September 14, 2023 13:43

RussKie marked this pull request as ready for review September 14, 2023 13:46

RussKie force-pushed the dependency_injection branch from 6f881e8 to 7363e9d Compare September 15, 2023 03:28

RussKie mentioned this pull request Sep 19, 2023

LeftPanel: Avoid deadlocks (in tests) #11162

Merged

RussKie force-pushed the dependency_injection branch from 1555c29 to fa71764 Compare September 19, 2023 13:35

RussKie requested review from mstv, gerhardol and pmiossec September 19, 2023 21:39

pmiossec reviewed Sep 20, 2023

View reviewed changes

GitExtensions/Program.cs Outdated Show resolved Hide resolved

mstv reviewed Sep 20, 2023

View reviewed changes

gerhardol reviewed Sep 20, 2023

View reviewed changes

mstv reviewed Sep 21, 2023

View reviewed changes

GitUI/Script/ScriptOptionsParser.cs Outdated Show resolved Hide resolved

IntegrationTests/UI.IntegrationTests/Script/ScriptRunnerTests.cs Outdated Show resolved Hide resolved

RussKie force-pushed the dependency_injection branch from 0a02a1b to 15f19ec Compare September 24, 2023 07:24

RussKie force-pushed the dependency_injection branch from 15f19ec to d31ae17 Compare September 24, 2023 07:47

mstv reviewed Sep 24, 2023

View reviewed changes

RussKie force-pushed the dependency_injection branch from f5ffdb9 to 9078788 Compare September 25, 2023 02:45

Plumb service provider

5d26d0b

RussKie force-pushed the dependency_injection branch from 9078788 to 7ecedd9 Compare September 25, 2023 06:24

mstv force-pushed the dependency_injection branch from 0c81f0a to 89b620e Compare September 25, 2023 18:59

mstv approved these changes Sep 25, 2023

View reviewed changes

RussKie commented Sep 26, 2023

View reviewed changes

mstv reviewed Sep 26, 2023

View reviewed changes

Replace MEF service injection with ServiceProvider

3d98756

RussKie force-pushed the dependency_injection branch from 0b48cb5 to 3d98756 Compare September 28, 2023 03:19

RussKie closed this Sep 28, 2023

RussKie reopened this Sep 28, 2023

RussKie merged commit d725f7f into gitextensions:master Sep 28, 2023

ghost added this to the vNext milestone Sep 28, 2023

RussKie deleted the dependency_injection branch September 28, 2023 07:50

RussKie mentioned this pull request Oct 1, 2023

Plumb service provider to settings pages #11235

Merged

RussKie mentioned this pull request Mar 23, 2024

Restore splitter positions early in the layout process for FormCommit #11646

Merged

		/// Creates a new instance of <see cref="GitUICommands"/> for a git repository specified by <paramref name="module"/>.
		/// Creates a new instance of <see cref="GitUICommands"/> for a git repository specified by <paramref name="module"/>.

		return _commitDataBodyRenderer?.Render(data, showRevisionsAsLinks: false) ?? string.Empty;
		return _commitDataBodyRenderer.Render(data, showRevisionsAsLinks: false);

Uh oh!

Dependency injection #11142

Dependency injection #11142

Uh oh!

Conversation

RussKie commented Aug 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Merge strategy

Uh oh!

RussKie commented Sep 20, 2023

Uh oh!

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gerhardol left a comment

Choose a reason for hiding this comment

Uh oh!

RussKie commented Sep 21, 2023

Uh oh!

mstv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RussKie commented Sep 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mstv commented Sep 24, 2023

Uh oh!

RussKie commented Sep 25, 2023

Uh oh!

mstv Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

RussKie Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

mstv Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

RussKie Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

mstv Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

mstv Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

RussKie commented Sep 28, 2023

Uh oh!

Uh oh!

RussKie commented Aug 12, 2023 •

edited

Loading

RussKie commented Sep 24, 2023 •

edited

Loading