Repaint when the picture changed and nothing else did (#842)

* Repaint when the picture changed and nothing else did

The Windows head repaints only when the new screen differs from the last one,
because ScreenBuilder allocates a fresh Screen every frame and record equality
would repaint sixty times a second. That comparison never looked at the image
pane, and an image pane said only where the file was and how big it was.

An image side's rows are format, dimensions and byte count, so a re-run that
rewrites a received image at the same size changes nothing in the model - for BMP,
being uncompressed, that is every re-run. Apply returned before invalidating, the
canvas never repainted, and the pane kept the previous picture. The stamp based
freshness check in ImageCache was never even consulted, since nothing asked it to
paint.

ImagePane carries the content hash the model already had, and the pane comparison
includes the image. The hash is not for drawing - a head reads the file - but for
telling one picture from another at the same path and size.

* Say the hash in the pane the test asserts on

Author: SimonCropp

src/DiffEngineViewer.Tests/ImageScreenTests.cs

@@ -99,8 +99,10 @@ public Task DeleteInQueue() =>
     public async Task PanesCarryThePicture()
     {
         var screen = ScreenBuilder.Build(State(Received(), Expected()));
-        await Assert.That(screen.Left.Image).IsEqualTo(new("temp/sample.received.png", 800, 600));
-        await Assert.That(screen.Right.Image).IsEqualTo(new("code/sample.verified.png", 800, 600));
+        // The hash the side carried, which is how a head tells one picture from another at the
+        // same path and size - and how the Windows head knows to repaint for it
+        await Assert.That(screen.Left.Image).IsEqualTo(new("temp/sample.received.png", 800, 600, "0A"));
+        await Assert.That(screen.Right.Image).IsEqualTo(new("code/sample.verified.png", 800, 600, "0A"));
     }
 
     [Test]

src/DiffEngineViewer.Windows.Tests/PaneChangeTests.cs

@@ -0,0 +1,56 @@
+/// <summary>
+/// Whether two panes are the same pane, which is what decides whether the window repaints.
+/// ScreenBuilder allocates a fresh Screen every frame, so record equality would report a change
+/// sixty times a second and the comparison is by hand.
+/// </summary>
+public class PaneChangeTests
+{
+    /// <summary>
+    /// The rows an image side shows are format, dimensions and byte count, and a re-run that
+    /// rewrites a received image at the same size changes none of them - for BMP, which is
+    /// uncompressed, that is every re-run. So nothing about the screen differed, Apply returned
+    /// before repainting, and the pane kept the previous picture while the rows beside it
+    /// described the new one.
+    /// </summary>
+    [Test]
+    public async Task A_picture_that_changed_is_not_the_same_pane()
+    {
+        var before = ImagePane("A1B2");
+        var after = ImagePane("C3D4");
+
+        await Assert.That(ViewerForm.Same(before, after)).IsFalse();
+    }
+
+    [Test]
+    public async Task A_picture_that_did_not_change_is_the_same_pane() =>
+        await Assert.That(ViewerForm.Same(ImagePane("A1B2"), ImagePane("A1B2"))).IsTrue();
+
+    [Test]
+    public async Task A_picture_replaced_by_one_of_another_size_is_not_the_same_pane()
+    {
+        var before = ImagePane("A1B2");
+        var after = before with
+        {
+            Image = new("sample.received.png", 20, 10, "A1B2")
+        };
+
+        await Assert.That(ViewerForm.Same(before, after)).IsFalse();
+    }
+
+    [Test]
+    public async Task Text_panes_are_unaffected()
+    {
+        var pane = new Pane("received", [new(1, RowKind.Unchanged, "one")], 0, 1);
+
+        await Assert.That(ViewerForm.Same(pane, pane with { })).IsTrue();
+        await Assert.That(ViewerForm.Same(pane, pane with { ScrollTop = 1 })).IsFalse();
+    }
+
+    static Pane ImagePane(string hash) =>
+        new(
+            "received",
+            [],
+            0,
+            0,
+            new("sample.received.png", 10, 10, hash));
+}

src/DiffEngineViewer.Windows/ViewerForm.cs

@@ -422,9 +422,14 @@ left is null
               left.Row == right.Row &&
               left.Labels.SequenceEqual(right.Labels);
 
-    static bool Same(Pane left, Pane right) =>
+    internal static bool Same(Pane left, Pane right) =>
         left.Header == right.Header &&
         left.ScrollTop == right.ScrollTop &&
         left.TotalRows == right.TotalRows &&
+        // Records all the way down, so this compares the path, the size and the content stamp. A
+        // re-run that rewrites a received image at the same size changes nothing else about the
+        // screen - the rows say format, dimensions and byte count, and for BMP those hold - so
+        // without it Apply returned before repainting and the pane kept the previous picture.
+        left.Image == right.Image &&
         left.Rows.SequenceEqual(right.Rows);
 }

src/DiffEngineViewer/Model/ImagePane.cs

@@ -13,4 +13,10 @@
 /// worth trying.
 /// </para>
 /// </summary>
-record ImagePane(string Path, int Width, int Height);
+/// <param name="Hash">
+/// What the file held when the model was built, so a head can tell one picture from another at the
+/// same path and size. Not for drawing - the head reads the file - but for deciding whether what
+/// is on screen is still this. A re-run that rewrites a received image at the same dimensions
+/// changes nothing else in the model, and for BMP that is every re-run.
+/// </param>
+record ImagePane(string Path, int Width, int Height, string? Hash);

src/DiffEngineViewer/ScreenBuilder.cs

@@ -95,7 +95,7 @@ static Pane BuildPane(
             return null;
         }
 
-        return new(file.Path, header.Width, header.Height);
+        return new(file.Path, header.Width, header.Height, file.Hash);
     }
 
     static IReadOnlyList<QueueItem> BuildQueue(SessionState state, int body, out int top)