[SIL.Windows.Forms] Fix ImageCropper resource leaks and cropping correctness bugs

CHANGELOG.md

@@ -44,6 +44,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - [SIL.Windows.Forms] Updated ImageToolbox UI to consistently use "image" (not "picture").
 - [SIL.Windows.Forms.Keyboarding] Removed Timer-based deferred IME conversion status restore from WindowsKeyboardSwitchingAdapter, which disrupted active Chinese Pinyin IME compositions (LT-22442). Added diagnostic tracing for keyboard switching and IME state.
 - [SIL.DictionaryServices] Fix memory leak in LiftWriter
+- [SIL.Windows.Forms] ImageCropper now unsubscribes from `Application.Idle` when disposed, fixing a handler leak that could fire on a disposed cropper
+- [SIL.Windows.Forms] Fixed ImageCropper `Image` setter leaking the previous temp file and cropping image on re-set; fields are now nulled after disposal so a mid-setter failure does not leave disposed-but-non-null references
+- [SIL.Windows.Forms] Fixed ImageCropper not downscaling tall images before cropping (height condition was checking width)
+- [SIL.Windows.Forms] Fixed ImageCropper `GetCroppedImage` throwing `NullReferenceException` when `Image` property is set directly rather than via `SetImage`
 - [SIL.WritingSystems] Fix IetfLanguageTag.GetGeneralCode to handle cases when zh-CN or zh-TW is a prefix and not the whole string.
 - [SIL.WritingSystems] More fixes to consistently use 繁体中文 and 简体中文 for Traditional and Simplified Chinese native language names, and Chinese (Traditional) and Chinese (Simplified) for their English names.
 - [SIL.Windows.Forms] Prevent BetterLabel from responding to OnTextChanged when it has been disposed.

SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs

@@ -0,0 +1,110 @@
+using System.Drawing;
+using System.Drawing.Imaging;
+using System.IO;
+using System.Threading;
+using NUnit.Framework;
+using SIL.IO;
+using SIL.Windows.Forms.ImageToolbox;
+using SIL.Windows.Forms.ImageToolbox.Cropping;
+
+namespace SIL.Windows.Forms.Tests.ImageToolbox
+{
+	[Apartment(ApartmentState.STA)]
+	[TestFixture]
+	public class ImageCropperTests
+	{
+		[Test]
+		public void Dispose_CalledTwice_DoesNotThrow()
+		{
+			var cropper = new ImageCropper();
+			Assert.DoesNotThrow(() => cropper.Dispose());
+			Assert.DoesNotThrow(() => cropper.Dispose());
+		}
+
+		[Test]
+		public void GetCroppedImage_PngImage_ReturnsUsableBitmap()
+		{
+			using (var tempFile = TempFile.WithExtension(".png"))
+			{
+				using (var bmp = new Bitmap(100, 80))
+					bmp.Save(tempFile.Path, ImageFormat.Png);
+
+				using (var palasoImage = PalasoImage.FromFile(tempFile.Path))
+				using (var cropper = new ImageCropper())
+				{
+					cropper.Size = new Size(400, 300);
+					cropper.SetImage(palasoImage);
+
+					using (var result = cropper.GetCroppedImage())
+					{
+						Assert.IsNotNull(result);
+						Assert.Greater(result.Width, 0);
+						// Re-encode to force GDI+ to re-read the pixel data from the backing store.
+						using (var ms = new MemoryStream())
+							Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png));
+					}
+				}
+			}
+		}
+
+		[Test]
+		public void GetCroppedImage_JpegImage_SetViaPropertyDirectly_ReturnsJpegBitmap()
+		{
+			// Setting Image directly (not via SetImage) must still initialize _originalFormat so
+			// GetCroppedImage does not throw NullReferenceException on JPEG re-encoding.
+			using (var tempFile = TempFile.WithExtension(".jpg"))
+			{
+				using (var bmp = new Bitmap(100, 80))
+					bmp.Save(tempFile.Path, ImageFormat.Jpeg);
+
+				using (var palasoImage = PalasoImage.FromFile(tempFile.Path))
+				using (var cropper = new ImageCropper())
+				{
+					cropper.Size = new Size(400, 300);
+					cropper.Image = palasoImage; // bypass SetImage intentionally
+
+					using (var result = cropper.GetCroppedImage())
+					{
+						Assert.IsNotNull(result);
+						Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid);
+						Assert.Greater(result.Width, 0);
+						// Re-encode to force GDI+ to re-read the pixel data. The JPEG path returns an
+						// image built from a MemoryStream that GetCroppedImage disposes, so this guards
+						// against that disposal corrupting the returned bitmap.
+						using (var ms = new MemoryStream())
+							Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Jpeg));
+					}
+				}
+			}
+		}
+
+		[Test]
+		public void SetImage_Reassign_UpdatesFormatAndDoesNotThrow()
+		{
+			using (var tempFile1 = TempFile.WithExtension(".png"))
+			using (var tempFile2 = TempFile.WithExtension(".jpg"))
+			{
+				using (var bmp = new Bitmap(100, 80))
+				{
+					bmp.Save(tempFile1.Path, ImageFormat.Png);
+					bmp.Save(tempFile2.Path, ImageFormat.Jpeg);
+				}
+
+				using (var img1 = PalasoImage.FromFile(tempFile1.Path))
+				using (var img2 = PalasoImage.FromFile(tempFile2.Path))
+				using (var cropper = new ImageCropper())
+				{
+					cropper.Size = new Size(400, 300);
+					cropper.SetImage(img1);
+					cropper.SetImage(img2);
+
+					using (var result = cropper.GetCroppedImage())
+					{
+						Assert.IsNotNull(result);
+						Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid);
+					}
+				}
+			}
+		}
+	}
+}

SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs

@@ -145,6 +145,12 @@
 				if (value == null)
 					return;
 
+				_savedOriginalImage?.Dispose();
+				_savedOriginalImage = null;
+				_croppingImage?.Dispose();
+				_croppingImage = null;
+				_originalFormat = value.Image.RawFormat;
+
 				//other code changes the image of this palaso image, at which time the PI disposes of its copy,
 				//so we better keep our own.
 
@@ -153,7 +159,7 @@
 				value.Image.Save(_savedOriginalImage.Path, ImageFormat.Png);
 
 				// make a reasonable sized copy to crop
-				if ((value.Image.Width > 1000) || (value.Image.Width > 1000))
+				if ((value.Image.Width > 1000) || (value.Image.Height > 1000))
 				{
 					_croppingImage = CreateCroppingImage(value.Image.Height, value.Image.Width);
 
@@ -267,6 +273,8 @@
 
 		private void CalculateSourceImageArea()
 		{
+			if (_croppingImage == null)
+				return;
 			float imageToCanvaseScaleFactor = GetImageToCanvasScaleFactor(_croppingImage);
 			_sourceImageArea = new Rectangle(GripThickness, GripThickness,
 											 (int)(_croppingImage.Width*imageToCanvaseScaleFactor),
@@ -418,14 +426,15 @@
 
 					var cropped = originalImage.Clone(selection, originalImage.PixelFormat); //do the actual cropping
 
 					if (_originalFormat.Guid == ImageFormat.Jpeg.Guid)
 					{
 						//We've sadly lost our jpeg formatting, so now we encode a new image in jpeg
 						using (var stream = new MemoryStream())
 						{
 							cropped.Save(stream, ImageFormat.Jpeg);
+							stream.Position = 0;
 							var oldCropped = cropped;
-							cropped = System.Drawing.Image.FromStream(stream) as Bitmap;
+							cropped = (Bitmap)System.Drawing.Image.FromStream(stream);
 							oldCropped.Dispose();
 							Require.That(ImageFormat.Jpeg.Guid == cropped.RawFormat.Guid, "lost jpeg formatting");
 						}
@@ -449,7 +458,6 @@
 			}
 			else
 			{
-				_originalFormat = image.Image.RawFormat;
 				Image = image;
 			}
 		}
@@ -482,6 +490,8 @@
 					components = null;
 				}
 
+				Application.Idle -= Application_Idle;
+
 				try
 				{
 					if (_savedOriginalImage != null)