Merge pull request #1314 from sillsdev/fix-logbox

Checked for disposed log box or caller-requested cancel in SafeInvoke

CHANGELOG.md

@@ -16,17 +16,21 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased]
 
+### Changed
+
 - [SIL.Archiving] Upgraded to L10nSharp 7.0.0
 - [SIL.Windows.Forms] Upgraded to L10nSharp 7.0.0
 - [SIL.Windows.Forms.DblBundle] Upgraded to L10nSharp 7.0.0
 - [SIL.Windows.Forms.Keyboarding] Upgraded to L10nSharp 7.0.0
 - [SIL.Windows.Forms.WritingSystems] Upgraded to L10nSharp 7.0.0
+- [SIL.Core] `RaiseExceptionIfFailed` no longer throws an exception if user cancelled
 
 ## [13.0.1] - 2024-01-09
 
 ### Fixed
 
 - [SIL.Core] Fixed bug in extension method GetLongestUsefulCommonSubstring when string ends with an Object replacement character
+- [SIL.Core] LogBox: Checked for disposed log box or caller-requested cancel in SafeInvoke so we don't try to write messages or scroll.
 
 ## [13.0.0] - 2023-12-07
 

Palaso.sln.DotSettings

@@ -31,6 +31,7 @@
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=bidi/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=bldr/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=browsable/@EntryIndexedValue">True</s:Boolean>
+	<s:Boolean x:Key="/Default/UserDictionary/Words/=cancellability/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=Canonicalizes/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=choco/@EntryIndexedValue">True</s:Boolean>
 	<s:Boolean x:Key="/Default/UserDictionary/Words/=Clipboarding/@EntryIndexedValue">True</s:Boolean>

SIL.Core/CommandLineProcessing/CommandLineRunner.cs

@@ -1,14 +1,16 @@
-// Copyright (c) 2018 SIL International
+// Copyright (c) 2024 SIL International
 // This software is licensed under the MIT License (http://opensource.org/licenses/MIT)
 
 using System;
 using System.Diagnostics;
 using System.Globalization;
 using System.IO;
 using System.Text;
+using JetBrains.Annotations;
 using SIL.IO;
 using SIL.PlatformUtilities;
 using SIL.Progress;
+using static System.String;
 
 namespace SIL.CommandLineProcessing
 {
@@ -23,7 +25,7 @@ public class CommandLineRunner
 		private Process _process;
 
 		/// <summary>
-		/// This one doesn't attemtp to influence the encoding used
+		/// This one doesn't attempt to influence the encoding used
 		/// </summary>
 		public static ExecutionResult Run(string exePath, string arguments, string fromDirectory, int secondsBeforeTimeOut, IProgress progress)
 		{
@@ -102,9 +104,11 @@ public ExecutionResult Start(string exePath, string arguments, Encoding encoding
 				exePath = "mono";
 			}
 			progress.WriteVerbose("running '{0} {1}' from '{2}'", exePath, arguments, fromDirectory);
-			ExecutionResult result = new ExecutionResult();
-			result.Arguments = arguments;
-			result.ExePath = exePath;
+			ExecutionResult result = new ExecutionResult
+			{
+				Arguments = arguments,
+				ExePath = exePath
+			};
 
 			using (_process = new Process())
 			{
@@ -191,19 +195,24 @@ public ExecutionResult Start(string exePath, string arguments, Encoding encoding
 		}
 
 		/// <summary>
-		/// On Windows, We can't get unicode over the command-line barrier, so
-		/// instead create 8.3 filename, which, happily, will have no non-english characters
-		/// for any part of the path. This is safe to call from Linux, too
+		/// On Windows, We can't get Unicode over the command-line barrier, so
+		/// instead create 8.3 filename, which, happily, will have no non-English characters
+		/// for any part of the path. This is safe to call from Linux, too.
 		/// </summary>
+		[PublicAPI]
 		public static string MakePathToFileSafeFromEncodingProblems(string path)
 		{
 			if (Directory.Exists(path))
-				throw new ArgumentException(string.Format("MakePathToFileSafeFromEncodingProblems() is only for files, but {0} is a directory.", path));
+			{
+				throw new ArgumentException(
+					$"MakePathToFileSafeFromEncodingProblems() is only for files, but {path} is a directory.");
+			}
 
 			var safe = "";
 
-			//if the filename doesn't exist yet, we can't get the 8.3 name. So we make it, get the name, then delete it.
-			//NB: this will not yet deal with the problem of creating a directory
+			// If the filename doesn't exist yet, we can't get the 8.3 name. So we make it, get
+			// the name, then delete it.
+			// NB: this will not yet deal with the problem of creating a directory
 			if (!File.Exists(path))
 			{
 				File.WriteAllText(path, "");
@@ -223,11 +232,12 @@ public static string MakePathToFileSafeFromEncodingProblems(string path)
 		/// instead create 8.3 filename, which, happily, will have no non-english characters
 		/// for any part of the path. This is safe to call from Linux, too
 		/// </summary>
+		[PublicAPI]
 		public static string MakePathToDirectorySafeFromEncodingProblems(string path)
 		{
 			if (File.Exists(path))
 				throw new ArgumentException(
-					string.Format(
+					Format(
 						"MakePathToDirectorySafeFromEncodingProblems() is only for directories, but {0} is a file.",
 						path));
 
@@ -251,9 +261,10 @@ public static string MakePathToDirectorySafeFromEncodingProblems(string path)
 
 
 		/// <summary>
-		/// Some command line applications (e.g. exiftool) can handle html-encoded characters in their command arguments
+		/// Some command line applications (e.g. exiftool) can handle HTML-encoded characters in their command arguments
 		/// </summary>
 		/// <remarks>From Rick Strahl at http://www.west-wind.com/weblog/posts/2009/Feb/05/Html-and-Uri-String-Encoding-without-SystemWeb</remarks>
+		[PublicAPI]
 		public static string HtmlEncodeNonAsciiCharacters(string text)
 		{
 			if (text == null)
@@ -289,17 +300,23 @@ public class ExecutionResult
 		public string StandardOutput;
 		public string ExePath;
 		public string Arguments;
-		public bool DidTimeOut { get { return ExitCode == kTimedOut; } }
-		public bool UserCancelled { get { return ExitCode == kCancelled; } }
+		public bool DidTimeOut => ExitCode == kTimedOut;
+		public bool UserCancelled => ExitCode == kCancelled;
 
+		[PublicAPI]
 		public void RaiseExceptionIfFailed(string contextDescription, params object[] originalPath)
 		{
-			if (ExitCode == 0)
+			// REVIEW: This method used to raise a regular ApplicationException in the case of
+			// UserCancelled. If anything, I assume we would want it to raise a
+			// UserCancelledException, but since the method name says "IfFailed" and the caller
+			// should always already know if the user cancelled, not sure there is any need to
+			// raise an exception in this case.
+			if (ExitCode == 0 || UserCancelled)
 				return;
 
 			var builder = new StringBuilder();
-			builder.AppendLine(string.Format("{0} {1} failed.", ExePath, Arguments));
-			builder.AppendLine("In the context of " + string.Format(contextDescription, originalPath));
+			builder.AppendLine($"{ExePath} {Arguments} failed.");
+			builder.AppendLine("In the context of " + Format(contextDescription, originalPath));
 			builder.AppendLine("StandardError: " + StandardError);
 			builder.AppendLine("StandardOutput: " + StandardOutput);
 			if (DidTimeOut)
@@ -308,7 +325,7 @@ public void RaiseExceptionIfFailed(string contextDescription, params object[] or
 		}
 	}
 
-
+	[PublicAPI]
 	public class UserCancelledException : ApplicationException
 	{
 		public UserCancelledException()

SIL.Core/IO/PathHelper.cs

@@ -122,8 +122,10 @@ static extern uint GetShortPathName(
 		   uint cchBuffer);
 
 		/// <summary>
-		/// When calling external exe's on Windows any non-ascii characters can get converted to '?'. This
-		/// will convert them to 8.3 format which is all ascii (and do nothing on Linux).
+		/// When calling external executables on Windows any non-ASCII characters can get converted to '?'. This
+		/// will convert them to 8.3 format which is all ascii (and do nothing on Linux). Note that this
+		/// is dependent on the drive being able to generate 8.3 filenames, so this is not guaranteed to produce
+		/// a short name.
 		/// </summary>
 		public static string MakePathSafeFromEncodingProblems(string path)
 		{

SIL.Windows.Forms.Tests/Progress/LogBox/LogBoxFormForTest.cs

@@ -6,25 +6,19 @@ namespace SIL.Windows.Forms.Tests.Progress.LogBox
 	public class LogBoxFormForTest : IDisposable
 	{
 		private Form _window;
-		private Windows.Forms.Progress.LogBox _box;
+		private Forms.Progress.LogBox _box;
 
 		public LogBoxFormForTest ()
 		{
 			CreateWindow();
 		}
 
-		public Windows.Forms.Progress.LogBox progress
-		{
-			get
-			{
-				return _box;
-			}
-		}
+		public Forms.Progress.LogBox progress => _box;
 
 		private void CreateWindow()
 		{
 			_window = new Form();
-			_box = new Windows.Forms.Progress.LogBox();
+			_box = new Forms.Progress.LogBox();
 			_box.Dock = DockStyle.Fill;
 			_window.Controls.Add(_box);
 

SIL.Windows.Forms.Tests/Progress/LogBox/LogBoxTests.cs

@@ -31,6 +31,57 @@ public void ShowLogBox()
 			}
 		}
 
+		[Test]
+		[Category("SkipOnTeamCity")]
+		public void WriteError_Disposed_IgnoreRequestsToWriteMessages()
+		{
+			Console.WriteLine("Showing LogBox");
+			using (var e = new LogBoxFormForTest())
+			{
+				progress = e.progress;
+				progress.WriteMessage("Hi, Mom!");
+				progress.Dispose();
+				progress.WriteError("Ignore this.");
+				Assert.IsTrue(progress.ErrorEncountered);
+				Assert.IsFalse(progress.Rtf.Contains("Ignore this."));
+				Assert.IsTrue(progress.Rtf.Contains("Hi, Mom!"));
+			}
+		}
+
+		[Test]
+		[Category("SkipOnTeamCity")]
+		public void WriteMessage_CancelRequested_HandleRequestsToWriteMessages()
+		{
+			Console.WriteLine("Showing LogBox");
+			using (var e = new LogBoxFormForTest())
+			{
+				progress = e.progress;
+				progress.WriteMessage("Hi, Mom!");
+				progress.CancelRequested = true;
+				progress.WriteMessage("Do not ignore this.");
+				Assert.IsFalse(progress.ErrorEncountered);
+				Assert.IsTrue(progress.Rtf.Contains("Do not ignore this."));
+				Assert.IsTrue(progress.Rtf.Contains("Hi, Mom!"));
+			}
+		}
+
+		[Test]
+		[Category("SkipOnTeamCity")]
+		public void WriteError_CancelRequested_HandleRequestsToWriteMessages()
+		{
+			Console.WriteLine("Showing LogBox");
+			using (var e = new LogBoxFormForTest())
+			{
+				progress = e.progress;
+				progress.WriteMessage("Hi, Mom!");
+				progress.CancelRequested = true;
+				progress.WriteError("User cancelled operation.");
+				Assert.IsTrue(progress.ErrorEncountered);
+				Assert.IsTrue(progress.Rtf.Contains("Hi, Mom!"));
+				Assert.IsTrue(progress.Rtf.Contains("User cancelled operation."));
+			}
+		}
+
 		[Test]
 		[Explicit]
 		[Category("LongRunning")]

SIL.Windows.Forms/Extensions/ControlExtensions.cs

@@ -1,4 +1,4 @@
-using System;
+using System;
 using System.ComponentModel;
 using System.Runtime.InteropServices;
 using System.Windows.Forms;
@@ -129,7 +129,7 @@ public static IAsyncResult SafeInvoke(this Control control, Action action, strin
 
 			if (!control.InvokeRequired)
 			{
-				if (control.IsDisposed)
+				if (control.IsDisposed || control.Disposing)
 				{
 					if (errorHandling == ErrorHandlingAction.Throw)
 						throw new ObjectDisposedException("SafeInvoke called after the control was disposed. (" + nameForErrorReporting + ")");

SIL.Windows.Forms/Progress/LogBox.cs

@@ -418,15 +418,11 @@ private void WriteErrorInternal(string message, params object[] args)
 		{
 			Write(Color.Red, _box.Font.Style, message, args);
 
-			// There is no Invoke method on ToolStripItems (which the _reportLink is)
-			// and setting it to visible from another thread seems to work okay.
-			_reportLink.Visible = true;
-
-			menuStrip1.Invoke(new Action(() =>
+			SafeInvoke(menuStrip1, () =>
 			{
+				_reportLink.Visible = true;
 				menuStrip1.Visible = true;
-			}));
-
+			});
 			ErrorEncountered = true;
 		}