diff --git a/CHANGELOG.md b/CHANGELOG.md index 517764067..e79f3ff00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,11 @@ 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] Fixed ImageCropper crash when switching between Crop and Choose tabs: `Application.Idle` handler was never unsubscribed, causing it to fire on a disposed object +- [SIL.Windows.Forms] Fixed ImageCropper `GetCroppedImage` returning a JPEG-format bitmap backed by a prematurely disposed `MemoryStream` +- [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. diff --git a/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs new file mode 100644 index 000000000..d9458ef8c --- /dev/null +++ b/SIL.Windows.Forms.Tests/ImageToolbox/ImageCropperTests.cs @@ -0,0 +1,171 @@ +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_ReturnsJpegBitmapUsableAfterReturn() + { + 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.SetImage(palasoImage); + + Image result; + // GetCroppedImage returns a Bitmap backed by a MemoryStream for JPEG. + // The stream must still be alive after the method returns. + result = cropper.GetCroppedImage(); + try + { + Assert.IsNotNull(result); + Assert.AreEqual(ImageFormat.Jpeg.Guid, result.RawFormat.Guid); + // Re-encode to force GDI+ to re-read the pixel data from the backing stream. + using (var ms = new MemoryStream()) + Assert.DoesNotThrow(() => result.Save(ms, ImageFormat.Png)); + } + finally + { + result?.Dispose(); + } + } + } + } + [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); + } + } + } + } + + [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); + } + } + } + } + + [Test] + public void SetImage_ReCropPreviouslyCroppedJpeg_DoesNotThrow() + { + // GetImage() returns a JPEG Bitmap backed by a MemoryStream; feeding that result + // back into a new cropper re-saves it in the Image setter (value.Image.Save), which + // crashed once the backing stream had been disposed. + using (var tempFile = TempFile.WithExtension(".jpg")) + { + using (var bmp = new Bitmap(1200, 900)) + bmp.Save(tempFile.Path, ImageFormat.Jpeg); + + // GetImage() returns the same PalasoImage instance, now holding the cropped JPEG, + // so the outer using disposes it exactly once. + using (var palasoImage = PalasoImage.FromFile(tempFile.Path)) + { + PalasoImage cropped; + using (var cropper1 = new ImageCropper()) + { + cropper1.Size = new Size(400, 300); + cropper1.SetImage(palasoImage); + cropped = cropper1.GetImage(); + } + Assert.That(cropped, Is.Not.Null); + + using (var cropper2 = new ImageCropper()) + { + cropper2.Size = new Size(400, 300); + Assert.DoesNotThrow(() => cropper2.SetImage(cropped)); + } + } + } + } + } +} diff --git a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs index 4ded520a6..834ebbf54 100644 --- a/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs +++ b/SIL.Windows.Forms/ImageToolbox/Cropping/ImageCropper.cs @@ -145,6 +145,12 @@ public PalasoImage Image 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 @@ public PalasoImage Image 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 ImageCropper_Resize(object sender, EventArgs e) private void CalculateSourceImageArea() { + if (_croppingImage == null) + return; float imageToCanvaseScaleFactor = GetImageToCanvasScaleFactor(_croppingImage); _sourceImageArea = new Rectangle(GripThickness, GripThickness, (int)(_croppingImage.Width*imageToCanvaseScaleFactor), @@ -421,14 +429,24 @@ public Image GetCroppedImage() 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()) + var stream = new MemoryStream(); + try { cropped.Save(stream, ImageFormat.Jpeg); + stream.Position = 0; + // Do not dispose stream on success: GDI+ bitmaps reference the stream for lazy + // decoding. The stream is collected when the returned Bitmap is disposed. 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"); } + catch + { + stream.Dispose(); + cropped.Dispose(); + throw; + } + Require.That(ImageFormat.Jpeg.Guid == cropped.RawFormat.Guid, "lost jpeg formatting"); } return cropped; } @@ -449,7 +467,6 @@ public void SetImage(PalasoImage image) } else { - _originalFormat = image.Image.RawFormat; Image = image; } } @@ -482,6 +499,8 @@ protected override void Dispose(bool disposing) components = null; } + Application.Idle -= Application_Idle; + try { if (_savedOriginalImage != null)