refactor screenshot/bitmap handling to immediately dispose when done

Author: PandaWood

src/ExceptionReporter/Core/ScreenshotTaker.cs

@@ -7,8 +7,7 @@ namespace ExceptionReporting.Core
 {
 	internal interface IScreenshotTaker
 	{
-		Bitmap TakeScreenShot();
-		string GetImageAsFile(Bitmap bitmap);
+		string TakeScreenShot();
 	}
 
 	/// <summary>
@@ -19,8 +18,8 @@ internal class ScreenshotTaker : IScreenshotTaker
 		private const string ScreenshotFileName = "exceptionreport-screenshot.jpg";
 
 		/// <summary> Take a screenshot (supports multiple monitors) </summary>
-		/// <returns>Bitmap of the screen, as at the time called</returns>
-		public Bitmap TakeScreenShot()
+		/// <returns>temp file name of JPEG image</returns>
+		public string TakeScreenShot()
 		{
       if (ExceptionReporter.IsRunningMono()) return null;
 
@@ -31,24 +30,25 @@ public Bitmap TakeScreenShot()
 				rectangle = Rectangle.Union(rectangle, screen.Bounds);
 			}
 
-			var bitmap = new Bitmap(rectangle.Width, rectangle.Height, PixelFormat.Format32bppArgb);
-
-			using (var graphics = Graphics.FromImage(bitmap))
+			using (var bitmap = new Bitmap(rectangle.Width, rectangle.Height, PixelFormat.Format32bppArgb))
 			{
-				graphics.CopyFromScreen(rectangle.X, rectangle.Y, 0, 0, rectangle.Size, CopyPixelOperation.SourceCopy);
-			}
 
-			return bitmap;
+				using (var graphics = Graphics.FromImage(bitmap))
+				{
+					graphics.CopyFromScreen(rectangle.X, rectangle.Y, 0, 0, rectangle.Size, CopyPixelOperation.SourceCopy);
+				}
+
+				return GetImageAsFile(bitmap);
+			}
 		}
 
 		/// <summary>
 		/// Return the supplied Bitmap, as a file on the system, in JPEG format
 		/// </summary>
 		/// <param name="bitmap">The Bitmap to save</param>
-		/// <returns></returns>
-		public string GetImageAsFile(Bitmap bitmap)
+		private static string GetImageAsFile(Image bitmap)
 		{
-			var tempFileName = Path.GetTempPath() + ScreenshotFileName;		// the image is not deleted but the same file is reused every time
+			var tempFileName = Path.GetTempPath() + ScreenshotFileName; // image is not deleted but same file is reused
 			bitmap.Save(tempFileName, ImageFormat.Jpeg);
 			return tempFileName;
 		}

src/ExceptionReporter/ExceptionReportInfo.cs

@@ -216,24 +216,11 @@ public bool ShowEmailButton
 		/// </summary>
 		public bool TakeScreenshot { get; set; } = false;
 
-		/// <summary>
-		/// The Screenshot Bitmap, used internally but exposed for flexibility
-		/// </summary>
-		public Bitmap ScreenshotImage { get; set; }
-
 		/// <summary>
 		/// The method used to send the report
 		/// </summary>
 		public ReportSendMethod SendMethod { get; set; } = ReportSendMethod.None;
 
-		/// <summary>
-		/// Whether a screenshot is configured to be taken and that it has been taken - used internally
-		/// </summary>
-		public bool ScreenshotAvailable
-		{
-			get { return TakeScreenshot && ScreenshotImage != null; }
-		}
-
 		/// <summary>
 		/// Show the Exception Reporter as a "TopMost" window (ie TopMost property on a WinForm)
 		/// This can be quite important in some environments (eg Office Addins) where it might get covered by other UI
@@ -314,12 +301,6 @@ public enum EmailMethod
 
 		[Obsolete("use 'SendMethod' property instead")]
 		public EmailMethod MailMethod { get; set; }
-
-		protected override void DisposeManagedResources()
-		{
-			ScreenshotImage?.Dispose();
-			base.DisposeManagedResources();
-		}
 	}
 
 	/// <summary>

src/ExceptionReporter/Mail/Attacher.cs

@@ -29,15 +29,8 @@ public void AttachFiles(IAttach attacher)
 
 			try
 			{
-				if (_config.TakeScreenshot && !_config.ScreenshotAvailable)
-					_config.ScreenshotImage = ScreenshotTaker.TakeScreenShot();
-			}
-			catch { /* ignored */ }
-
-			if (_config.ScreenshotAvailable)
-			{
-				files.Add(ScreenshotTaker.GetImageAsFile(_config.ScreenshotImage));
-			}
+				if (_config.TakeScreenshot) files.Add(ScreenshotTaker.TakeScreenShot());
+			} catch { /* ignored */ }
 
 			var filesThatExist = files.Where(f => File.Exists(f)).ToList();
 

src/Test.ExceptionReporter/Attacher_Tests.cs

@@ -1,4 +1,3 @@
-using System.Drawing;
 using ExceptionReporting.Core;
 using ExceptionReporting.Mail;
 using Moq;
@@ -144,28 +143,5 @@ public void Should_Not_Take_Screenshot_If_Not_Configured()
 
 			screenshot.Verify(s => s.TakeScreenShot(), Times.Never);
 		}
-
-		/// <summary>
-		/// There's not actually a pathway in the code to have a screenshot "already taken" - but I think this is a worthwhile guard
-		/// </summary>
-		[Test]
-		public void Should_Not_Take_Screenshot_If_Already_Taken()
-		{
-			var screenshot = new Mock<IScreenshotTaker>();
-
-			using (var bm = new Bitmap(1, 1))
-			{
-				var attacher =
-					new Attacher(new ExceptionReportInfo
-					{
-						TakeScreenshot = true,
-						ScreenshotImage = bm		// this makes the screenshot "already taken" ie exists
-					}) { ScreenshotTaker = screenshot.Object };
-
-				attacher.AttachFiles(_iattach.Object);
-			}
-
-			screenshot.Verify(s => s.TakeScreenShot(), Times.Never);
-		}
 	}
 }