SP-2353 ContributorsListControl thread safety improvements #1401
Conversation
…adsafe Grid properties on non-UI thread
…w 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
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
SIL.Windows.Forms/ClearShare/WinFormsUI/ContributorsListControl.cs
Fixed
Show resolved
Hide resolved
SIL.Windows.Forms/ClearShare/WinFormsUI/ContributorsListControl.cs
Fixed
Show resolved
Hide resolved
|
|
||
| } | ||
|
|
||
| private System.ComponentModel.IContainer components = null; |
Check notice
Code scanning / CodeQL
Missed 'readonly' opportunity Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI 3 days ago
To fix the problem, we need to add the readonly modifier to the components field. This will ensure that the field cannot be modified after the object has been initialized, preventing unintended assignments and improving code safety.
- Locate the declaration of the
componentsfield in theContributorsForm.Designer.csfile. - Add the
readonlymodifier to thecomponentsfield declaration.
| @@ -136,3 +136,3 @@ | ||
|
|
||
| private System.ComponentModel.IContainer components = null; | ||
| private readonly System.ComponentModel.IContainer components = null; | ||
| private System.Windows.Forms.TableLayoutPanel _tableLayout; |
There was a problem hiding this comment.
This is in generated code.
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @tombogle)
SIL.Windows.Forms/ClearShare/WinFormsUI/ContributorsListControl.cs line 259 at r1 (raw file):
{ Logger.WriteError("Error updating contribution list", ex); // REVIEW: Then what? Retry every second until this succeeds???
Probably at least want something less silent?
And if we do think we will get here, probably worth documenting how/why.
…d improved error handling in HandleNewContributionListAvailable
There was a problem hiding this comment.
Reviewable status: 7 of 9 files reviewed, 5 unresolved discussions (waiting on @andrew-polk and @Github-advanced-security[bot])
SIL.Windows.Forms/ClearShare/WinFormsUI/ContributorsListControl.cs line 259 at r1 (raw file):
Previously, andrew-polk wrote…
Probably at least want something less silent?
And if we do think we will get here, probably worth documenting how/why.
Done.
SIL.Windows.Forms/ClearShare/WinFormsUI/ContributorsListControl.cs
Fixed
Show resolved
Hide resolved
SIL.Windows.Forms/ClearShare/WinFormsUI/ContributorsListControl.cs
Fixed
Show resolved
Hide resolved
|
|
||
| } | ||
|
|
||
| private System.ComponentModel.IContainer components = null; |
There was a problem hiding this comment.
This is in generated code.
| catch (Exception exception) | ||
| { | ||
| Logger.WriteError(exception); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix AI about 12 hours ago
To fix the problem, we should replace the generic catch clause with specific exception types that are expected to be thrown by the code within the try block. In this case, we should catch exceptions that are likely to be thrown by Graphics.FromHwnd and LocalizationManager.GetString. These methods can throw ArgumentException, InvalidOperationException, and OutOfMemoryException. We will catch these specific exceptions and log them accordingly.
| @@ -297,3 +297,11 @@ | ||
| } | ||
| catch (Exception exception) | ||
| catch (ArgumentException exception) | ||
| { | ||
| Logger.WriteError(exception); | ||
| } | ||
| catch (InvalidOperationException exception) | ||
| { | ||
| Logger.WriteError(exception); | ||
| } | ||
| catch (OutOfMemoryException exception) | ||
| { |
There was a problem hiding this comment.
It's okay. I don't want to make things worse if something goes wrong trying to display the error message.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Github-advanced-security[bot] and @tombogle)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Github-advanced-security[bot])
| catch (Exception exception) | ||
| { | ||
| Logger.WriteError(exception); | ||
| } |
There was a problem hiding this comment.
It's okay. I don't want to make things worse if something goes wrong trying to display the error message.
|
|
||
| } | ||
|
|
||
| private System.ComponentModel.IContainer components = null; |
…claimed previous implementation might cause problems, but I don't think that's true.) Removed outdated exception comment.
There was a problem hiding this comment.
Reviewable status: 9 of 10 files reviewed, 6 unresolved discussions (waiting on @Github-advanced-security[bot] and @tombogle)
| { | ||
| return Equals(obj as Subtag); | ||
| } | ||
| public override bool Equals(object obj) => obj is Subtag subtag && Equals(subtag); |
Check warning
Code scanning / CodeQL
Equals should not apply "is" Warning
Made ContributorsListControl more threadsafe. Possibly fixes crashes like SP-2353 or at least makes them less likely.
Added ContributorsListControl.Initialize method to allow the model to be set later when using the (existing) parameterless constructor in Designer.
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
This change is