Add support for {SelectedFiles} to toolbar event scripts #11734

mdonatas · 2024-05-10T09:44:01Z

Proposed changes

Once I've noticed this PR #11239 I wanted to use {SelectedFiles} in my script but it didn't work, I got literal value of {SelectedFiles} as a result.
Question: Is it a bug to leave {SelectedFiles} unreplaced into say an empty string?

Since Toolbar Event Scripts go through FormBrowse when looking for a IScriptOptionsProvider, FormBrowse conveniently has two potential sources for it
Override GetScriptOptionsProvider() in FormBrowse to forward this request to RevisionDiffControl or RevisionFileTreeControl depending on which one is visible

Test methodology

Manually. I've been using this feature for ~2 weeks almost daily

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.

mstv · 2024-05-10T10:28:58Z

I am using something similar: 00d0c84.
But I rather want {SelectedFiles} to not be supported from the revision grid, i.e. only from the panels Diff and File Tree.
Use case: Open the repo folder in VS Code from revision grid but not just a single file unless in a file context - and then at the current position in the file.

Only the first part of the feature has been merged yet (after waiting too long for additional reviews). The plan is to add #11342. It will give full control.

mdonatas · 2024-05-10T10:36:10Z

But I rather want {SelectedFiles} to not be supported from the revision grid, i.e. only from the panels Diff and File Tree.

Are you describing your needs/workflow or vision for how it should be implemented in GE? For my scenario I very much need {SelectedFiles} from wherever.
edit: I think my scenario would also work with your code.

Specific case: I find it very helpful to open VS/Rider right from GE, usually GE is the first thing I open and IDE follows from there. There is this one repo which has many solutions, so naturally to open the right one a script needs more context e.g. selected files.

RussKie · 2024-05-11T11:20:20Z

The "selected files" in the revision grid make no sense - the revision grid only carries the context of revisions, "files" is the context carried by the revision tree and the diff tree controls. Also, more importantly - the "selected files" are generally applicable to the working tree only - i.e., the real files that exist on the hard drive. A commit is a snapshot in time, and files don't exist on a hard drive unless that commit is checked out. It's kind of wrong to think that you select an arbitrary file in an arbitrary commit and work on that file - you don't.

#6135 is one of the primary use-cases for the "selected files".

IMO your scripts could/should be rewritten in a way where they don't require you pre-selecting some file in either the revision or the diff trees but rather resolve/search required files.
In fact, your script looks like doing a lot of extra work where you could just open VS by invoking the following command:

PS> start @(Get-ChildItem -Filter *.sln)[0]
# or
PS> start (Get-ChildItem -Filter *.sln | Select-Object -First 1)

mdonatas · 2024-05-11T18:04:31Z

The "selected files" in the revision grid make no sense

That could be the case if one is strict in a way the math is strict. But then how come it does me wonders in a repo with 60+ solutions where every single time I've used my script to open a relevant (based on diff/tree file selection) solution it worked as expected.

My SolutionFinder thingy works when nothing is selected too, does basically the same as suggested PS script but in C#.

If the script is capable of handling non-existent files, why should GE be the judge of whether what the script author tries to achieve does or doesn't make sense? There is a valid subset of scripts that are interested in selected files of a commit and are not impacted by whether these files are on the disk or not.

I'll be fine as I'm always running a custom build but my colleagues won't be.

RussKie · 2024-05-12T04:26:12Z

If the script is capable of handling non-existent files, why should GE be the judge of whether what the script author tries to achieve does or doesn't make sense? There is a valid subset of scripts that are interested in selected files of a commit and are not impacted by whether these files are on the disk or not.

I'll be fine as I'm always running a custom build but my colleagues won't be.

My point here is that a lot (and I mean a lot) of users do not very well understand the difference between a commit and a working directory; and we'd be doing a disservice to our users further reinforcing this misconception by allowing the "selected files" anywhere but for the working directory. As well as in the revision grid which has no concept of "files".

. . .

On a slightly unrelated topic, could you please elaborate on your workflow you're trying to achieve here? Are you selecting a file in a diff/revision tree then select a commit in the revision grid to execute a script? If so, this feels back to front, but I probably completely misunderstand your flow.

mdonatas · 2024-05-12T07:36:38Z

A workflow which requires {SelectedFiles}:
TLDR: Looking at a diff -> oh this is interesting -> toolbar script click to open relevant sln in IDE.

There is a repo with 60+ solutions in it. Now I am either doing a PR review and choose to do it in GE or I am in this repo looking at the work others have done. Naturally this involves looking at a diff and if I want to see the code in an IDE I click the Rider/VS button in the toolbar. My SolutionFinder.exe looks for sln files closest to passed in files (or from the root of the repo if no files are passed. Without {SelectedFiles} I get the first sln opened which is never the one I want.. and I want the one I was looking at in the diff tab.

Maybe then this variable should be called {SelectedCommitBlobPaths} (https://git-scm.com/book/id/v2/Git-Internals-Git-Objects)

Tree Objects

The next type we’ll look at is the tree, which solves the problem of storing the filename and also allows you to store a group of files together. Git stores content in a manner similar to a UNIX filesystem, but a bit simplified. All the content is stored as tree and blob objects, with trees corresponding to UNIX directory entries and blobs corresponding more or less to inodes or file contents.

mdonatas · 2024-05-12T07:56:45Z

It's my turn to be confused 😅 given all you've said about users getting confused with files/blobs not being the same and that this only makes sense for files in a working dir.... then how come this works just fine (without changes in this PR):

A script with {SelectedFiles} triggered by ShowInFileList

Script executed on a blob in a stash!

With paths resolved just fine and passed into VS Code

So the only changes this PR introduce is that before one HAD to right-click a file(s) to reach the (menu)button but with these changes one can also have the same script run in ShowInUserMenuBar and have the exact same behavior.

edit: Am I right to assume (based on comments above) that you thought this wouldn't work on non-working-dir commits?

mdonatas · 2024-05-12T08:08:10Z

Another idea which does not require a WorkingDir

The possibilities are numerous and again doing this from a context menu works for all revs today, I only want to allow for more convenience for those who prefer to click a button on a toolbar. Just like my actual use-case, a script could already work from a context menu but since I already have a toolbar button why not enable it with this extended scenario.

mdonatas · 2024-05-12T09:05:14Z

To drive my point home 😅

Triggering a script via a hotkey (which works globally) or a toolbar button (which is also globally available) are so similar as they only differ in user's preference. This to me is a good argument to enable {SelectedFiles} via ShowInUserMenuBar trigger.

RussKie · 2024-05-12T10:30:29Z

then how come this works just fine (without changes in this PR)

The scripOptionProvider resolves the selected file as it designed.

However, does that file exist in the working directory? The answer is "it depends" - the snapshot may contain a file with the same name (the content could be wildly different) or it may not (because the file was deleted in subsequent commits). This is what is confusing for a lot of users - they select a random commit, pick a file in the diff/revision tree, see the file's content and think that file is available to them on the hard drive when they execute a script against that file. The script will be executed against the file that exists in the working directory (if it exists).

So, the variables exposing "files" and the ability to execute scripts should only be constrained to the two artificial commits and the HEAD because this is where the file actually exist (with few corner cases, such as a file is being deleted and staged).

RussKie · 2024-05-12T10:34:23Z

To drive my point home 😅

Users are creative bunch, and they find amazing ways to use and abuse SDKs, frameworks and extensibility models provided to them. It doesn't mean we should make it free-for-all, more often than not the users have to be guarded against themselves.

mdonatas · 2024-05-12T11:01:51Z

I am not sure if we are not talking passed each other.

So, the variables exposing "files" and the ability to execute scripts should only be constrained to the two artificial commits and the HEAD because this is where the file actually exist (with few corner cases, such as a file is being deleted and staged).

Does this mean there are plans to remove/restrict functionality in the future? To make it work only on files that ARE on disk and revisions mentioned above? Because right now this works on any file in any revision. And thus I am confused as these comments make it seem as if I am enabling anything more than simply allowing users to click the toolbar button and make use of {SelectedFiles}.

Here I checked-out a commit and executing a script against a deleted file in a previous commit

Users are creative bunch, and they find amazing ways to use and abuse SDKs

I find this comment to be a bit unfair. Adding a hotkey on a script seems reasonable. Then adding that same script as a button for convenience (not everyone likes keyboard shortcuts) doesn't seems to be stretching the paradigm into abuse territory 🤔

gerhardol · 2024-05-12T11:16:10Z

doesn't seems to be stretching the paradigm into abuse territory 🤔

You may know what you are doing, stretching the use.
I do not think that use should be limited but should it be encouraged further?

If you start the action from the difftab, it is slightly easier to understand what the script operates on, there is one less layer in between.
But there are users reporting bugs for difftab operations on the worktree file. For you, changing would be a bug.

(I have not decided myself here.)

mdonatas · 2024-05-12T11:20:31Z

Maybe it would be OK to add {ActiveCommitSelectedBlobPaths} OR better to rename currently misleading {SelectedFiles}

RussKie · 2024-05-12T14:02:18Z

Does this mean there are plans to remove/restrict functionality in the future? To make it work only on files that ARE on disk and revisions mentioned above? Because right now this works on any file in any revision. And thus I am confused as these comments make it seem as if I am enabling anything more than simply allowing users to click the toolbar button and make use of {SelectedFiles}.

I stated my position above - this functionality should only be enabled and available for the artificial and the HEAD commits. If the scripts are allowed to operate on other revisions, then IMO it's a bug and it should be fixed.
This is how the menus should operate IMO:
RussKie@1e8078e?diff=unified&w=1#diff-b7de90d3e40d1fd99c4cda15a14063713af51afece741a6670ee29963a01c8a2R550-R574

. . .

I let @gerhardol @mstv and @pmiossec decide and take or reject this change.

mdonatas · 2024-05-12T15:29:23Z

I think we can take a deep breath and make an effort to have both sides of the argument happy 🙂 🕊️

OK, so an argument for {SelectedFiles} to only be applicable when operating on a HEAD or a few virtual refs are that users find the concepts of files in commits and files on disk confusing. To align a current implementation with this position would require to check whether a script is executed on a valid revision and on an existing file(s).

Then, to make the power-user side of the isle happy we'd need something like {ActiveCommitSelectedBlobPaths} which is much simpler i.e. There are controls which provide blob (file) paths -> here are these paths. The name alone I think is enough to discourage less experienced users to approach it :) I'd also not have their values tied to a physical location, i.e. they should be the way git represents them src/MySolution/SomeFile.cs

And yet with both of these changes ☝️ in my mind, it still makes sense to enable them regardless of how a script is triggered... be it a context menu on a file, a hotkey or a toolbar button.

I am very glad that GE exists, this is the only actual deal-breaker for not using a mac at work, there is no other power-user-centric git GUI out there (at least I failed to find one). So it would really pain me to know that a completely valid feature be neutered because someone doesn't know a working dir from a commit made 7 years ago.
I've wanted to (╯°□°)╯︵ ┻━┻ this PR several times 😅 as I'll be fine either way, but I continue to put effort into this discussion because I think I represent power users such as myself but who don't run their own builds.

mdonatas · 2024-05-12T15:35:29Z

This is how the menus should operate IMO:
RussKie@1e8078e?diff=unified&w=1#diff-b7de90d3e40d1fd99c4cda15a14063713af51afece741a6670ee29963a01c8a2R550-R574

Imagine a script to open a file's specific version on GitHub, this would prevent this and many other valid ideas.

Maybe a script needs an option "Require HEAD revision" which could be checked by default.

gerhardol · 2024-05-12T21:17:58Z

Maybe a script needs an option "Require HEAD revision" which could be checked by default.

Explicit (radioboxes?) "Use worktree version" / "Use selected revision"?

Some features in GE is for power users, all will not be accessible for all users.
But features should be expected and explainable (in not too many words).
For users that write these scripts themselves, it may be OK, but scripts may be included in user settings with repos (I push a settings file with all repos I am involved in at work).

Are files expected to be selected outside of Diff/FileTree tabs?

I stated my position above - this functionality should only be enabled and available for the artificial and the HEAD commits. If the scripts are allowed to operate on other revisions, then IMO it's a bug and it should be fixed.

I expect all commits or not at all.

mstv · 2024-05-12T22:13:45Z

{SelectedBlobPaths} might be a compromise because it avoids the term "Files".
(But I would not introduce a new term "active commit".)

mstv

Anyway, we need #11342 in addition in order to handle the case "no blobs exist / are selected".

mstv · 2024-05-12T22:17:23Z

GitUI/CommandsDialogs/FormBrowse.cs

+ if (fileTree.Visible)
+ {
+ return fileTree.GetScriptOptionsProvider();
+ }
+ else if (revisionDiff.Visible)
+ {
+ return revisionDiff.GetScriptOptionsProvider();
+ }
+
+ return base.GetScriptOptionsProvider();


Suggested change

if (fileTree.Visible)

{

return fileTree.GetScriptOptionsProvider();

}

else if (revisionDiff.Visible)

{

return revisionDiff.GetScriptOptionsProvider();

}

return base.GetScriptOptionsProvider();

return revisionDiff.Visible ? revisionDiff.ScriptOptionsProvider

: fileTree.Visible ? fileTree.ScriptOptionsProvider

: base.GetScriptOptionsProvider();

If it's OK I would like to avoid a nested ternary, at least for me it consumes way too many 🧠 cycles each time determining the order or evaluation and understanding the "what does this mean" without explicit parentheses.

mstv · 2024-05-12T22:18:12Z

GitUI/GitModuleControl.cs

@@ -165,7 +165,7 @@ bool ExecuteScriptCommand()
 }
 }

- protected virtual IScriptOptionsProvider? GetScriptOptionsProvider()
+ public virtual IScriptOptionsProvider? GetScriptOptionsProvider()


This should not be declared public.

mstv · 2024-05-12T22:18:22Z

GitUI/CommandsDialogs/RevisionDiffControl.cs

@@ -189,7 +189,7 @@ bool SelectFirstGroupChangesIfFileNotFocused()
 }
 }

- protected override IScriptOptionsProvider? GetScriptOptionsProvider()
+ public override IScriptOptionsProvider? GetScriptOptionsProvider()


Suggested change

public override IScriptOptionsProvider? GetScriptOptionsProvider()

internal override IScriptOptionsProvider? GetScriptOptionsProvider()

That doesn't work, it's not allowed to change visibility level when overriding. I've changed this to be exactly like in your commit.

mstv · 2024-05-12T22:18:43Z

GitUI/CommandsDialogs/RevisionFileTreeControl.cs

@@ -291,7 +291,7 @@ protected override bool ExecuteCommand(int cmd)
 return true;
 }

- protected override IScriptOptionsProvider? GetScriptOptionsProvider()
+ public override IScriptOptionsProvider? GetScriptOptionsProvider()


Suggested change

public override IScriptOptionsProvider? GetScriptOptionsProvider()

public internal IScriptOptionsProvider? GetScriptOptionsProvider()

mdonatas · 2024-05-19T13:43:01Z

Wanted to gather more input on the approach moving forward.

{SelectedBlobPaths} sounds fine, would we consider having this in addition to {SelectedFiles} or instead of it? I would vote to have just the {SelectedBlobPaths} and have paths relative to the repo root, like what's presented in git output. This would remove some confusion of what it represents and should still enable all the same scenarios as before but it would have to be used in combination with {WorkingDir}.. although it might be more complicated to script when multiple files are selected, then you couldn't just concat {WorkingDir}/{SelectedBlobPaths} right in params field.

Explicit (radioboxes?) "Use worktree version" / "Use selected revision"?

@gerhardol I am not sure I understand what these would mean.
And a broader question of what options or safe-guards do we need to help users help themselves against unexpected situations. In Igor's example a ShowInFileList limits showing script's context-menu item only when HEAD/artificial refs are active. It kinda call for some new script option to become available based on Event type. What could it be? Available on working directory revisions only.. and a broader description at the bottom of the PropertyGrid.

… RevisionFileTreeControl

gerhardol · 2024-05-19T16:06:48Z

@gerhardol I am not sure I understand what these would mean.

I rather have only have {SelectedFiles} and have it operate on either (with radioboxes)

Use worktree version (filter filepaths that is accessible for the selected commit?)
Use selected revision (file blobs, worktree/index to be specially handled, maybe LFS too)

If {SelectedBlobPaths} both types of information is always available (and maybe {SelectedFiles}be renamed to {SelectedWorktreePaths}
{SelectedBlobPaths} maybe need to be both path and blob or some utility function to get the blob/contents.

Add support for {SelectedFiles} to toolbar event scripts

210b3ec

RussKie self-requested a review May 11, 2024 04:54

RussKie removed their request for review May 12, 2024 14:02

mstv reviewed May 12, 2024

View reviewed changes

Expose IScriptOptionsProvider explicitly from RevisionDiffControl and…

e749ecb

… RevisionFileTreeControl

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for {SelectedFiles} to toolbar event scripts #11734

Add support for {SelectedFiles} to toolbar event scripts #11734

mdonatas commented May 10, 2024

mstv commented May 10, 2024

mdonatas commented May 10, 2024 •

edited

RussKie commented May 11, 2024

mdonatas commented May 11, 2024 •

edited

RussKie commented May 12, 2024

mdonatas commented May 12, 2024

mdonatas commented May 12, 2024 •

edited

mdonatas commented May 12, 2024

mdonatas commented May 12, 2024

RussKie commented May 12, 2024

RussKie commented May 12, 2024

mdonatas commented May 12, 2024

gerhardol commented May 12, 2024

mdonatas commented May 12, 2024

RussKie commented May 12, 2024

mdonatas commented May 12, 2024

mdonatas commented May 12, 2024 •

edited

gerhardol commented May 12, 2024

mstv commented May 12, 2024

mstv left a comment

mstv May 12, 2024

mdonatas May 19, 2024

mstv May 12, 2024

mstv May 12, 2024

mdonatas May 19, 2024

mstv May 12, 2024

mdonatas commented May 19, 2024

gerhardol commented May 19, 2024

	public override IScriptOptionsProvider? GetScriptOptionsProvider()
	internal override IScriptOptionsProvider? GetScriptOptionsProvider()

	public override IScriptOptionsProvider? GetScriptOptionsProvider()
	public internal IScriptOptionsProvider? GetScriptOptionsProvider()

Add support for {SelectedFiles} to toolbar event scripts #11734

Are you sure you want to change the base?

Add support for {SelectedFiles} to toolbar event scripts #11734

Conversation

mdonatas commented May 10, 2024

Proposed changes

Test methodology

Merge strategy

mstv commented May 10, 2024

mdonatas commented May 10, 2024 • edited

RussKie commented May 11, 2024

mdonatas commented May 11, 2024 • edited

RussKie commented May 12, 2024

mdonatas commented May 12, 2024

mdonatas commented May 12, 2024 • edited

mdonatas commented May 12, 2024

mdonatas commented May 12, 2024

RussKie commented May 12, 2024

RussKie commented May 12, 2024

mdonatas commented May 12, 2024

gerhardol commented May 12, 2024

mdonatas commented May 12, 2024

RussKie commented May 12, 2024

mdonatas commented May 12, 2024

mdonatas commented May 12, 2024 • edited

gerhardol commented May 12, 2024

mstv commented May 12, 2024

mstv left a comment

Choose a reason for hiding this comment

mstv May 12, 2024

Choose a reason for hiding this comment

mdonatas May 19, 2024

Choose a reason for hiding this comment

mstv May 12, 2024

Choose a reason for hiding this comment

mstv May 12, 2024

Choose a reason for hiding this comment

mdonatas May 19, 2024

Choose a reason for hiding this comment

mstv May 12, 2024

Choose a reason for hiding this comment

mdonatas commented May 19, 2024

gerhardol commented May 19, 2024

mdonatas commented May 10, 2024 •

edited

mdonatas commented May 11, 2024 •

edited

mdonatas commented May 12, 2024 •

edited

mdonatas commented May 12, 2024 •

edited