Skip to content

Conversation

mstv
Copy link
Member

@mstv mstv commented Nov 1, 2023

Fixes: preparation for git output / history control

Proposed changes

  • Ensure that FormStatus has a GitUICommands instance.
  • Pass GitUICommands instance to FormStatus when running scripts by downcasting from IGitUICommands to GitUICommands
  • Make GitUICommands instance mandatory for FormStatus & FormProcess

Screenshots

N/A

Test methodology

  • existing tests

Merge strategy

I agree that the maintainer squash merge this PR (if the commit message is clear).


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

Comment on lines 22 to 23
private FormProcess(IServiceProvider serviceProvider,
GitUICommands? commands,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we doing this? GitUICommands is a service provider

Copy link
Member Author

@mstv mstv Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commands is optional (can be null), serviceProvider is mandatory.

This ctor is private.

@RussKie
Copy link
Member

RussKie commented Nov 2, 2023 via email

@mstv
Copy link
Member Author

mstv commented Nov 2, 2023

Then we just need to make mandatory

        protected GitModuleForm(GitUICommands? commands, bool enablePositionRestore)
            : base(enablePositionRestore)
        {
            if (commands is null && _uiCommands is null)
            {
                // In some cases we may want to initialise a form without a module,
                // e.g. a process dialog that executes a git command which is completely self contained.
            }

For making UICommands mandatory, we would need to switch to IGitUICommands instead of GitUICommands...
and YAGNI.

@RussKie
Copy link
Member

RussKie commented Nov 2, 2023 via email

@mstv
Copy link
Member Author

mstv commented Nov 2, 2023

I wonder what these "some cases"...

Ask your self from 4 years ago ;-)
// e.g. a process dialog that executes a git command which is completely self contained.
This is true for all direct usages of FormStatus and FormProcess.

I do not see why FormStatus and all its descendants need to be a GitModuleForm. They have never used GitUICommands and should never do so except for IServiceProvider.
Even FormRemoteProcess uses its own property, only in order to access the subproperty Module for get & set settings but none of the many GitUICommands features nor UICommandsChanged, etc.
refer to 2fcac30

Though since an IGitUICommands - and thereby IServiceProvider - instance is already provided everywhere and since a refactoring of the grown inheritance chain will be even more difficult,
the anyway recommendable main topic of #11269 should be pursued.
FormStatus -> GitExtensionsDialog -> GitModuleForm -> GitExtensionsForm -> GitExtensionsFormBase (with TryGetUICommands)
I still feel uncomfortable with this overuse of IGitUICommands.

@RussKie
Copy link
Member

RussKie commented Nov 3, 2023 via email

when running scripts
by downcasting from IGitUICommands to GitUICommands

Make GitUICommands instance mandatory for FormStatus & FormProcess
@mstv
Copy link
Member Author

mstv commented Nov 4, 2023

I have tried the naive approach to use the completed IGitUICommands instead of GitUICommands - as a quick first step ... I thought.
It will not work without completing IGitModule, too.
Raise event functions would need to be declared public: db5c181.
Several types would need to be moved to GitUIPluginInterfaces: 796cc11.

So I wait for #11269 and the follow-ups.

For the meantime, I am downcasting IGitUICommands to GitUICommands for calling FormProcess.ShowDialog in RunScriptInternal.

@RussKie
Copy link
Member

RussKie commented Nov 4, 2023 via email

@mstv mstv merged commit 581ca00 into gitextensions:master Nov 4, 2023
@mstv mstv deleted the di_formstatus branch November 4, 2023 23:30
@ghost ghost added this to the vNext milestone Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants