Feature/add windows explorer integration in settings #8010
Conversation
Update fork to latest
update fork
gerhardol
added
the
📜 status: needs cla
|
I don't know what others think of it but I would have loved to see these checkboxes changed in a 3 state drop-down "Display in context menu" / "Display in a cascaded menu" / "Hidden". That way everyone could choose which items he wants to display. And if all are hidden, then that's equivalent to your new option. Personally, I'm only interested by "Browse" menu item (that I use a lot) and I hide everything else in the cascaded menu to not pollute the contextual menu. Others, a point of view ? |
No strong opinion here, but a checkbox is easier to handle |
| set => HandleShellIntegration(null, null, value); | ||
| } | ||
|
|
||
| private static void HandleShellIntegration(object sender = null, EventArgs e = null, bool register = false) |
There was a problem hiding this comment.
Please move out to own file, call it something like ShellExtensionManager:
public sealed ShellExtensionManager
{
/// <summary>
/// Check whether the shell extensions is registered.
/// </summary>
/// <returns>
/// <see langword="true"/> if the shell extensions is registered; otherwise <see langword="false"/>.
/// </returns>
public bool CheckRegistered();
/// <summary>
/// Registers the shell extensions. Throws an exception, if registration process failed.
/// </summary>
public void Register([NotNull] string extensionLocation);
/// <summary>
/// Unregisters the shell extensions. Throws an exception, if registration process failed.
/// </summary>
public void Unregister([NotNull] string extensionLocation);
}NB: don't make it static, to reduce risks of race conditions while testing. The implementation should be stateless (as far as our code is concerned), so there is no issue with creating multiple instances of the class.
We can later replace is with MEF DI.
Furthermore, I don't like the functionality of registering/unregestering being placed in AppSettings. It is not a settings, and it is unrelated to the app.
If there is an error while registering or unregistering the shell extensions there is no way to inform a user.
Please move checks and register/unregister to the settings page, where it belongs.
| const string GitExtensionsShellEx32Name = "GitExtensionsShellEx32.dll"; | ||
| const string GitExtensionsShellEx64Name = "GitExtensionsShellEx64.dll"; |
| var pi = new ProcessStartInfo | ||
| { | ||
| FileName = "regsvr32", | ||
| Arguments = $"{(!register ? "/u " : "")}" + path.Quote(), | ||
| Verb = "RunAs", | ||
| UseShellExecute = false, | ||
|
|
||
| // TODO confirmation window is not suppressed | ||
| CreateNoWindow = true | ||
| }; | ||
|
|
||
| // TODO is IntPtr really the correct way to check architecture here? | ||
| if (IntPtr.Size == 8) | ||
| { | ||
| path = path.Replace(GitExtensionsShellEx32Name, GitExtensionsShellEx64Name); | ||
| if (File.Exists(path)) | ||
| { | ||
| pi.Arguments = $"{(!register ? "/u" : "")}" + path.Quote(); | ||
| } | ||
| } |
There was a problem hiding this comment.
❗ Multiple string allocations for no reason.
ghost
added
the
📭 needs: author feedback
Make it a dropdown list 😉 |
mstv
added
🖊️ status: cla signed
and removed
📜 status: needs cla
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs. |
|
This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 30 days. It will be closed if no further activity occurs. |
|
This is an old PR and should be closed or set to a draft. @gerhardol |
Fixes #7982
Proposed changes
Screenshots
Before
After
Test methodology
Test environment(s)
✒️ I contribute this code under The Developer Certificate of Origin.