+semver:major Made FadingMessageWindow implement all UI logic on the …

…main UI thread

CHANGELOG.md

@@ -18,9 +18,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Changed
 - [SIL.TestUtilities] Made FluentAssertXml classes use "Assert.That" so they can work in clients that use NUnit 4.
+- [SIL.Windows.Forms] Removed protected members from FadingMessageWindow: MsgThread, MsgForm, Text, MsgPoint, and ShowForm. (The underlying implementation needed to be changed, and the existing implementation was such that these members would be unlikely to have been meaningful or helpful in a derived class anyway.)
+- [SIL.Windows.Forms] Added the stated requirement that FadingMessageWindow.Show be called on the 
 - 
 ### Fixed
 - [SIL.Windows.Forms] Changed build date in SILAboutBox to be computed using the last write time instead of creation time.
+- [SIL.Windows.Forms] Made FadingMessageWindow implement all UI logic on the main UI thread in a thread-safe way. Fixes crashes like SP-2340.
 
 ## [15.0.0] - 2025-01-06
 

SIL.Windows.Forms/ClearShare/WinFormsUI/ContributorsListControl.cs

@@ -10,6 +10,7 @@
 using L10NSharp.UI;
 using SIL.Code;
 using SIL.Core.ClearShare;
+using SIL.Windows.Forms.Extensions;
 using SIL.Windows.Forms.Widgets.BetterGrid;
 
 namespace SIL.Windows.Forms.ClearShare.WinFormsUI
@@ -99,7 +100,7 @@ private void _grid_ColumnHeaderMouseClick(object sender, DataGridViewCellMouseEv
 
 		/// ------------------------------------------------------------------------------------
 		/// <summary>
-		/// Gets a value indicating whether or not the extender is currently in design mode.
+		/// Gets a value indicating whether the extender is currently in design mode.
 		/// I have had some problems with the base class' DesignMode property being true
 		/// when in design mode. I'm not sure why, but adding a couple more checks fixes the
 		/// problem.
@@ -233,9 +234,7 @@ private void HandleGridRowValidating(object sender, DataGridViewCellCancelEventA
 			if (ValidatingContributor == null)
 				return;
 
-			var args = new CancelEventArgs(e.Cancel);
-			var kvp = ValidatingContributor(this, contribution, args);
-			e.Cancel = args.Cancel;
+			var kvp = ValidatingContributor(this, contribution, e);
 
 			if (!string.IsNullOrEmpty(kvp.Key))
 			{
@@ -247,7 +246,11 @@ private void HandleGridRowValidating(object sender, DataGridViewCellCancelEventA
 					int col = dataGridViewColumn.Index;
 					var rc = _grid.GetCellDisplayRectangle(col, e.RowIndex, true);
 					var pt = new Point(rc.X + (rc.Width / 2), rc.Y + 4);
-					_msgWindow.Show(kvp.Value, _grid.PointToScreen(pt));
+					this.SafeInvoke(() =>
+						{
+							_msgWindow.Show(kvp.Value, _grid.PointToScreen(pt));
+						}, "Showing fading message with contributor field validation error",
+						ControlExtensions.ErrorHandlingAction.IgnoreIfDisposed, true);
 
 					// Invoking here because of "reentrant call to the SetCurrentCellAddressCore" exception.
 					// Setting the CurrentCell can trigger validation again.
@@ -388,8 +391,7 @@ private void DeleteRow(int rowIndex)
 			if (_grid.IsCurrentCellInEditMode)
 				_grid.EndEdit(DataGridViewDataErrorContexts.RowDeletion);
 
-			if (_msgWindow != null)
-				_msgWindow.Close();
+			_msgWindow?.Close();
 
 			_grid.Rows.RemoveAt(rowIndex);
 			_grid.CurrentCell = _grid[0, _grid.CurrentCellAddress.Y];

SIL.Windows.Forms/ClearShare/WinFormsUI/FadingMessageForm.cs

@@ -1,8 +1,8 @@
-using System;
+using System;
 using System.Drawing;
 using System.Drawing.Drawing2D;
-using System.Threading;
 using System.Windows.Forms;
+using SIL.Windows.Forms.Extensions;
 using Timer = System.Windows.Forms.Timer;
 
 namespace SIL.Windows.Forms.ClearShare.WinFormsUI
@@ -47,11 +47,8 @@ protected override void Dispose(bool disposing)
 		{
 			if (disposing)
 			{
-				if (components != null)
-					components.Dispose();
-
-				if (Timer != null)
-					Timer.Dispose();
+				components?.Dispose();
+				Timer?.Dispose();
 			}
 
 			Timer = null;
@@ -60,15 +57,12 @@ protected override void Dispose(bool disposing)
 		}
 
 		/// ------------------------------------------------------------------------------------
-		protected override bool ShowWithoutActivation
-		{
-			get { return true; }
-		}
+		protected override bool ShowWithoutActivation => true;
 
 		/// ------------------------------------------------------------------------------------
 		public override string Text
 		{
-			get { return base.Text; }
+			get => base.Text;
 			set
 			{
 				base.Text = value;
@@ -232,53 +226,50 @@ protected override void OnPaint(PaintEventArgs e)
 	/// ----------------------------------------------------------------------------------------
 	public class FadingMessageWindow
 	{
-		protected Thread MsgThread { get; set; }
+		private FadingMessageForm m_msgForm;
+		private readonly object m_syncLock = new object();
 
-		protected FadingMessageForm MsgForm { get; set; }
-
-		protected string Text { get; set; }
-
-		protected Point MsgPoint { get; set; }
-
-	    /// ------------------------------------------------------------------------------------
+		/// <summary>
+		/// Shows a fading message with the specified text at the specified screen location.
+		/// </summary>
+		/// <remarks>Caller is responsible for ensuring this is called on the UI thread.</remarks>
 		public void Show(string text, Point pt)
 		{
-			if (MsgThread != null)
-				return;
-
-			Text = text;
-			MsgPoint = pt;
-
-			// For some reason we have to specify a stack size, otherwise we get a stack overflow.
-			// The default stack size of 1MB works on WinXP. Needs to be 2MB on Win2K.
-			// Don't know what value it's using if we don't specify it.
-			MsgThread = new Thread(ShowForm, 0x200000);
-			MsgThread.CurrentUICulture = Thread.CurrentThread.CurrentUICulture;
-			MsgThread.IsBackground = true;
-			MsgThread.SetApartmentState(ApartmentState.STA);
-			MsgThread.Name = "FadingWindow";
-			MsgThread.Start();
-		}
-
-		/// ------------------------------------------------------------------------------------
-		protected virtual void ShowForm()
-		{
-			MsgForm = new FadingMessageForm(MsgPoint);
-			MsgForm.Text = Text;
-			MsgForm.ShowDialog();
-			MsgThread = null;
-			MsgForm = null;
+			lock (m_syncLock)
+			{
+				Close();
+				m_msgForm = new FadingMessageForm(pt)
+				{
+					Text = text
+				};
+				m_msgForm.FormClosed += (s, e) =>
+				{
+					lock (m_syncLock)
+					{
+						m_msgForm.Dispose();
+						m_msgForm = null; // Clear the reference when the form is closed
+					}
+				};
+				m_msgForm.Show();
+			}
 		}
 
-		/// ------------------------------------------------------------------------------------
 		public void Close()
 		{
-			if (MsgForm != null)
+			lock (m_syncLock)
 			{
-				lock (MsgForm)
-				{
-					MsgForm.Invoke(new MethodInvoker(MsgForm.Close));
-				}
+				if (m_msgForm == null || m_msgForm.IsDisposed)
+					return;
+
+				// Close the form on the UI thread
+				m_msgForm.SafeInvoke(() =>
+					{
+						// Technically, since this is being invoked synchronously, we don't need to
+						// re-obtain the lock here, but it makes the IDE (or Resharper) happy.
+						lock (m_syncLock)
+							m_msgForm?.Close();
+					}, "closing fading message",
+					ControlExtensions.ErrorHandlingAction.IgnoreIfDisposed, true);
 			}
 		}
 	}