Shrink large images, reporting wherever the caller already reports (BL-16646)

src/BloomExe/Book/Book.cs

@@ -1107,7 +1107,7 @@ public void EnsureUpToDate(IProgress progress = null, bool forCopyOfUpToDateBook
             EnsureUpToDateMemory(progress);
             UpdateSupportFiles();
 
-            Storage.MigrateToMediaLevel1ShrinkLargeImages();
+            Storage.MigrateToMediaLevel1ShrinkLargeImages(progress);
 
             Storage.CleanupUnusedSupportFiles(forCopyOfUpToDateBook);
 
@@ -1895,7 +1895,7 @@ public void EnsureUpToDateMemory(IProgress progress)
             // already been done, so they must be called in exactly this order.
             Storage.RestoreStuffBeforeMigration();
             Storage.MigrateMaintenanceLevels();
-            Storage.MigrateToMediaLevel1ShrinkLargeImages();
+            Storage.MigrateToMediaLevel1ShrinkLargeImages(progress);
             Storage.MigrateToLevel2RemoveTransparentComicalSvgs();
             Storage.MigrateToLevel3PutImgFirst();
             Storage.MigrateToLevel4UseAppearanceSystem();

src/BloomExe/Book/BookProcessor.cs

@@ -88,13 +88,13 @@ public static int ProcessBook(Book book, bool fitImageTextSplits = false)
             // BookStorage.MigrateToMediaLevel1ShrinkLargeImages, won't help here: the bridge HTML
             // already carries a modern maintenance level, so BringBookUpToDate below treats that
             // migration as already done and skips it. So we do the shrink ourselves, unconditionally
-            // (not gated by mediaMaintenanceLevel), and it must come BEFORE BringBookUpToDate: (a) on
-            // a book old enough that the migration WOULD run, it normally finds nothing left to
-            // shrink, so its modal progress dialog is not created on this background thread (where a
-            // WinForms dialog is illegal, BL-16646) -- "normally" because a GraphicsMagick failure can
-            // leave an image oversized, in which case that narrow old-book case can still hit the
-            // dialog; (b) the off-screen per-page fix-up then measures and lays out against the
-            // final, already-shrunk images.
+            // (not gated by mediaMaintenanceLevel), and it must come BEFORE BringBookUpToDate so that
+            // the off-screen per-page fix-up measures and lays out against the final, already-shrunk
+            // images. (This used to have a second reason -- keeping the migration from creating its
+            // modal progress dialog on this background thread, which WinForms forbids. Since BL-16646
+            // the migration only creates that dialog when it is already on the UI thread, and reports
+            // through the caller's IProgress otherwise, so it could no longer do that here anyway and
+            // only the layout reason remains.)
             if (ImageUtils.NeedToShrinkImages(book.FolderPath))
             {
                 Log("shrinking oversized images in the book folder");

src/BloomExe/Book/BookStorage.cs

@@ -7,6 +7,7 @@
 using System.IO;
 using System.Linq;
 using System.Net;
+using System.Runtime.ExceptionServices;
 using System.Security;
 using System.Text;
 using System.Text.RegularExpressions;
@@ -107,7 +108,7 @@ void CleanupUnusedSupportFiles(
         void CaptureInitialStateForMigration();
         void RestoreStuffBeforeMigration();
         void MigrateMaintenanceLevels();
-        void MigrateToMediaLevel1ShrinkLargeImages();
+        void MigrateToMediaLevel1ShrinkLargeImages(IProgress progress);
         void MigrateToLevel2RemoveTransparentComicalSvgs();
         void MigrateToLevel3PutImgFirst();
 
@@ -4047,7 +4048,13 @@ public void MigrateMaintenanceLevels()
         /// memory. However, it is still helpful for performance and reducing published file sizes.
         /// Does nothing if mediaMaintenanceLevel indicates it has already been done.
         /// </summary>
-        public void MigrateToMediaLevel1ShrinkLargeImages()
+        /// <param name="progress">Where to report the (potentially very slow) shrinking, so the user
+        /// can see why we are busy. This is used when we are already off the UI thread, or headless,
+        /// or under test: the messages then land in whatever progress the caller already owns
+        /// (headless callers pass a NullProgress and so report nothing). When we are ON the UI thread
+        /// we put up our own dialog instead and this is not used -- see the branch below. Must not be
+        /// null: ImageUtils.FixSizeAndTransparencyOfImagesInFolder dereferences it.</param>
+        public void MigrateToMediaLevel1ShrinkLargeImages(IProgress progress)
         {
             var levelString = Dom.GetMetaValue("mediaMaintenanceLevel", "0");
             if (!int.TryParse(levelString, out int level))
@@ -4058,10 +4065,18 @@ public void MigrateToMediaLevel1ShrinkLargeImages()
             {
                 // If the book contains overlarge images, we want to fix those before editing because this can lead
                 // to thumbnails not being created properly and other bad behavior.  This is a one-time fix that can
-                // permanently change the images in the original book folder.  If any images must be shrunk, then a
-                // progress dialog pops up because that can be a very slow process.  If nothing needs to be done,
-                // nothing will appear on the screen, and it usually takes a small fraction of a second to determine
-                // this.
+                // permanently change the images in the original book folder.  Shrinking can be very slow, so we
+                // always report it somewhere -- but which way round depends on the thread we are on, because
+                // WinForms only allows a Form to be created on the UI thread.  Creating it anywhere else was the
+                // bug behind BL-16646.
+                //   - Already off the UI thread: the caller got here from something that is itself reporting
+                //     progress (a progress dialog's background worker, or a websocket progress), so we hand our
+                //     messages to the progress it passed us and add no window of our own.
+                //   - On the UI thread: there is no such progress to borrow, and doing the work inline would
+                //     freeze Bloom for the duration, so we put up our own dialog, which runs the work on a
+                //     background worker and keeps the UI alive.
+                // If nothing needs shrinking, nothing is reported at all, and it usually takes a small fraction
+                // of a second to determine that.
 
                 // Bloom 4.9 and later limit images used by Bloom books to be no larger than 3500x2550 in
                 // order to avoid out of memory errors that can happen with really large images.
@@ -4073,35 +4088,73 @@ public void MigrateToMediaLevel1ShrinkLargeImages()
                 // This update can be very slow, so encourage the user that something is happening.
                 // NO images should have transparency removed.  See https://issues.bloomlibrary.org/youtrack/issue/BL-8846.
 
-                if (Program.RunningUnitTests)
+                var shell = Shell.GetShellOrOtherOpenForm();
+                // shell is null when no window is open at all -- the bulk-upload and hydrate CLI
+                // commands. There is nothing to show a dialog on and no thread affinity to respect,
+                // so use the caller's progress like any other off-the-UI-thread case.
+                if (
+                    Program.RunningUnitTests
+                    || progress is WebProgressAdapter
+                    || shell == null
+                    || shell.InvokeRequired
+                )
                 {
-                    // TeamCity enforces not showing modal dialogs during unit tests on Windows 10.
                     ImageUtils.FixSizeAndTransparencyOfImagesInFolder(
                         FolderPath,
                         new List<string>(),
-                        new NullProgress()
+                        progress
                     );
                 }
                 else
                 {
-                    using (var dlg = new ProgressDialogBackground())
-                    {
-                        dlg.Text = "Updating Image Files";
-                        dlg.ShowAndDoWork(
-                            (progress, args) =>
-                                ImageUtils.FixSizeAndTransparencyOfImagesInFolder(
-                                    FolderPath,
-                                    new List<string>(),
-                                    progress
-                                )
-                        );
-                    }
+                    // InvokeRequired was false, so we are on the shell's own thread and may create
+                    // the dialog right here; no marshalling needed.
+                    ShrinkImagesBehindProgressDialog();
                 }
             }
-
             Dom.UpdateMetaElement("mediaMaintenanceLevel", "1");
         }
 
+        /// <summary>
+        /// Shrink this book's overlarge images behind our own "Updating Image Files" dialog, which runs
+        /// the work on a background worker so Bloom stays responsive while it happens. Must be called
+        /// on the UI thread: WinForms does not allow creating a Form anywhere else.
+        /// </summary>
+        /// <remarks>
+        /// ProgressDialogBackground never reads RunWorkerCompletedEventArgs.Error, so an exception
+        /// thrown by the work would otherwise disappear and we would carry on and record the book as
+        /// migrated when its images were not in fact shrunk -- permanently, since the level is never
+        /// revisited. So capture it and rethrow it here, which both reports the failure and leaves
+        /// mediaMaintenanceLevel alone, exactly as the direct (no-dialog) path does.
+        /// </remarks>
+        private void ShrinkImagesBehindProgressDialog()
+        {
+            Exception errorInWorker = null;
+            using (var dlg = new ProgressDialogBackground())
+            {
+                dlg.Text = "Updating Image Files";
+                dlg.ShowAndDoWork(
+                    (dialogProgress, args) =>
+                    {
+                        try
+                        {
+                            ImageUtils.FixSizeAndTransparencyOfImagesInFolder(
+                                FolderPath,
+                                new List<string>(),
+                                dialogProgress
+                            );
+                        }
+                        catch (Exception e)
+                        {
+                            errorInWorker = e;
+                        }
+                    }
+                );
+            }
+            if (errorInWorker != null)
+                ExceptionDispatchInfo.Capture(errorInWorker).Throw();
+        }
+
         private int GetMaintenanceLevel()
         {
             var levelString = Dom.GetMetaValue("maintenanceLevel", "0");

src/BloomExe/CollectionTab/CollectionModel.BloomSourceImport.cs

@@ -154,7 +154,7 @@ public void ImportBloomSourceFiles(
                     {
                         // The derivative path builds its Book in memory and adds it to the collection
                         // itself, so it needs no reload.
-                        var book = MakeDerivativeFromBloomSourceFile(path);
+                        var book = MakeDerivativeFromBloomSourceFile(path, progress);
                         if (book != null)
                             lastImported = book;
                     }
@@ -561,7 +561,10 @@ out string instanceId
         /// creation was cancelled (e.g. a template configuration dialog). Because a derivative always
         /// gets a fresh id, there is never a duplicate to resolve.
         /// </summary>
-        internal Book.Book MakeDerivativeFromBloomSourceFile(string sourcePath)
+        internal Book.Book MakeDerivativeFromBloomSourceFile(
+            string sourcePath,
+            IWebSocketProgress progress
+        )
         {
             var tempFolder = ExtractAndPrepareBloomSourceToTemp(sourcePath, out _, out _);
             try
@@ -589,7 +592,10 @@ internal Book.Book MakeDerivativeFromBloomSourceFile(string sourcePath)
                     newBook.BookData.Language1Tag
                 );
 
-                newBook.BringBookUpToDate(new NullProgress(), false);
+                newBook.BringBookUpToDate(
+                    progress == null ? new NullProgress() : new WebProgressAdapter(progress),
+                    false
+                );
 
                 TheOneEditableCollection.AddBookInfo(newBook.BookInfo);
                 newBook.RecordPendingCreatedHistoryEvent();

src/BloomExe/web/WebProgressAdapter.cs

@@ -7,7 +7,7 @@
 namespace Bloom.web
 {
     /// <summary>
-    /// Class that allows code expecting an SIL.Progress.IProgress object to use a WebSocketProgress instead.
+    /// Class that allows code expecting an SIL.Progress.IProgress object to use an IWebSocketProgress instead.
     /// </summary>
     public class WebProgressAdapter : IProgress
     {
@@ -32,9 +32,9 @@ public void IndicateUnknownProgress() { }
             public void Initialize() { }
         }
 
-        private readonly WebSocketProgress _webProgress;
+        private readonly IWebSocketProgress _webProgress;
 
-        public WebProgressAdapter(WebSocketProgress progress)
+        public WebProgressAdapter(IWebSocketProgress progress)
         {
             _webProgress = progress;
         }

src/BloomTests/Book/BookStorageTests.cs

@@ -1999,7 +1999,7 @@ public void PerformNecessaryMaintenanceOnBook_DeletesSVGIfOnlyNoneStyle()
             );
 
             //SUT
-            storage.MigrateToMediaLevel1ShrinkLargeImages(); // does nothing in this situation, but should still bump level
+            storage.MigrateToMediaLevel1ShrinkLargeImages(new NullProgress()); // does nothing in this situation, but should still bump level
             var mediaLevel = storage.Dom.GetMetaValue("mediaMaintenanceLevel", "0");
             Assert.That(mediaLevel, Is.EqualTo("1"));
             storage.MigrateToLevel2RemoveTransparentComicalSvgs();
@@ -2129,7 +2129,7 @@ public void PerformNecessaryMaintenanceOnBook_EnsuresImgAtStartOfImageContainer(
             );
 
             //SUT
-            storage.MigrateToMediaLevel1ShrinkLargeImages();
+            storage.MigrateToMediaLevel1ShrinkLargeImages(new NullProgress());
             storage.MigrateToLevel2RemoveTransparentComicalSvgs();
             storage.MigrateToLevel3PutImgFirst();
 
@@ -2167,7 +2167,7 @@ public void PerformNecessaryMaintenanceOnBook_DoesNotDeleteSVGIfOtherStylePresen
             );
 
             //SUT
-            storage.MigrateToMediaLevel1ShrinkLargeImages();
+            storage.MigrateToMediaLevel1ShrinkLargeImages(new NullProgress());
             storage.MigrateToLevel2RemoveTransparentComicalSvgs();
             storage.MigrateToLevel3PutImgFirst();
 
@@ -2333,7 +2333,7 @@ public void PerformNecessaryMaintenanceOnBook_HandlesMultipleSVGs()
             );
 
             //SUT
-            storage.MigrateToMediaLevel1ShrinkLargeImages();
+            storage.MigrateToMediaLevel1ShrinkLargeImages(new NullProgress());
             storage.MigrateToLevel2RemoveTransparentComicalSvgs();
             storage.MigrateToLevel3PutImgFirst();
 
@@ -2615,7 +2615,7 @@ public void PerformNecessaryMaintenanceOnBook_DoesNothingIfAlreadyProcessed()
             );
 
             //SUT
-            storage.MigrateToMediaLevel1ShrinkLargeImages();
+            storage.MigrateToMediaLevel1ShrinkLargeImages(new NullProgress());
             storage.MigrateToLevel2RemoveTransparentComicalSvgs();
             storage.MigrateToLevel3PutImgFirst();