+semver:minor Added ContributorsListControl.Initialize method to allow the model to be set later when using the (existing) parameterless constructor in Designer.

Made ContributorsListControl more threadsafe. Possibly fixes crashes like SP-2353 or at least makes them less likely.
Made the TestApp's ContributorsForm able to be edited in Designer and added code to test accessing various methods and properties on ContributorsListControl from a non-UI thread
Also did some minor refactoring, comment/spelling cleanup, etc.
[WIP?] See REVIEW comment at line 259 in ContributorsListControl.cs

Author: tombogle

CHANGELOG.md

@@ -21,6 +21,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [SIL.WritingSystems] Added public DownloadLanguageTags method for updating the cached langtags.json from the SLDR repository.
 - [SIL.Core] Add environment variable to disable `GlobalMutex` across processes. Helpful for snap packages in Linux.
 - [SIL.Core] Added string extension method SplitLines.
+- [SIL.Windows.Forms] Added ContributorsListControl.Initialize method to allow the model to be set later when using the (existing) parameterless constructor in Designer.
 
 ### Changed
 
@@ -61,6 +62,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 - [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.
+- [SIL.Windows.Forms] Made ContributorsListControl more threadsafe. Possibly fixes crashes like SP-2353 or at least makes them less likely.
 
 ## [15.0.0] - 2025-01-06
 

SIL.Windows.Forms/ClearShare/ContributorsListControlViewModel.cs

@@ -26,10 +26,7 @@ public ContributorsListControlViewModel(IAutoCompleteValueProvider autoCompleteP
 		}
 
 		/// ------------------------------------------------------------------------------------
-		public IEnumerable<Role> OlacRoles
-		{
-			get { return _olacSystem.GetRoles(); }
-		}
+		public IEnumerable<Role> OlacRoles => _olacSystem.GetRoles();
 
 		/// ------------------------------------------------------------------------------------
 		public GridSettings ContributorsGridSettings
@@ -53,8 +50,7 @@ internal void SaveContributionList(ContributionCollection list)
 		{
 			Contributions = (list ?? new ContributionCollection());
 
-			if (_saveAction != null)
-				_saveAction();
+			_saveAction?.Invoke();
 		}
 
 		/// ------------------------------------------------------------------------------------

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

@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.ComponentModel;
 using System.ComponentModel.Design;
+using System.Diagnostics;
 using System.Drawing;
 using System.Linq;
 using System.Media;
@@ -10,8 +11,10 @@
 using L10NSharp.UI;
 using SIL.Code;
 using SIL.Core.ClearShare;
+using SIL.Reporting;
 using SIL.Windows.Forms.Extensions;
 using SIL.Windows.Forms.Widgets.BetterGrid;
+using static System.String;
 
 namespace SIL.Windows.Forms.ClearShare.WinFormsUI
 {
@@ -39,7 +42,7 @@ public delegate KeyValuePair<string, string> ValidatingContributorHandler(
 		public event ColumnHeaderMouseClickHandler ColumnHeaderMouseClick;
 
 		private FadingMessageWindow _msgWindow;
-		private readonly ContributorsListControlViewModel _model;
+		private ContributorsListControlViewModel _model;
 
 		/// ------------------------------------------------------------------------------------
 		public ContributorsListControl()
@@ -48,6 +51,13 @@ public ContributorsListControl()
 			_grid.DataError += _grid_DataError;
 		}
 
+		/// ------------------------------------------------------------------------------------
+		public ContributorsListControl(ContributorsListControlViewModel model) : this()
+		{
+			Initialize(model);
+		}
+
+		/// ------------------------------------------------------------------------------------
 		private void _grid_DataError(object sender, DataGridViewDataErrorEventArgs e)
 		{
 		    if (e.Exception != null &&
@@ -58,11 +68,25 @@ private void _grid_DataError(object sender, DataGridViewDataErrorEventArgs e)
 		}
 
 		/// ------------------------------------------------------------------------------------
-		public ContributorsListControl(ContributorsListControlViewModel model) : this()
+		[PublicAPI]
+		public void Initialize(ContributorsListControlViewModel model)
 		{
+			Debug.Assert(_model == null);
+
 			_model = model;
 			_model.NewContributionListAvailable += HandleNewContributionListAvailable;
 
+			// Do not use SafeInvoke here because we want this to work even if called before the
+			// handle is created.
+			if (InvokeRequired)
+				Invoke(new Action(InitializeGrid));
+			else
+				InitializeGrid();
+		}
+
+		/// ------------------------------------------------------------------------------------
+		private void InitializeGrid()
+		{
 			_grid.Font = SystemFonts.MenuFont;
 
 			DataGridViewColumn col = BetterGrid.CreateTextBoxColumn(kNameColName, "Name");
@@ -99,8 +123,12 @@ public ContributorsListControl(ContributorsListControlViewModel model) : this()
 			_grid.ColumnHeaderMouseClick += _grid_ColumnHeaderMouseClick;
 
 			_model.ContributorsGridSettings?.InitializeGrid(_grid);
+
+			if (_model.Contributions.Any())
+				HandleNewContributionListAvailable(_model, EventArgs.Empty);
 		}
 
+		/// ------------------------------------------------------------------------------------
 		// SP-874: Not able to open L10NSharp with Alt-Shift-click
 		private void _grid_ColumnHeaderMouseClick(object sender, DataGridViewCellMouseEventArgs e)
 		{
@@ -119,26 +147,37 @@ private void _grid_ColumnHeaderMouseClick(object sender, DataGridViewCellMouseEv
 			GetService(typeof(IDesignerHost)) != null ||
 			LicenseManager.UsageMode == LicenseUsageMode.Designtime;
 
+		/// ------------------------------------------------------------------------------------
 		public void SetColumnAutoSizeMode(StandardColumns col, DataGridViewAutoSizeColumnMode autoSizeMode)
 		{
-			DataGridViewColumn column;
-			switch (col)
+			if (_model == null)
 			{
-				case StandardColumns.Name:
-					column = Grid.Columns[kNameColName];
-					break;
-				case StandardColumns.Role:
-					column = Grid.Columns[kRoleColName];
-					break;
-				case StandardColumns.Comments:
-					column = Grid.Columns[kCommentsColName];
-					break;
-				case StandardColumns.Date:
-					column = Grid.Columns[kDateColName];
-					break;
-				default:
-					throw new ArgumentOutOfRangeException(nameof(col), col, null);
+				if (IsDisposed)
+					throw new ObjectDisposedException(Name ?? GetType().Name);
+				throw new InvalidOperationException(
+					$"{nameof(Initialize)} must be called to set the model first.");
 			}
+
+			// Do not use SafeInvoke here because we want this to work even if called before the
+			// handle is created.
+			if (Grid.InvokeRequired)
+				Grid.Invoke(new Action(() => SetColumnAutoSizeMode_Internal(col, autoSizeMode)));
+			else
+				SetColumnAutoSizeMode_Internal(col, autoSizeMode);
+		}
+
+		/// ------------------------------------------------------------------------------------
+		private void SetColumnAutoSizeMode_Internal(StandardColumns col, DataGridViewAutoSizeColumnMode autoSizeMode)
+		{
+			var column = col switch
+			{
+				StandardColumns.Name => Grid.Columns[kNameColName],
+				StandardColumns.Role => Grid.Columns[kRoleColName],
+				StandardColumns.Comments => Grid.Columns[kCommentsColName],
+				StandardColumns.Date => Grid.Columns[kDateColName],
+				_ => throw new ArgumentOutOfRangeException(nameof(col), col, null)
+			};
+
 			if (column != null)
 				column.AutoSizeMode = autoSizeMode;
 		}
@@ -149,7 +188,7 @@ public bool InEditMode
 		{
 			get
 			{
-				if (_grid.InvokeRequired)
+				if (InvokeRequired)
 					return (bool)_grid.Invoke(new Func<bool>(() => _grid.IsCurrentRowDirty));
 				return _grid.IsCurrentRowDirty;
 			}
@@ -175,23 +214,50 @@ protected override void OnHandleDestroyed(EventArgs e)
 			base.OnHandleDestroyed(e);
 		}
 
-		/// ------------------------------------------------------------------------------------
 		private void HandleNewContributionListAvailable(object sender, EventArgs e)
 		{
-			Guard.AgainstNull(_model.Contributions, "Contributions");
+			if (InvokeRequired)
+			{
+				// Since BeginInvoke ensures the call is executed in the UI message loop (at a
+				// later time), it might avoid conflicts with UI state updates still in progress.
+				_grid.BeginInvoke(new Action(() => HandleNewContributionListAvailable(sender, e)));
+				return;
+			}
 
-			_grid.RowValidated -= HandleGridRowValidated;
-			_grid.RowsRemoved -= HandleGridRowsRemoved;
-			_grid.Rows.Clear();
+			Guard.AgainstNull(_model.Contributions, nameof(_model.Contributions));
 
-			foreach (var contribution in _model.Contributions)
-				_grid.Rows.Add(contribution.ContributorName, contribution.Role.Name, contribution.Date, contribution.Comments);
+			try
+			{
+				// Ensure any pending edits are committed or canceled before modifying rows
+				if (_grid.IsCurrentCellInEditMode)
+					_grid.EndEdit();
+				if (_grid.CurrentCell != null && _grid.CurrentRow?.IsNewRow == false)
+					_grid.CommitEdit(DataGridViewDataErrorContexts.Commit);
 
-			_grid.CurrentCell = _grid[0, 0];
-			_grid.IsDirty = false;
+				_grid.RowValidated -= HandleGridRowValidated;
+				_grid.RowsRemoved -= HandleGridRowsRemoved;
+        
+				_grid.SuspendLayout(); // Improve performance when modifying multiple rows
+				_grid.Rows.Clear();
 
-			_grid.RowValidated += HandleGridRowValidated;
-			_grid.RowsRemoved += HandleGridRowsRemoved;
+				foreach (var contribution in _model.Contributions)
+					_grid.Rows.Add(contribution.ContributorName, contribution.Role.Name, contribution.Date, contribution.Comments);
+
+				if (_grid.Rows.Count > 0)
+					_grid.CurrentCell = _grid[0, 0];
+
+				_grid.IsDirty = false;
+
+				_grid.ResumeLayout(); // Re-enable layout updates
+
+				_grid.RowValidated += HandleGridRowValidated;
+				_grid.RowsRemoved += HandleGridRowsRemoved;
+			}
+			catch (InvalidOperationException ex)
+			{
+				Logger.WriteError("Error updating contribution list", ex);
+				// REVIEW: Then what? Retry every second until this succeeds???
+			}
 		}
 
 		/// ------------------------------------------------------------------------------------
@@ -277,7 +343,7 @@ private void HandleGridRowValidating(object sender, DataGridViewCellCancelEventA
 
 			var kvp = ValidatingContributor(this, contribution, e);
 
-			if (!string.IsNullOrEmpty(kvp.Key))
+			if (!IsNullOrEmpty(kvp.Key))
 			{
 				_msgWindow ??= new FadingMessageWindow();
 
@@ -390,12 +456,12 @@ private void HandleGridCellEndEdit(object sender, DataGridViewCellEventArgs e)
 				// is the current text an exact match for the autocomplete list?
 				var list = txtBox.AutoCompleteCustomSource.Cast<object>().ToList();
 				var found = list.FirstOrDefault(item =>
-					String.Equals(item.ToString(), txtBox.Text, StringComparison.CurrentCulture));
+					string.Equals(item.ToString(), txtBox.Text, StringComparison.CurrentCulture));
 
 				if (found == null)
 				{
 					// is the current text a match except for case for the autocomplete list?
-					found = list.FirstOrDefault(item => String.Equals(item.ToString(),
+					found = list.FirstOrDefault(item => string.Equals(item.ToString(),
 						txtBox.Text, StringComparison.CurrentCultureIgnoreCase));
 					if (found != null)
 					{
@@ -442,7 +508,19 @@ private void DeleteRow(int rowIndex)
 		/// <remarks>SP-874: Localize column headers</remarks>
 		public void SetColumnHeaderText(int columnIndex, string headerText)
 		{
-			_grid.Columns[columnIndex].HeaderText = headerText;
+			if (_model == null)
+			{
+				if (IsDisposed)
+					throw new ObjectDisposedException(Name ?? GetType().Name);
+
+				throw new InvalidOperationException(
+					$"{nameof(Initialize)} must be called to set the model first.");
+			}
+
+			if (InvokeRequired)
+				BeginInvoke(new Action(() => { _grid.Columns[columnIndex].HeaderText = headerText; }));
+			else
+				_grid.Columns[columnIndex].HeaderText = headerText;
 		}
 
 		/// <remarks>SP-874: Localize column headers</remarks>
@@ -453,7 +531,9 @@ public void SetLocalizationExtender(L10NSharpExtender extender)
 			extender.SetLocalizingId(_grid, "ContributorsEditorGrid");
 		}
 
-		/// <remarks>We need to be able to adjust the visual properties to match the hosting program</remarks>
+		/// <remarks>We need to be able to adjust the visual properties to match the hosting program.
+		/// If used in code that could run on a thread other than the main UI thread, caller is
+		/// responsible for invoking on UI thread if needed.</remarks>
 		public BetterGrid Grid => _grid;
 	}
 }

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

@@ -168,15 +168,15 @@ public static IAsyncResult SafeInvoke(this Control control, Action action, strin
 					// between the time we invoke and the time the action is executed. All we really need from SafeInvoke is that little
 					// bit of code at the start that checks for IsDisposed, but re-using SafeInvoke for the delegate is probably better
 					// than repeating that bit of code. Rechecking InvokeRequired should be lightning fast.
-					var delgate = (Action)delegate
+					var actionToInvoke = (Action)delegate
 					{
 						treatInvalidOperationExceptionAsObjectDisposedException = false;
 						control.SafeInvoke(action, nameForErrorReporting, errorHandling, forceSynchronous);
 					};
 					if (forceSynchronous)
-						control.Invoke(delgate);
+						control.Invoke(actionToInvoke);
 					else
-						return control.BeginInvoke(delgate);
+						return control.BeginInvoke(actionToInvoke);
 				}
 				catch (InvalidOperationException e)
 				{