FormBrowse: Add all supported shells to the toolbar #8213

pmiossec · 2020-06-11T10:50:04Z

Fixes #6460

Proposed changes

Being able to launch console of other shells (powershell, cmd, pwsh)

Screenshots

Before

After

Test methodology

Manual

Test environment(s)

Git Extensions 33.33.33
Build 9bf0b97
Git 2.25.0.windows.1 (recommended: 2.25.1 or later)
Microsoft Windows NT 6.2.9200.0
.NET Framework 4.8.4180.0
DPI 168dpi (175% scaling)

codecov · 2020-06-11T11:14:51Z

Codecov Report

Merging #8213 into master will increase coverage by 0.02%.
The diff coverage is 44.89%.

@@            Coverage Diff             @@
##           master    #8213      +/-   ##
==========================================
+ Coverage   52.63%   52.65%   +0.02%     
==========================================
  Files         857      863       +6     
  Lines       62464    62503      +39     
  Branches    11185    11189       +4     
==========================================
+ Hits        32875    32910      +35     
+ Misses      26969    26966       -3     
- Partials     2620     2627       +7

Flag	Coverage Δ
#production	`40.65% <44.89%> (+0.06%)`	⬆️
#tests	`94.45% <ø> (-0.01%)`	⬇️

RussKie · 2020-06-11T11:33:49Z

Nice!
Do you think we could get different icons for each?

GitUI/ShellHelper.cs

GitUI/CommandsDialogs/FormBrowse.cs

gerhardol

ok

GitUI/CommandsDialogs/FormBrowse.cs

pmiossec · 2020-06-12T10:50:52Z

Still need to address messagebox comments but here are custom shell icons... (with updated screenshot)

RussKie · 2020-06-12T11:05:22Z

Looks awesome!

pmiossec · 2020-06-12T14:10:20Z

Message boxes stuff done (with some not translated more strings...)

GitUI/CommandsDialogs/FormBrowse.cs

GitUI/MessageBoxes.cs

RussKie · 2020-06-14T12:15:39Z

The existing code that deals with shells is starting to rot quickly, turning into a mess that we had with difftools resolution.
I took a liberty and refactored the code to do the following:

Break out each shell into own type that defines shell-specific definitions and settings
Refactor and rework FormBrowse and settings screen to use the new shell definitions
Rework how ConEmu terminal changes folders
Remove GitModule.RunBash()

I also made a change that caused the toolbar button to match the shell selection for the ConEmu. I think it fits into the mental model, but the setting location is now in the "wtf?" territory. So I'm somewhat conflicted on this change. We can revert it, if you don't think it is worthwhile change.

Tests are TBD, happy to help (I had only so much time).

RussKie · 2020-06-14T12:40:58Z

Something I didn't get to, but would be good to have - make AppSettings.ConEmuTerminal.ValueOrDefault of type ShellType (represented as int) instead of string. This would allow removal string comparison routines, and remove few allocations.

RussKie · 2020-06-17T12:56:09Z

@pmiossec I'd like to take this into 3.4.2. I have rebased the branch (+1 commit) on to release/3.4 but I was reluctant to push it into your fork. You can see it here - https://github.com/RussKie/gitextensions/tree/launch_shell_consoles

pmiossec · 2020-06-17T23:32:59Z

@RussKie

I'd like to take this into 3.4.2.

Tested. It seems to work well.

Not reviewed because there are a lot of changes and I have to take the time to understand it.
But it seems that the behavior when bash is not found has been changed. On purpose?
Before: a message in the "cmd" console open to tell that bash is not found.
After: (I'm not sure) nothing happens.

I have rebased the branch (+1 commit) on to release/3.4 but I was reluctant to push it into your fork

Go if that's the way forward ;)

RussKie · 2020-06-18T01:38:18Z

Not reviewed because there are a lot of changes and I have to take the time to understand it.

Fair point.
It is easier to review commit-by-commit. The general idea is that each shell is implemented as own type (btw 37b2301, 88ea6d2 and df15749 should be squashed for ease of review).

I have also initially changed how the toolbar item was bound to the same shell selected by a user as a ConEmu default shell. But in the last commit e3c0ed4 I've undone that, as it didn't feel completely coherent with the rest of the change.

But it seems that the behavior when bash is not found has been changed. On purpose?
Before: a message in the "cmd" console open to tell that bash is not found.

I tried to account for a fact that a user may not have all shells available, and only bind shells that user had.
Entirely possible that I may missed some edge case here...

pmiossec

@RussKie Some discussions ;)

pmiossec · 2020-06-18T11:42:50Z

GitUI/GitUICommands.cs

@@ -1505,9 +1505,6 @@ private void RunCommandBasedOnArgument(IReadOnlyList<string> args, IReadOnlyDict
                case "formatpatch":
                    StartFormatPatchDialog();
                    return;
-                case "gitbash":


You removed a feature from the command line.
Please confirm that it is on purpose because I admit that it should not be often used.
If yes, we have to remove the help line in FormCommandlineHelp.resx

Yes, I did remove it, as I removed GitModule.RunBash() in 14690b0 as it duplicates the functionality of running bash that we have in FormBrowse in a way that is excessive.

Ok. So I pushed the fixup...

I think I got why you removed GitModule.RunBash, @RussKie. But dropping a CLI in this PR feels like a submarine.
It breaks the usecase: bash and e.g. grep not in the PATH, but gitex.cmd in the PATH.
So gitex gitbash should open a bash.

I mulled over it, and you're probably right - it's a "dot" release, and this could be a breaking behaviour... we can drop 14690b0 from this PR, but I intent do it in the master.

It breaks the usecase: bash and e.g. grep not in the PATH, but gitex.cmd in the PATH.
So gitex gitbash should open a bash.

If bash is not in the path, then we can't open it. Trying to open bash via the app feels wrong.

This removal of the CLI command "gitbash" must be mentioned in the change log.

GitUI/CommandsDialogs/FormBrowse.cs

pmiossec · 2020-06-18T12:06:33Z

GitUI/Shells/ShellProvider.cs

+                    return CmdShellInstance ?? (CmdShellInstance = new CmdShell());
+
+                case "bash":
+                default:


A shellType not found is transformed in a git-bash. Strange...

GitUI/Shells/ShellDescriptor.cs

pmiossec · 2020-06-18T12:28:14Z

GitUI/Shells/ShellProvider.cs

+
+        public IShellDescriptor GetShell(string shellType)
+        {
+            switch (shellType.ToLowerInvariant())


I'm not 100% sold by the introduction of `ShellType' that:

seems redundant with names

makes (a little) more difficult to add a new shell because you should update 2 places (ShellType and here) and there is no help for the developer if there is a mismatch

The only advantage is to be able to so ShellType.Bash but for this special case, a const could be used.

I will push a commit with a draft in another branch https://github.com/pmiossec/gitextensions/tree/refactor_shells
Please, have a look....

The idea with the enum was to convert the setting to the enum. But your version offers probably equally good alternative.
I left a comment there.

RussKie · 2020-06-21T11:14:44Z

Given there are few unresolved and outstanding issues, I'm not going to include this into 3.4.2. On the surface it looked to me as something relatively simple, but few changes may be breaking, thus not fit for a dot release.

mstv · 2020-07-04T20:11:01Z

GitUI/GitUICommands.cs

@@ -1505,9 +1505,6 @@ private void RunCommandBasedOnArgument(IReadOnlyList<string> args, IReadOnlyDict
                case "formatpatch":
                    StartFormatPatchDialog();
                    return;
-                case "gitbash":


This removal of the CLI command "gitbash" must be mentioned in the change log.

GitUI/Shells/BashShell.cs

GitUI/Shells/ConEmuControlExtensions.cs

GitUI/MessageBoxes.cs

GitUI/Shells/ShellProvider.cs

GitUI/Shells/ShellType.cs

mstv

Should I prepare a commit with my requested changes to MessageBoxes, @pmiossec?

GitUI/CommandsDialogs/FormBrowse.cs

pmiossec · 2020-07-05T20:52:40Z

Should I prepare a commit with my requested changes to MessageBoxes, @pmiossec?

Yes, it would be great!

GitUI/Shells/IShellDescriptor.cs

GitUI/Shells/ConEmuControlExtensions.cs

GitUI/Shells/IShellDescriptor.cs

RussKie · 2020-07-08T12:54:50Z

Squash and merge at will

and - Fix some MessageBox translations - Refactor shell type system * Break out each shell into own type that defines shell-specific defintions and settings * Refactor and rework FormBrowse and settings screen to use the new shell definitions * Rework how ConEmu terminal changes folders * Refactor how shells are intialised: Remove Lazy construct, as shells are initialised and iterated over when FormBrowse is loaded anyway. Co-authored-by: RussKie <russkie@gmail.com>

mstv · 2020-07-09T22:03:45Z

@msftbot merge in 1 day

ghost · 2020-07-09T22:03:48Z

Hello @mstv!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Fri, 10 Jul 2020 22:03:47 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

ghost assigned pmiossec Jun 11, 2020

pmiossec force-pushed the launch_shell_consoles branch from 9bf0b97 to 5fc71bb Compare June 11, 2020 10:58

RussKie reviewed Jun 11, 2020

View reviewed changes

GitUI/ShellHelper.cs Outdated Show resolved Hide resolved

GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved

gerhardol reviewed Jun 11, 2020

View reviewed changes

mstv reviewed Jun 11, 2020

View reviewed changes

GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved

GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved

pmiossec force-pushed the launch_shell_consoles branch from 5fc71bb to 92cd410 Compare June 12, 2020 10:46

mstv reviewed Jun 12, 2020

View reviewed changes

GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved

GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved

GitUI/MessageBoxes.cs Outdated Show resolved Hide resolved

pmiossec force-pushed the launch_shell_consoles branch 2 times, most recently from e5b028a to 82be5ce Compare June 13, 2020 23:19

RussKie added this to the 3.4.2 milestone Jun 17, 2020

mstv changed the base branch from master to release/3.4 June 17, 2020 22:16

mstv changed the base branch from release/3.4 to master June 17, 2020 22:18

pmiossec force-pushed the launch_shell_consoles branch from ee87f14 to e3c0ed4 Compare June 17, 2020 23:34

pmiossec changed the base branch from master to release/3.4 June 17, 2020 23:35

pmiossec closed this Jun 17, 2020

pmiossec reopened this Jun 17, 2020

RussKie self-assigned this Jun 18, 2020

pmiossec force-pushed the launch_shell_consoles branch from 81bde4e to 14690b0 Compare June 18, 2020 11:31

pmiossec commented Jun 18, 2020

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jun 19, 2020

RussKie modified the milestones: 3.4.2, 4.0 Jun 21, 2020

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Jul 3, 2020

RussKie approved these changes Jul 4, 2020

View reviewed changes

mstv reviewed Jul 4, 2020

View reviewed changes

mstv reviewed Jul 5, 2020

View reviewed changes

GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved

GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved

mstv mentioned this pull request Jul 5, 2020

Refactor MessageBoxes #8295

Merged

mstv requested a review from RussKie July 5, 2020 22:33

mstv force-pushed the launch_shell_consoles branch from cc73a04 to 1062b46 Compare July 5, 2020 22:45

RussKie reviewed Jul 7, 2020

View reviewed changes

GitUI/Shells/IShellDescriptor.cs Outdated Show resolved Hide resolved

GitUI/Shells/ConEmuControlExtensions.cs Outdated Show resolved Hide resolved

GitUI/Shells/IShellDescriptor.cs Show resolved Hide resolved

mstv force-pushed the launch_shell_consoles branch from 269fb09 to 991f372 Compare July 9, 2020 22:00

ghost added the status: auto merge label Jul 9, 2020

mstv changed the base branch from release/3.4 to master July 9, 2020 22:10

Remove GitModule.RunBash()

4b3609f

mstv force-pushed the launch_shell_consoles branch from 991f372 to 4b3609f Compare July 9, 2020 22:21

RussKie merged commit d4b3e13 into gitextensions:master Jul 10, 2020

RussKie mentioned this pull request Jul 12, 2020

Introduce a new console styles settings page under Appearance #8307

Merged

pmiossec deleted the launch_shell_consoles branch July 13, 2020 14:08

This was referenced Jul 26, 2021

Support git-bash.exe again #9416

Merged

Git executable not found in Console #9353

Closed

BashShell: Try git-bash.exe first #9430

Merged

gerhardol mentioned this pull request Aug 10, 2021

ConEmu require ExecutableCommandLine to be set #9474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FormBrowse: Add all supported shells to the toolbar #8213

FormBrowse: Add all supported shells to the toolbar #8213

pmiossec commented Jun 11, 2020 •

edited

codecov bot commented Jun 11, 2020 •

edited

RussKie commented Jun 11, 2020

gerhardol left a comment

pmiossec commented Jun 12, 2020

RussKie commented Jun 12, 2020 via email •

edited by ghost

pmiossec commented Jun 12, 2020

RussKie commented Jun 14, 2020

RussKie commented Jun 14, 2020

RussKie commented Jun 17, 2020

pmiossec commented Jun 17, 2020

RussKie commented Jun 18, 2020

pmiossec left a comment

pmiossec Jun 18, 2020

RussKie Jun 18, 2020

pmiossec Jun 18, 2020

mstv Jun 18, 2020

RussKie Jun 19, 2020 •

edited

mstv Jul 4, 2020

pmiossec Jun 18, 2020

pmiossec Jun 18, 2020

RussKie Jun 19, 2020

RussKie commented Jun 21, 2020

mstv Jul 4, 2020

mstv left a comment

pmiossec commented Jul 5, 2020

RussKie commented Jul 8, 2020

mstv commented Jul 9, 2020

ghost commented Jul 9, 2020

FormBrowse: Add all supported shells to the toolbar #8213

FormBrowse: Add all supported shells to the toolbar #8213

Conversation

pmiossec commented Jun 11, 2020 • edited

Proposed changes

Screenshots

Before

After

Test methodology

Test environment(s)

codecov bot commented Jun 11, 2020 • edited

Codecov Report

RussKie commented Jun 11, 2020

gerhardol left a comment

Choose a reason for hiding this comment

pmiossec commented Jun 12, 2020

RussKie commented Jun 12, 2020 via email • edited by ghost

pmiossec commented Jun 12, 2020

RussKie commented Jun 14, 2020

RussKie commented Jun 14, 2020

RussKie commented Jun 17, 2020

pmiossec commented Jun 17, 2020

RussKie commented Jun 18, 2020

pmiossec left a 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

RussKie Jun 19, 2020 • 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

RussKie commented Jun 21, 2020

Choose a reason for hiding this comment

mstv left a comment

Choose a reason for hiding this comment

pmiossec commented Jul 5, 2020

RussKie commented Jul 8, 2020

mstv commented Jul 9, 2020

ghost commented Jul 9, 2020

pmiossec commented Jun 11, 2020 •

edited

codecov bot commented Jun 11, 2020 •

edited

RussKie commented Jun 12, 2020 via email •

edited by ghost

RussKie Jun 19, 2020 •

edited