Allow user scripts to operate on selected files

[SlugFiller]

Fixes #9974 Fixes #8769 Alternative to #11183

Proposed changes

• Add Run Scripts option to diff file list, file tree, and commit form file lists
• Add SelectedFiles and LineNumber parameters to script parameters, based on currently selected files in the matching list, and the current selected line in the diff/blame view.
• Add conditional parameters, e.g. {if:SelectedFiles} --files {SelectedFiles}{/if}. The condition is based on whether the parameter is available, e.g. the script was executed from the diff view and any files are selected. Otherwise, the text in between is discarded. Also added {ifnot:option} for the opposite condition/fallback.
• Added ShowInFileList event for showing an option directly in the context menu for file lists, and not in the Run Scripts submenu. Similar to Show in RevisionGrid for the revision grid.

Screenshots

Before

Run Script was only available in the revision grid,

After

Run Script is available in revision grid, diff file list, file tree, and commit file list (staged and unstaged).

Test methodology

• Added a test user script using all the new features (notepad++.exe {if:SelectedFiles}{SelectedFiles} {if:LineNumber}-n{LineNumber}{/if}{/if})
• Ran from the menu
• Ran using a shortcut from multiple locations (revision tree, diff file list, file tree, commit window, diff view, blame view

Test environment(s)

• Git Extensions 33.33.33
• Build 98f366d73cb355b0d677a19edd5c36a57f5bcb6b
• Git 2.42.0.windows.1
• Microsoft Windows NT 10.0.19045.0
• .NET 6.0.22
• DPI 96dpi (no scaling)
• Portable: False
• Microsoft.WindowsDesktop.App Versions

    Microsoft.WindowsDesktop.App 6.0.21 [D:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
    Microsoft.WindowsDesktop.App 6.0.22 [D:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]

Merge strategy

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

---

:black_nib: I contribute this code under The Developer Certificate of Origin.

[RussKie]

I wish you've indicated your desire to work on this so that we could discuss the approach and the implementation.

The script engine is being refactored to enable DI support as well as enable per-repo scripts support (https://github.com/gitextensions/gitextensions/pull/10866). https://github.com/gitextensions/gitextensions/pull/11242 provides the foundation for that work. Please have a look.

• Add Run Scripts option to diff file list, file tree, and commit form file lists

This is a "thin ice" kind of territory. This is what #6135 attempted to do. The issue here is the script can only be executed for an existing file - which works in the context of the Commit form. However, in the context of diff file list and the file tree - it is not always the case. Most of the time those show committed files - i.e., a fixed snapshot in time.

So, the menu must either

• be shown for the current working folder (and the artificial commits), or
• the script has to extract the selected files to a temp location before being executed.

[SlugFiller]

I wish you've indicated your desire to work on this so that we could discuss the approach and the implementation.

The time between me posting this PR and you responding is longer than the time I spent working on it. So even if I posted about it the second I started working, it wouldn't make much of a difference.

Also, I'm not sure what's the recommended forum for "discussing the approach and the implementation"

Please have a look.

If that PR is a priority, and is going to be merged first (It's much newer and larger, so hard to tell), I can pre-emptively rebase on top of it. Of course, it's possible the merge order would be opposite.

Feature-wise, I believe they are orthogonal, and I'm not even sure how much they collide code-wise.

If there's one detail I would add to a refactor, is allowing a script to be set to multiple events, so that Show in RevisionGrid can be converted from a separate boolean to an event. This would also allow splitting ShowInFileList to per-list events.

The issue here is the script can only be executed for an existing file

This is not universally true. For example, Notepad++ would create the file if it doesn't exist. Although, that might not be what the user intends. However, the exact same issue exists for the Edit working directory file option. And also when dragging and dropping a file from the list to another program. So it's not unprecedented/unacceptable behavior.

the script has to extract the selected files to a temp location before being executed

This could be added as an extra option, e.g. {fcSelectedFiles}. This is what TortoiseHG does on drag and drop. But I can definitely say, from direct experience, this is not what a user expects to happen by default.

This is especially the case when you consider neither the Working directory nor Commit index have a File tree view. Navigating through the File tree of the last commit, and doing operations on the files presented there, as they exist in the working directory, is a common flow.

Ergo, it should be a separate option, which can be added in a separate PR.

This is what #6135 attempted to do.

From what I can tell, the primary issue with that one is the same as with #11183 : It implements one specific operation, instead of allowing arbitrary ones. Ergo, this PR is a replacement for both.

[gerhardol]

Also, I'm not sure what's the recommended forum for "discussing the approach and the implementation"

(One of) the existing issues.

The issue here is the script can only be executed for an existing file

This is not universally true. For example, Notepad++ would create the file if it doesn't exist. Although, that might not be what the user intends. However, the exact same issue exists for the Edit working directory file option. And also when dragging and dropping a file from the list to another program. So it's not unprecedented/unacceptable behavior.

The existing behavior is an existing problem. I expect the current file to be changed though, but there have been reports (including drag&drop). Some actions like delete is limited to WorkTree, which occasionally is a limitation. A File tree with WorkTree would be useful (but that is not opposing current file changes as in this PR).

Even if we add script options by default I believe this is a good change. The first change (that could be added by default) is to open the file in VS Code.

Have not looked at it, it is slightly bigger than what can be done quickly.

[mstv]

I think the new file options do not need the prefix "f". "c" stands for the current / checked-out commit in contrast to "s" for the selected commit.

---

If that PR is a priority, and is going to be merged first (It's much newer and larger, so hard to tell), I can pre-emptively rebase on top of it. Of course, it's possible the merge order would be opposite.

The architect's PRs come first. 😉 #10866 is older anyway. Though that draft has a few open points yet. But the DI refactoring should be straight forward.

Feature-wise, I believe they are orthogonal, and I'm not even sure how much they collide code-wise.

I think so, too.

If there's one detail I would add to a refactor, is allowing a script to be set to multiple events, so that Show in RevisionGrid can be converted from a separate boolean to an event. This would also allow splitting ShowInFileList to per-list events.

"Show in RevisionGrid" just means "Do not 'hide' the item in the 'Run script' submenu". This option could be renamed accordingly.

The issue here is the script can only be executed for an existing file

...

I agree to ignore this.

[SlugFiller]

I think the new file options do not need the prefix "f". "c" stands for the current / checked-out commit in contrast to "s" for the selected commit.

And "f" stands for files list. More importantly, the prefix is used, in all 3 cases, to decide whether to query the passed parameter for the relevant detail. For instance, if an option starting with "s" is present in a script, it will query to see which revision is currently selected, and throw an error if none are.

"f" initially behaved the same, but the error was incredibly user-unfriendly (it looked like the program crashed), and there was no clean way to suppress the error if the use of the option was surrounded by a matching {if:. Still, if an option starting with "f" is not present (nor the if form thereof), then a query for the currently selected files is avoided.

"Show in RevisionGrid" just means "Do not 'hide' the item in the 'Run script' submenu". This option could be renamed accordingly.

And ShowInFileList is implemented the same. But as an event. My investigation of the setting's history suggested that this is tech debt: GitUI.Script.ScriptInfo.AddToRevisionGridContextMenu was created along with the first scripts prototype, and GitUI.Script.ScriptInfo.OnEvent was added a couple of months afterwards. Later, when ShowInUserMenuBar was added, it was added as a ScriptEvent, not a new boolean member of GitUI.Script.ScriptInfo.

And IMO, this is the correct way to do it. It doesn't make sense to add an extra boolean for every place an option could appear (or made more prominent than a menu). And C# has HashSet/ISet. WinForm ComboBoxes don't have multiple selection, but that can be worked around. GitUI.Script.ScriptInfo.OnEvent should be an ISet<GitUI.Script.ScriptEvent>, not a GitUI.Script.ScriptEvent. That's just my opinion, though.

[mstv]

The prefix "f" does not have a meaning for the user. There will be no "gSelectedFiles". So it should be omitted.

It doesn't make sense to add an extra boolean for every place an option could appear

I have not suggested anything like this. Up to now, all scripts shall appear in the context menu of the RevisionGrid. AddToRevisionGridContextMenu just controls whether they are added as top level item or in the submenu. I agree ScriptEvent.ShowInFileList scripts should not be shown in the RevisionGrid context menu. But please do not filter for AddToRevisionGridContextMenu! Rather rename this flag.

[SlugFiller]

The prefix "f" does not have a meaning for the user. There will be no "gSelectedFiles". So it should be omitted.

There might be "fs", if there's popular request for running scripts on a read-only temporary copy of a file from a specific revision.

But please do not filter for AddToRevisionGridContextMenu! Rather rename this flag.

I did not understand what you are trying to say, here. Rename it to what, why, in what situation, and what would it accomplish?

For comparison, if multiple events are allowed for a script, it would be possible to split ScriptEvent.ShowInFileList to ScriptEvent.ShowInDiffFileList, ScriptEvent.ShowInFileTree, ScriptEvent.ShowInStagedFiles and ScriptEvent.ShowInUnstagedFiles. Then you could have a script that applies to all 4, but also a script that applies to only one.

This would control whether said script appears in the top level of the context menu for the specific list, or is hidden inside the Run Script sub-menu.

It would also be possible to add something like ScriptEvent.HideInRevisionGrid, so it does not appear even in Run Script sub-menu of the revision list. (Although I'm not sure if the limitation can be applied to hotkeys as well)

[mstv]

The prefix "f" does not have a meaning for the user. There will be no "gSelectedFiles". So it should be omitted.

There might be "fs", if there's popular request for running scripts on a read-only temporary copy of a file from a specific revision.

I do not expect that this will be implemented. If, then rather "w" for "file in workdir" and "t" or "s" for "temp copy of selected file of selected commit". And no prefix for "{LineNumber}" in neither case.

"Show in RevisionGrid" just means "Do not 'hide' the item in the 'Run script' submenu". But please do not filter for AddToRevisionGridContextMenu! Rather rename this flag.

I did not understand what you are trying to say, here. Rename it to what, why, in what situation, and what would it accomplish?

refer to the XMLdoc and the usage of AddToRevisionGridContextMenu i.e. rename it to ShowInRootOfContextMenu or the like

[RussKie]

@SlugFiller I consider #11242 complete to the state where you could consider rebasing on top of it.

RE: prefixes - I don't like the current prefix system and do not see why we need to shorten those. All these "s", "c", "f", "w", etc. feel like nonsense. These are a part of our public API surface, and in my view, the readability of these should be a paramount.

"Show in RevisionGrid" just means "Do not 'hide' the item in the 'Run script' submenu". This option could be renamed accordingly.

And ShowInFileList is implemented the same. But as an event. My investigation of the setting's history suggested that this is tech debt: GitUI.Script.ScriptInfo.AddToRevisionGridContextMenu was created along with the first scripts prototype, and GitUI.Script.ScriptInfo.OnEvent was added a couple of months afterwards. Later, when ShowInUserMenuBar was added, it was added as a ScriptEvent, not a new boolean member of GitUI.Script.ScriptInfo.

A lot of original implementations were likely added in ad-hoc manner, which resulted in... certain inconsistencies. It's my view that we're in a position to re-imagine and re-implement some of these (though we should be consider the upgrade experience).

[SlugFiller]

I tried rebasing, but hit a snag: In the current master, whenever I open the settings window, I get an exception here: https://github.com/gitextensions/gitextensions/blob/a0d8a30c79ba54e5b231786f46d381b96c584955/GitUI/CommandsDialogs/SettingsDialog/Pages/ScriptsSettingsPage.cs#L124

I can push the rebased version anyway, but I feel iffy about pushing a version that I can only test partially.

[SlugFiller]

Pushed anyway, because dotnet test passed, and it's better than a version with conflicts. Changes:

• Rebased
• Removed f prefix
• s and c prefix options will only generate an error if a revision is not selected and they are not discarded by an {if: or {ifnot: option.
• File options will now generate an error if a file is not selected, and they were not discarded by an {if: or {ifnot: option.

[SlugFiller]

Rebased

[RussKie]

One step at a time, the more targeted PR the better, since it's easier to review and merge. You may have noticed my latest PRs are tactical increments working up towards a strategic goal.

Perhaps, as the first step we could generalise how to add the universal script support across different controls using the IScriptHostControl. Once we do this, then we can look into how to add support for passing selected files to the script engine.

How does that sound?

[SlugFiller]

I'll try to make a tiny "Remove IRunScript, replace with IScriptHostControl" PR, and then rebase this PR on top of it.

[SlugFiller]

Okay, I hit a problem. In order to take on execution UserScriptContextMenuExtensions.AddUserScripts needs to take, as a replacement to Action<int> scriptInvoker, an IScriptHostControl, a Control (for Control.FindForm) and a IGitModuleControl (for IGitModuleControl.UICommands). Normally, a control would have all three. CSharp doesn't have first-class intersection types, but this can be achieved using a generic parameter. So far so good.

The issue comes in with RepoObjectsTree. This one takes a whole set of interfaces (e.g. ICheckRefs, and IRevisionGridInfo), that mostly just resolve to RevisionGrid. Since it stores them in variables, the generic parameter workaround is not an option. One of these parameters is the parameter sent to AddUserScripts, this creates an issue.

Possible solutions:

1. Have UserScriptContextMenuExtensions.AddUserScripts take multiple parameters (IScriptHostControl, Control, IGitModuleControl. Alternately: IScriptHostControl, GitUICommands, GitModuleForm) as well
2. Make RepoObjectsTree generic, and clean up the extra variables. (Alternative: Make it generic only in the parameter sent to AddUserScripts)
3. Make RepoObjectsTree not directly call AddUserScripts, and instead, pass an Action<ContextMenuStrip, ToolStripMenuItem> that does the work
4. Have RepoObjectsTree just have a RevisionGrid variable. It isn't used anywhere else, after all.
5. Have RepoObjectsTree implement IScriptHostControl, acting purely as a proxy, since it's already a Control and an IGitModuleControl. Then have it pass this.
6. Have UserScriptContextMenuExtensions.AddUserScripts take a single IScriptHostControl, but have it use as to cast it to Control and IGitModuleControl as necessary.
7. Have both just take an Action<int>. The result is basically the same as IRunScript, but with a different name.

@RussKie Which option do you think is best?

[SlugFiller]

For the time being I implemented option 5 in #11273. Can easily change to the other options later.

[gerhardol]

For the time being I implemented option 5 in #11273. Can easily change to the other options later.

ROT usage was separated from IScriptHostControl to IRevisionGridInfo in #11025. Adding IScriptHostControl again should not be a problem.

[SlugFiller]

If you read the discussion in that PR, I've already implemented a better solution, where IScriptHostControl carries the necessary parameters, avoiding the need to pass them another way.

I've also been able to perform additional improvements to that solution, since IScriptsRunner never really needed a combined IGitModuleForm, IWin32Window. In fact, it never needed IGitModuleForm at all, only its IGitUICommands UICommands. So passing both an IWin32Window and an IGitUICommands as properties inside IScriptHostControl removes the need for that whole song and dance.

The final version is now a lot cleaner, far less hacky, and is simply waiting for the merge of #11278, #11279 and #11273.

As an aside, this became one tall PR stack. Can't wait for them to be reviewed and, hopefully, merged.

[gerhardol]

As an aside, this became one tall PR stack. Can't wait for them to be reviewed and, hopefully, merged.

@mstv need support in motivating the first PR there #11278 to motivate why those changes are needed So you can assist to (I would also like to see this go forward, but I cannot supply the motivation...)

[RussKie]

The MVVM stuff is contentious, and it'd likely take sometime for the architecture and design to settle down. I don't think the functionality you're so patiently trying to implement requires MVVM. Can you base your work off the master instead please?

[SlugFiller]

The MVVM might not be necessary, but #11279 is necessary, because passing a method around instead of just using an event makes for all sort of complications. See here

What I could do is base #11279 on a regular event instead of MVVM. But I'm not sure about the etiquette of picking someone else's PR and creating a rebase/alternative of it.

[mstv]

#11279 is based on and requires #11278 but not the undesired MVVM toolkit. I am going to replace #11278 (latest tomorrow).

[SlugFiller]

I can't figure out why AppVeyor is failing. It's not specifying a failed test. Running dotnet test locally causes some complaints about AsyncLoader, but the tests are all passing. I assume the issue is with the changes to FormCommit, but can't figure out what needs to be different

[RussKie]

The CI fails with the timeout, very likely there's a popup during the test execution which blocks the test runner. The popup is likely to do with something not working correctly.

causes some complaints about AsyncLoader,

What kind of complaints?

On Thu, 2 Nov 2023 at 22:57, SlugFiller @.***> wrote:

I can't figure out why AppVeyor is failing. It's not specifying a failed test. Running dotnet test locally causes some complaints about AsyncLoader, but the tests are all passing. I assume the issue is with the changes to FormCommit, but can't figure out what needs to be different

— Reply to this email directly, view it on GitHub https://github.com/gitextensions/gitextensions/pull/11239#issuecomment-1790595149, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABBTEXQWN6OKUVY445QYQJLYCODCNAVCNFSM6AAAAAA5RALB46VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJQGU4TKMJUHE . You are receiving this because you were mentioned.Message ID: @.***>

[SlugFiller]

Hmm. Managed to reproduce it. If I open the commit dialog, run a script on a file that opens a Process window (Doesn't happen if done on a background script, or a script that fails to parse arguments), then close the dialog, I get Cannot access a disposed object. Object name: 'AsyncLoader'.

The script I'm testing with, that triggers the problem, is cmd.exe /c echo {cHash}. For completeness sake, I also tested with cmd.exe /c echo Hello. The issue occurs when it runs in foreground mode, and doesn't happen when it runs in background mode.

Now I need to figure out why that causes an issue.

[SlugFiller]

Rebased to use IScriptOptionsProvider. Needed to de-unify IScriptOptionsProvider from GitModuleControl, so the commit dialog can provide options based on whether the menu was opened (or hotkey was pressed) from the staged or unstaged file list (or a different control).

[pmiossec]

FYI, have a look at this comment (because you have conflicts on .Designer.cs in some your PRs): https://github.com/gitextensions/gitextensions/pull/11345#issuecomment-1806788072

[SlugFiller]

Do hotkeys assigned to scripts, work in the tabs "Diff" & "File tree"?

Yes, this is what the changes to GitModuleControl (and GitModuleForm) are for. When a hotkey is pressed, ExecuteCommand is executed on the currently focused control. The modified code then looks up the control tree to find the nearest script options provider, which allows a child control (diff view, diff list) get the file list from a parent control (diff tab).

[SlugFiller]

Okay, rebasing started being a mess, with commits reverting previous commits, forcing a double merge of the same changes in both directions. So I squashed it. The rebased version matches the last commit, so last changes can still be easily viewed.

[SlugFiller]

Had to rebase again due to conflict (Minor change in an deleted line)