search

sillsdev / libpalaso

Palaso Library: A set of .Net libraries useful for developers of Language Software.
MIT License
44 stars 50 forks source link

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

Closed tombogle closed 6 months ago

tombogle commented 6 months ago

This change is Reviewable

tombogle commented 6 months ago

SIL.Windows.Forms/Progress/LogBox.cs line 290 at r1 (raw file):

Previously, andrew-polk wrote…
It is not clear to me why you would not allow the code to add more messages after the operation is cancelled. The obvious example where you would want to do so is to add a message stating the operation has been cancelled. But seems like there could be others. But maybe this only applies to an error condition? Still I'm not convinced you wouldn't want to log an error after the user cancelled. At a more meta level, should a method named something as generic as "SafeInvoke" never complete because the user cancelled an operation?

Fair questions. At least in HearThis, the dialog box that contains the log box is getting disposed when the user cancels. But maybe that would not always be the case. Perhaps I should only do this if it's disposed or disposing.

tombogle commented 6 months ago

SIL.Core/CommandLineProcessing/CommandLineRunner.cs line 314 at r2 (raw file):

Previously, andrew-polk wrote…
This seems like a logical change. And the only users of this method (AFAICT via github) are HearThis and Bloom. Do you need to do anything in HearThis to account for this? If so, Bloom does too as it looks like our calling code was copied from HearThis.

Good sleuthing. I'm making the change in order to try to prevent a current crashing bug in HearThis whenever the user cancels an export. I don't think I'll need to make any changes in HearThis itself, but I'm not 100% sure. Because it is a functional change that could theoretically break something, maybe it should be semver:major. I think it will already be part of a major version change when released.