Fix more problems with LT-22357#874
Conversation
johnml1135
left a comment
johnml1135
left a comment
There was a problem hiding this comment.
Draft review comments
| m_synchronizeInvoke.Invoke((Action)m_progressDialog.Close, null); | ||
| else | ||
| m_progressDialog.Close(); | ||
| if (m_progressDialog != null) |
There was a problem hiding this comment.
drafted with the help of GPT 5.5: The new m_progressDialog != null guard helps, but this is still a check-then-use race: the dialog/control can be disposed after the null check and before Invoke / Close. Could this close path use the same scoped teardown-safe helper as the property setters, or at least catch the same expected teardown exceptions here too?
| progress.Message = xWorksStrings.ksObtainingEntriesToDisplay; | ||
| var configuration = new DictionaryConfigurationModel(configurationFile, Cache); | ||
| publicationDecorator.Refresh(); | ||
| if (Clerk.IsDisposed) |
There was a problem hiding this comment.
drafted with the help of GPT 5.5: Could we capture Clerk once and use that same reference for the disposed check, VirtualFlid, and the later SavePreviewHtmlWithStyles call? As written we check Clerk.IsDisposed, then later re-read Clerk.VirtualFlid, then pass Clerk again, which makes the teardown lifetime harder to reason about.
| else | ||
| m_progressDialog.Message = value; | ||
| } | ||
| catch (Exception e) |
There was a problem hiding this comment.
drafted with the help of GPT 5.5: Optional: would it be worth adding a focused regression test around the progress-dialog wrapper with a synchronizing control/object that tears down mid-update? Even if the full Dictionary View import flow is hard to automate, a small test here would lock in that expected teardown exceptions are ignored while unexpected ones still surface.
| else | ||
| m_progressDialog.Message = value; | ||
| } | ||
| catch (Exception e) |
There was a problem hiding this comment.
drafted with the help of GPT 5.5: Could we narrow this catch to the expected teardown exceptions instead of catching all Exception? The evidence here seems pretty specific: CheckDisposed() in this class throws ObjectDisposedException, Dispose(bool) nulls out m_progressDialog/m_worker, and the WinForms Control.BeginInvoke docs explicitly call out InvalidOperationException when no appropriate window handle can be found. The Control.Invoke / InvokeRequired docs also note exceptions if no appropriate handle exists or if the UI thread is no longer active. Unexpected exceptions here would still be useful to surface.
| { | ||
| return null; | ||
| } | ||
| var entriesToPublish = publicationDecorator.GetEntriesToPublish(m_propertyTable, Clerk.VirtualFlid); |
There was a problem hiding this comment.
drafted with the help of GPT 5.5: In this same method we still do progress.Position++, but Position is not covered by the new LT-22357 handling and ++ here is a cross-thread read/modify/write. Would it be cleaner to switch callers like this to Step(1) and then make ProgressDialogWithTask.Step use the same teardown-safe helper as Message / Minimum / Maximum? That would collapse this to one marshaled UI operation instead of a getter+setter pair.
2 participants
This fixes more problems with https://jira.sil.org/browse/LT-22357 by widening the scope of try ... catch expressions and adding more guards.
This change is