-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose ScriptOptionsProvider to toolbar event scripts #11734
Expose ScriptOptionsProvider to toolbar event scripts #11734
Conversation
I am using something similar: 00d0c84. 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. |
Are you describing your needs/workflow or vision for how it should be implemented in GE? For my scenario I very much need 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. |
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. PS> start @(Get-ChildItem -Filter *.sln)[0]
# or
PS> start (Get-ChildItem -Filter *.sln | Select-Object -First 1) |
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 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. |
A workflow which requires 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 Maybe then this variable should be called {SelectedCommitBlobPaths} (https://git-scm.com/book/id/v2/Git-Internals-Git-Objects)
|
The 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). |
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. |
You may know what you are doing, stretching the use. 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. (I have not decided myself here.) |
Maybe it would be OK to add |
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 let @gerhardol @mstv and @pmiossec decide and take or reject this change. |
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 Then, to make the power-user side of the isle happy we'd need something like 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. |
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. |
Explicit (radioboxes?) "Use worktree version" / "Use selected revision"? Some features in GE is for power users, all will not be accessible for all users. Are files expected to be selected outside of Diff/FileTree tabs?
I expect all commits or not at all. |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, we need #11342 in addition in order to handle the case "no blobs exist / are selected".
Wanted to gather more input on the approach moving forward.
@gerhardol I am not sure I understand what these would mean. |
I rather have only have
If |
I think this just needs an update of the PR title & description and a rebase in order to resolve the MC. |
e749ecb
to
1fceca3
Compare
@mstv thanks 🙏 I've rebased and updated both the title and the description to be more generic |
+0 |
Previous PR description before it got "rebranded"
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?FormBrowse
when looking for aIScriptOptionsProvider
,FormBrowse
conveniently has two potential sources for itGetScriptOptionsProvider()
inFormBrowse
to forward this request toRevisionDiffControl
orRevisionFileTreeControl
depending on which one is visibleProposed changes
Currently scripts using
toolbar
event type do not have access toIScriptOptionsProvider
thus are unable to make use of all variables provided by the scripting mechanism.FormBrowse
when looking for aIScriptOptionsProvider
,FormBrowse
conveniently has two potential sources for itGetScriptOptionsProvider()
inFormBrowse
to retrieveIScriptOptionsProvider
fromRevisionDiffControl
orRevisionFileTreeControl
depending on which one is visible/activeTest methodology
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.