Allow configuring editor for local files #11183
Conversation
There was a problem hiding this comment.
Could you please clarify why we can't execute the configured editor?
E.g.,
I see multiple issues with the proposed implementation. E.g., the new setting seems to be duplicating the existing functionality (thus creating a confusion). Also, the setting's placement doesn't sit well with me.
GitUI/GitUICommands.cs
Outdated
| string cmd = AppSettings.LocalFileEditor; | ||
| Match m = SingleArgRegex.Match(cmd); | ||
| if (!m.Success) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| StringBuilder args = new StringBuilder(); | ||
| if (m.Length < cmd.Length) | ||
| { | ||
| if (m.Index + m.Length < cmd.Length) | ||
| { | ||
| args.Append(cmd.Substring(m.Index + m.Length).TrimStart(_whiteSpaceChars)); | ||
| args.Append(" "); | ||
| } | ||
|
|
||
| cmd = cmd.Substring(m.Index, m.Length); | ||
| } |
There was a problem hiding this comment.
Because file names can contain spaces, the chosen editor is allowed to have command line option (e.g. -multiInst on the default Notepad++ configuration), and the system needs a clear separate arguments from executable in order for it to work.
Unfortunately, there's no piece of code that does this elsewhere, because calls to the other editor are done from git, which means that the code that basically does the above is in git's codebase.
An alternative would be to run the command using cmd /c, but that would force-open a console window, and also kill Linux/Mono compatibility.
There was a problem hiding this comment.
Because file names can contain spaces...
This shouldn't be a problem. We handle files with spaces in various situations. Just call Quote(...) method.
...the chosen editor is allowed to have command line option (e.g.
-multiInston the default Notepad++ configuration),
Why is this a problem?
...and the system needs a clear separate arguments from executable in order for it to work.
Why?
There was a problem hiding this comment.
also kill Linux/Mono compatibility.
We don't maintain the compatibility since v3.
There was a problem hiding this comment.
Let me try to explain it as simple as possible. Running:
new Executable("Notepad++ -multiInst -nosession").Start("MyFile.cs", useShellExecute: true, throwOnErrorExit: false);
will fail with an exception. Running
new Executable("Notepad++").Start("-multiInst -nosession MyFile.cs", useShellExecute: true, throwOnErrorExit: false);
will work as expected.
The code transforms the prior into the latter. the Quote extension cannot do that.
|
|
||
| cboEditor.Items.Add(string.Empty); | ||
| cboEditor.Items.AddRange(EditorHelper.GetEditors()); |
There was a problem hiding this comment.
I don't think this is the right settings page for the setting. Besides, the wrong context, the page is already at the size limit for small factor screens.
There was a problem hiding this comment.
I looked at all the other setting pages, and none of them seem to remotely fit. Except maybe "Advanced", but it's not a particularly advanced feature.
There was a problem hiding this comment.
Moved the setting to the Diff viewer page, which has a lot more room. Even though commit dialog and file list are also influenced, the prior doesn't directly link to its settings, and the latter doesn't have a settings page at all.
RussKie
added
📭 needs: author feedback
ghost
removed
the
📭 needs: author feedback
Constraint differences. The configured editor has a rigid constraint of having to finish in finite time, so the commit/rebase/etc operation may continue. This is why each of the default options has some form of When editing code files, this constraint does not exist. It can "fire and forget". There's no operation that's waiting for the editing to finish. And in fact, it would make significantly more sense to open and edit a bunch of files at once in the same editor, rather than spawn a new window/session for each. So all the The existing Editor, that is specifically and appropriately in the Git Config tab, is for editing commit messages and the such. For the most part, it could probably be replaced with a message box. But it provides the option of opening a full editor in a message box-like state. |
SlugFiller
force-pushed
the
custom-local-file-editor
branch
3 times, most recently
from
a118e87
to
771fafb
Compare
SlugFiller
force-pushed
the
custom-local-file-editor
branch
from
771fafb
to
1f25511
Compare
Fair, though does it really lock up the app? |
In the case of git calling the app, yes, it locks up git. It's technically because git explicitly waits for the editor using You can easily test, of course. Do an interactive rebase with any editor other than the built-in one, and see what happens to GitExtensions in the background while the editor is open. Conversely, if multi instance is used when it's not needed, it will spawn a bunch of unnecessary windows (and processes), overloading the system (Especially if using a heavy editor, like VS), and making editing very uncomfortable.
Yes, but that is the user's choice, not the editor. While it's not recommended, it is entirely possible and even reasonable, for a user to want to open every local file in a new process. And there are also other possible command lines that might not need to be stripped, like selecting user profile, picking a specific desktop window to open on, or any other custom command line options an editor may offer. Auto-stripping is not a realistic option. |
I think this is over-engineering. In the 15 years this hasn't been a real issue. In the OP has expressed the desire to open the files in NPP, which can already be easily achieved with the functionality available in the context menus: It could be a little clunky, but again I would expect the users to be working with the "working directory" files in some kind of editor already (it's easy to type "code ." and then explore the files). I can see some value allowing users to open the working directory files with the pre-confgiured editor, which the users is already using for git interactions, but I oppose adding arbitrary configurations because they don't add significant value but add a maintenance overhead. |
Actually, it is a little trickier than I thought. Still, there are plenty of options, though I'd go with https://learn.microsoft.com/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw. |
Technically my executable is already splitting off the command line because
The OP was poorly expressed, in various ways. It's a common problem, but opening a different issue would just be closed as duplicate. A better thing to look at would be TortoiseHg, which does give a convenient right-click edit file.
That is a reasonable expectation for someone working primarily on CSharp projects built using Visual Studio. Like this one. It is completely unreasonable if one is working on a project that has to be built in MinGW (or heck, CygWin), because Microsoft aren't huge fans of the whole "compatibility" thing. GitExtensions is used for projects other than GitExtensions. There are people who work with Vim on Windows, you know.
In most of those 15 years, I was working with Mercurial, where it is indeed not an issue. Among other things (No interface for
Adding one setting line to allow users to have the right tool for the right job is not "over-engineering". If anything, stripping command line options so that you can use the same setting without it becoming painful is significantly more engineering than an option. And also, secret voodoo hidden from the user. And all of that before mentioning using an option from the git configuration section (saved in
A feature has maintenance cost. A setting does not. We're not discussing on whether the feature itself is needed or not (I hope. My posts may start being pages long if we go down that road), but on whether the feature can be offered without an associated setting. Adding one line of setting to the hundred of existing ones, that is essentially copy-pasted from existing ones, is not a real cost addition. Even if one was to do a huge refactor of the entire settings page, the change to the lines making up this setting would be indistinct from the changes to the lines making up the settings immediately before and after it. |
|
I don't get what AppVeyor wants from me. My best guess is that it relates to this message: |
Just wait some time. You may close/open the PR to trigger a build (repo maintainers can restart manually too). |
So does Git Extensions. The app provides several different options to open a file - both from the working directory and from a selected revision.
I am afraid I fail to see you point here. The working directory is the files on disk. Whether the user is using GUI editor or vim is immaterial. The user is able to open the required file in the editor of their choice. Here's a very crude example (took less than 10 min) using the code from http://pinvoke.net/ (that code it's grossly inefficient but works for demo purposes): diff --git a/GitUI/GitUICommands.cs b/GitUI/GitUICommands.cs
index c838a1584..b20008764 100644
--- a/GitUI/GitUICommands.cs
+++ b/GitUI/GitUICommands.cs
@@ -1,4 +1,7 @@
-using System.Text;
+using System.ComponentModel;
+using System.Diagnostics;
+using System.Runtime.InteropServices;
+using System.Text;
using GitCommands;
using GitCommands.Git;
using GitCommands.Git.Commands;
@@ -11,7 +14,6 @@
using GitUIPluginInterfaces;
using GitUIPluginInterfaces.RepositoryHosts;
using JetBrains.Annotations;
-using SmartFormat.Core.Output;
using static GitUI.CommandsDialogs.FormBrowse;
@@ -1684,12 +1686,76 @@ private bool RunRebaseCommand(IReadOnlyDictionary<string, string?> arguments)
return StartRebaseDialog(owner: null, onto: branch);
}
- public bool StartFileEditorDialog(string? filename, bool showWarning = false)
+ public bool StartFileEditorDialog(string? fileName, bool showWarning = false, bool useDefaultEditor = true)
{
- using FormEditor formEditor = new(this, filename, showWarning);
+ if (string.IsNullOrEmpty(fileName))
+ {
+ return false;
+ }
+
+ if (!useDefaultEditor)
+ {
+ // TODO: Read from the config the same way we read it in the settings
+ string[] args = SplitArgs("\"C:\\Program Files\\notepad++\\notepad++.exe\" -multiInst -nosession");
+ if (args.Length > 0)
+ {
+ ProcessStartInfo psi = new()
+ {
+ UseShellExecute = true,
+ FileName = args[0],
+ Arguments = fileName
+ };
+ Process.Start(psi);
+ return false;
+ }
+ }
+
+ using FormEditor formEditor = new(this, fileName, showWarning);
return formEditor.ShowDialog() != DialogResult.Cancel;
}
+#pragma warning disable SA1305 // Field names should not use Hungarian notation
+ private static string[] SplitArgs(string unsplitArgumentLine)
+ {
+ int numberOfArgs;
+ IntPtr ptrToSplitArgs;
+ string[] splitArgs;
+
+ ptrToSplitArgs = CommandLineToArgvW(unsplitArgumentLine, out numberOfArgs);
+
+ // CommandLineToArgvW returns NULL upon failure.
+ if (ptrToSplitArgs == IntPtr.Zero)
+ {
+ throw new ArgumentException("Unable to split argument.", new Win32Exception());
+ }
+
+ // Make sure the memory ptrToSplitArgs to is freed, even upon failure.
+ try
+ {
+ splitArgs = new string[numberOfArgs];
+
+ // ptrToSplitArgs is an array of pointers to null terminated Unicode strings.
+ // Copy each of these strings into our split argument array.
+ for (int i = 0; i < numberOfArgs; i++)
+ {
+ splitArgs[i] = Marshal.PtrToStringUni(Marshal.ReadIntPtr(ptrToSplitArgs, i * IntPtr.Size));
+ }
+
+ return splitArgs;
+ }
+ finally
+ {
+ // Free memory obtained by CommandLineToArgW.
+ LocalFree(ptrToSplitArgs);
+ }
+ }
+
+ [DllImport("shell32.dll", SetLastError = true)]
+ private static extern IntPtr CommandLineToArgvW([MarshalAs(UnmanagedType.LPWStr)] string lpCmdLine, out int pNumArgs);
+
+ [DllImport("kernel32.dll")]
+ private static extern IntPtr LocalFree(IntPtr hMem);
+#pragma warning restore SA1305 // Field names should not use Hungarian notation
+
/// <summary>
/// Remove working directory from filename and convert to POSIX path.
/// This is to prevent filenames that are too long while there is room left when the workingdir was not in the path.And the callsite: diff --git a/GitUI/CommandsDialogs/RevisionFileTreeControl.cs b/GitUI/CommandsDialogs/RevisionFileTreeControl.cs
index cbe8a560a..4b57ceefd 100644
--- a/GitUI/CommandsDialogs/RevisionFileTreeControl.cs
+++ b/GitUI/CommandsDialogs/RevisionFileTreeControl.cs
@@ -614,7 +614,9 @@ private void editCheckedOutFileToolStripMenuItem_Click(object sender, EventArgs
var fileName = _fullPathResolver.Resolve(gitItem.FileName);
Validates.NotNull(fileName);
- UICommands.StartFileEditorDialog(fileName);
+ UICommands.StartFileEditorDialog(fileName, useDefaultEditor: false);
_refreshGitStatus?.Invoke();
} |
I agree. The only drawback is that |
Here are the options:
I may consider switching from option 3 to option 4, but the other ones are not remotely viable.
You mean, as in, file, open, navigate to wherever in the harddrive the repository may be, one folder level at a time, then inside however many folders deep the repository is (which is already bad enough in GitExtensions, which isn't even a Java project), then find the one file from the diff you were looking at, hoping you don't get the wrong file which is named almost the same thing but with just 2 letters different? Yeah, that's great UX.
wot. Conflict resolution? What are you talking about? Conflict resolution would be done with a diff tool, like KDiff3. The external editor used by Git is for entering a commit message. It's a glorified messagebox. It is not intended as a code editor. In fact, a user would benefit from leaving it as the default, since it is intended for small texts in an interactive setting. It is nothing at all like editing code files. The triple assumption that it's going to be the same editor, that the user would be willing to use whatever editor is configured in each repository even if each defines a different one, and that whatever editor the user chooses does not require any extra command line arguments other than the filename, is a pretty huge assumption to make.
If your code contains script files (Python, Perl, batch files, etc), and your goal was to execute them rather than edit them, then this statement would be true. But that is rarely the case. If you're using a git client, you're probably aiming to edit the files, not execute them.
By that you mean, allow the addition of any number of arbitrary editors to the context menu? I mean, works for me, and I can probably do it, but I thought the point was to avoid over-engineering and extra options, and this seems like the extreme opposite of that. But, if you're more likely to merge such a huge change over this small one, I could probably make it work in a couple of days. |
👍 [EDIT] Found it: master...RussKie:gitextensions:Run_scripts_from_FileTree |
It depends on a specific IDE. Since VS Code has become ubiquitous, it's easy to
VS, VS Code and other IDEs are often used as diff tools too. But, agree, this is orthogonal.
The application supports a notion of "user scripts", which can currently be only executed against commits but not files. We had a FR to allow execution of user scripts against files, which means you could add a script with a hotkey that would allow you to open any file in an editor of your choice. No application changes would be required. |
Using the open with functionality of ctrl o and f3 options provides the user pick what to open with. Which is better than a setting in our program that is the idea of what f4 does now I don't see the need of a new setting along with once file scripts work, user can add a script that adds the open with blaaa editor menu option. |
|
Could do something like //chat gpt generated
using Shell32;
using System;
using System.Collections.Generic;
using System.IO;
using System.Runtime.InteropServices;
using System.Windows.Forms;
public class ExplorerContextMenuExample : Form
{
public ExplorerContextMenuExample()
{
Text = "Explorer Context Menu Example";
Size = new System.Drawing.Size(300, 200);
Button button1 = new Button();
button1.Text = "Show Explorer Context Menu";
button1.Click += (sender, e) =>
{
string filePath = @"C:\Path\To\Your\FileOrFolder";
CreateContextMenu(filePath);
};
Controls.Add(button1);
}
private Dictionary<string, string> GetContextMenuItems(string filePath)
{
Dictionary<string, string> contextMenuItems = new Dictionary<string, string>();
Shell shell = new Shell();
Folder folder = shell.NameSpace(Path.GetDirectoryName(filePath));
FolderItem folderItem = folder.ParseName(Path.GetFileName(filePath));
if (folderItem != null)
{
FolderItemVerbs verbs = folderItem.Verbs();
foreach (FolderItemVerb verb in verbs)
{
contextMenuItems.Add(verb.Name, verb.Name);
Marshal.ReleaseComObject(verb);
}
Marshal.ReleaseComObject(verbs);
Marshal.ReleaseComObject(folderItem);
}
Marshal.ReleaseComObject(folder);
Marshal.ReleaseComObject(shell);
return contextMenuItems;
}
private void ExecuteContextMenuCommand(string command)
{
Shell shell = new Shell();
shell.FileRun(command);
Marshal.ReleaseComObject(shell);
}
private void CreateContextMenu(string filePath)
{
Dictionary<string, string> contextMenuItems = GetContextMenuItems(filePath);
ContextMenu mainContextMenu = new ContextMenu();
MenuItem explorerMenuItem = new MenuItem("Explorer");
mainContextMenu.MenuItems.Add(explorerMenuItem);
foreach (var menuItem in contextMenuItems)
{
MenuItem subMenuItem = new MenuItem(menuItem.Key);
subMenuItem.Click += (sender, e) =>
{
ExecuteContextMenuCommand(menuItem.Value);
};
explorerMenuItem.MenuItems.Add(subMenuItem);
}
this.ContextMenu = mainContextMenu;
}
[STAThread]
public static void Main()
{
Application.EnableVisualStyles();
Application.SetCompatibleTextRenderingDefault(false);
Application.Run(new ExplorerContextMenuExample());
}
}to show the windows explorer menu in our program. |
I have no doubt VS Code has a large user base. You're making it sound like everyone except a small fringe are using it, however. I'm sure every person you know uses it. Conversely, no person I know uses it. That's just locality of information. The closest I can get to reliable statistics on usage is checking chocolatey download numbers, putting vscode at about 5 million downloads, and notepadplusplus at 20 million. There are other mirrors for download, of course, but the issue is that there are too many mirrors, and most don't share download statistics, making it impossible to sum them all up. I suppose the next best option is to ask Google trends. My point is, "people who don't use VS Code" is not a tiny fringe sector.
"Current state" of what? Although the most appropriate "current state" to compare to would be "notable competitors/alternatives", like, say, TortoiseHg, which has had right click->"Edit Local" open whatever is set in the "Visual Editor" settings since its first release more than a decade ago. They didn't see it as a problem to have an extra setting line.
Agreed. Is this on a roadmap? PR open? PRs invited?
Windows's default open with dialog prioritizes the run option associated with the file type. A text editor will usually not be there, and cannot be configured to always be there, except per-extension, replacing the default action, which would be catastrophic for such extensions as More often, that flow continues to "More apps", "Look for another app on this PC". From there, it's still another half dozen clicks before you get to your editor opening. And it has to be repeated for every file you open.
Agreed. It's a bit overkill. However, there are third party utilities for editing the context menu, so there can never be a missing option. I can make a PR for this if this option is preferred. |
|
(The Git editor is completely different from user editor, I believe this was established above.) Using F4 to quickly open an editor to make quick changes is a good feature: You quickly get to do what you want to do without loosing your current thoughts. Still, it would be nice to have a way to have a one key solution to open a selected file in currently preferred editor/IDE. Filelist user script could be a long term solution (I am not opposing changing the default editor but this is more versatile).
Seem like a good addition, regardless of the user script/default editor. |
Diff view user scripts would do that. That's a good point, though. |
Rebuilt OK
|
Fixes #8769
Proposed changes
In Settings, General, adds an option "Local file editor" which allows configuring an alternative editor for F4 action. If left blank, the current behavior is applied.
Only applies to actions associated with the F4 button. Does not apply to custom editors, e.g.
.gitignore,.git/config.Screenshots
Before
When pressing F4 in diff list, file tree, or commit window, an built-in editor opens up.
After
It is possible to configure a different editor. If left blank, the old behavior remains. If configured, the selected editor is executed instead.
Test methodology
"D:\Program Files\TortoiseHg\lib\kdiff3.exe" C:\temp\test.txtto test running a program that contains spaces in its filename, while also applying command line arguments.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.