Skip to content
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

Process.Start with ExternalOperationException #8721

Merged
merged 2 commits into from Jan 7, 2021

Conversation

mstv
Copy link
Member

@mstv mstv commented Jan 5, 2021

Contributes to #7795, extracted from #8278

Proposed changes

  • Use class Executable for OsShellUtil implementation
  • Use OsShellUtil functions instead of plain Process.Start where applicable

Test methodology

  • manual

Test environment(s)

  • Git Extensions 33.33.33
  • Build cca84cd
  • Git 2.27.0.windows.1 (recommended: 2.28.0 or later)
  • Microsoft Windows NT 10.0.19042.0
  • .NET Framework 4.8.4300.0
  • DPI 96dpi (no scaling)

✒️ I contribute this code under The Developer Certificate of Origin.

@mstv mstv self-assigned this Jan 5, 2021
@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #8721 (7c89f65) into master (371c4b3) will increase coverage by 0.02%.
The diff coverage is 18.03%.

@@            Coverage Diff             @@
##           master    #8721      +/-   ##
==========================================
+ Coverage   55.71%   55.74%   +0.02%     
==========================================
  Files         900      900              
  Lines       65096    65076      -20     
  Branches    11737    11736       -1     
==========================================
+ Hits        36270    36278       +8     
+ Misses      25995    25967      -28     
  Partials     2831     2831              
Flag Coverage Δ
production 42.75% <18.03%> (+0.02%) ⬆️
tests 94.95% <ø> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@gerhardol gerhardol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just questions about try-catch changes
Open, OpenAs are handled in OsShellUtil

GitUI/CommandsDialogs/FormBrowse.cs Show resolved Hide resolved
GitUI/CommandsDialogs/FormBrowse.cs Outdated Show resolved Hide resolved
@@ -33,7 +48,7 @@ public static void OpenUrlInDefaultBrowser(string url)
{
if (!string.IsNullOrWhiteSpace(url))
{
Process.Start(url);
new Executable(url).Start(useShellExecute: true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not work in .NET, despite the docs saying it should, and having it working in .NET Framework, I stumble across it yesterday in fact.

I had to add the verb option to get it working:

-            using var process = new Process
-            {
-                EnableRaisingEvents = false,
-                StartInfo = { FileName = uri.AbsoluteUri }
-            };
-            process.Start();
+            var ps = new ProcessStartInfo(uri.AbsoluteUri)
+            {
+                UseShellExecute = true,
+                Verb = "open"
+            };
+            Process.Start(ps);
         }
 }

@mstv
Copy link
Member Author

mstv commented Jan 6, 2021

@msftbot merge in 1 day

@ghost ghost added the status: auto merge label Jan 6, 2021
@ghost
Copy link

ghost commented Jan 6, 2021

Hello @mstv!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I won't merge this pull request until after the UTC date Thu, 07 Jan 2021 19:17:23 GMT, which is in 1 day

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@RussKie RussKie merged commit e0619e4 into gitextensions:master Jan 7, 2021
@ghost ghost added this to the 4.0 milestone Jan 7, 2021
@mstv mstv deleted the osshellutils branch January 7, 2021 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants