-
-
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
Allow user scripts to operate on selected files #11239
Conversation
eeb9700
to
c18df2d
Compare
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 (#10866). #11242 provides the foundation for that work.
This is a "thin ice" kind of territory. This is what #6135 attempted to do. So, the menu must either
|
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"
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
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
This could be added as an extra option, e.g. This is especially the case when you consider neither the Ergo, it should be a separate option, which can be added in a separate PR.
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. |
(One of) the existing issues.
The existing behavior is an existing problem. 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. |
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.
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.
I think so, too.
"Show in RevisionGrid" just means "Do not 'hide' the item in the 'Run script' submenu". This option could be renamed accordingly.
I agree to ignore this. |
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
And 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 |
The prefix "f" does not have a meaning for the user. There will be no "gSelectedFiles". So it should be omitted.
I have not suggested anything like this. |
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 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 This would control whether said script appears in the top level of the context menu for the specific list, or is hidden inside the It would also be possible to add something like |
I do not expect that this will be implemented.
refer to the XMLdoc and the usage of |
@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.
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). |
I tried rebasing, but hit a snag: In the current
I can push the rebased version anyway, but I feel iffy about pushing a version that I can only test partially. |
c18df2d
to
78b2f18
Compare
Pushed anyway, because
|
78b2f18
to
fbace8b
Compare
Rebased |
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.
First pass.
@@ -306,13 +307,14 @@ private async Task SetDiffsAsync(IReadOnlyList<GitRevision> revisions) | |||
} | |||
} | |||
|
|||
public void Bind(IRevisionGridInfo revisionGridInfo, IRevisionGridUpdate revisionGridUpdate, RevisionFileTreeControl revisionFileTree, Func<string>? pathFilter, Action? refreshGitStatus) | |||
public void Bind(IRevisionGridInfo revisionGridInfo, IRevisionGridUpdate revisionGridUpdate, RevisionFileTreeControl revisionFileTree, Func<string>? pathFilter, Action? refreshGitStatus, IRunScript scriptRunner) |
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.
IRunScript
shouldn't be used, it will be removed in the future. Scripts should be invoked viaIScriptsRunner
interfaceIScriptsRunner
interface must be resolved from the service provider, which isUICommands
.
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.
The purpose of IScriptsRunner
is to run the script, and it is a replacement of ScriptRunner
. The purpose of IRunScript
is to refresh the UI after a command has completed, a purpose which is not (and should not be) served by IScriptsRunner
. Case in point: RevisionGridControl
still extends IRunScript
. There is, as of yet, no replacement for it.
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.
Case in point:
RevisionGridControl
still extendsIRunScript
. There is, as of yet, no replacement for it.
This is correct, and this is the fundamental task that needs to be solved first - work out how to support and execute scripts outside the RevisionGrid.
Currently, the scripts are assumed to be run in the context of the RevisionGrid
. Hence, it's expected to be available.
We have IScriptHostControl
abstraction (introduced in #7950) which purpose is to extend the script support to other controls - e.g., it allows executing user scripts in the left panel. The same approach should work for the FileTree and the RevisionDiff controls as well. The interface can/should be extended as necessary.
I think this method's signature must change taking IScriptHostControl
instead of RevisionGridControl
:
gitextensions/GitUI/ScriptsEngine/ScriptsManager.ScriptRunner.cs
Lines 18 to 19 in 4eae184
public static CommandStatus RunScript<THostForm>(ScriptInfo script, THostForm form, RevisionGridControl? revisionGrid) | |
where THostForm : IGitModuleForm, IWin32Window |
Consequently, each control providing the script menu would implement
IScriptHostControl
and then pass itself to the script runner.
Here's a very old prototype of mine (that served as a foundation for #7950) which attempted to add the script support to RevisionFileTreeControl
.
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.
IScriptHostControl
is currently a bit different, because it's used to get the current revision details. Something that, incidentally, would not be possible in the commit window. More importantly, since it doesn't wrap script running, the only way it could handle a UI refresh, is if it implemented a refresh callback, and it would become the duty of GitUI.ScriptsEngine.ScriptsManager.ScriptRunner.RunScript
to call it.
This gives me an idea, though. It's possible to unify IScriptHostControl
, IRunScript.IFileListSource
, and IRunScript
into a single interface IScriptHostControl
that would handle all of the above. It would simply return null
or be a no-op in cases where it's not relevant (e.g. getting the file list when running a script on the revision grid).
Then, methods like RevisionDiffControl.Bind
can take n
IScriptHostControlinstead of
IRunScript`, and forward any methods it doesn't handle.
I can do it, but I'm not sure what your opinion is regarding a PR that changes a bunch of things at once.
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.
P.S. I do want scripts in the sidebar too. But it has to be done correctly, and would be material for a separate PR. I have a script that runs git branch -rd
, and currently, since it can only be run in the revision grid, it requires me to pick a remote branch from a tiny dialog, if I have multiple remote branches on the same revision. Being able to run it directly on a remote branch instead would be pretty great. But it would require modifying IScriptHostControl
to be able to return a specific branch, instead of a revision.
@@ -15,6 +15,7 @@ public enum ScriptEvent | |||
BeforeMerge, | |||
AfterMerge, | |||
BeforeFetch, | |||
AfterFetch | |||
AfterFetch, | |||
ShowInFileList |
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.
This enum contains "git events". What does "ShowInFileList" mean in this context?
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.
ShowInUserMenuBar
is also an event. This enum is called ScriptEvent
, not GitEvent
. It represents situations in which a script should appear/be executed.
fbace8b
to
e4ce8ac
Compare
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.
+1
Only speed review, not run
I like the functionality
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?
|
I'll try to make a tiny "Remove |
9f77227
to
ad62741
Compare
Had to rebase again due to conflict (Minor change in an deleted line) |
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.
Thank you for your patience!
Working good. I understand that the consequence of this option is in fact to display the script directly in the menu and not in the "Run script" sub-menu but it is a bit counterintuitive for me. Should we not have a "ShowAtRootInFileList" and "ShowInFileList" that displays the script only where file(s) could be selected and with the 1st one keeping the current position in menu and the second displaying it in the "Run script" sub-menu (for less frequent usages)? Because, whatever option we select, it is available in the revision grid where no file(s) could be selected and the script is failing. |
|
So, like, what's the status of this? I haven't received any updates. What is the merge lifecycle like in this repo? |
I've made all requested changes, though, and even got an approval above. I'm not sure what else to do. |
just wait for @RussKie to approve, his request has been addressed |
Reviews from others are welcome, too - as always. If it affects the architecture, we want to get @RussKie's advice. Though he is pretty busy in RL. |
friendly nudge, @RussKie |
Thank you for the nudge, it's on my to do list.
I've been away for two months and working through the rl and work stuff
pile. I'll try to get to this PR as soon as possible.
|
I am going to squash-merge this PR next weekend. |
Cool. Finally. This has been slowing down my workflow for so long. |
Fixes #9974
Fixes #8769
Alternative to #11183
Proposed changes
SelectedFiles
andLineNumber
parameters to script parameters, based on currently selected files in the matching list, and the current selected line in the diff/blame view.{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.ShowInFileList
event for showing an option directly in the context menu for file lists, and not in theRun Scripts
submenu. Similar toShow 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
notepad++.exe
{if:SelectedFiles}{SelectedFiles} {if:LineNumber}-n{LineNumber}{/if}{/if}
)Test environment(s)
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.