diff --git a/DistFiles/localization/en/BloomLowPriority.xlf b/DistFiles/localization/en/BloomLowPriority.xlf index d8af6f488ded..0c24fdd95c19 100644 --- a/DistFiles/localization/en/BloomLowPriority.xlf +++ b/DistFiles/localization/en/BloomLowPriority.xlf @@ -383,6 +383,11 @@ ID: EditTab.CustomCover.FieldType.CoverCredits One choice in the "Field Type:" menu of a text block on a custom layout page. Choosing it makes that block hold the credits (author, illustrator, etc.) that are shown on the cover. + + Bloom could not make this book's pictures smaller. It will try again the next time the book is updated. + ID: ImageUtils.ShrinkingImagesFailed + Shown as a toast when the one-time shrinking of an old book's oversized pictures fails. The book still works; the pictures are just left large, and Bloom retries later. "Bloom" is a product name and must not be translated. + diff --git a/src/BloomExe/Book/Book.cs b/src/BloomExe/Book/Book.cs index 1afc54b13833..4fd37391f2fb 100644 --- a/src/BloomExe/Book/Book.cs +++ b/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(); diff --git a/src/BloomExe/Book/BookProcessor.cs b/src/BloomExe/Book/BookProcessor.cs index 7afb473385a4..cc4bb01ab822 100644 --- a/src/BloomExe/Book/BookProcessor.cs +++ b/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"); diff --git a/src/BloomExe/Book/BookStorage.cs b/src/BloomExe/Book/BookStorage.cs index af8867ca5036..9702fa27f8ca 100644 --- a/src/BloomExe/Book/BookStorage.cs +++ b/src/BloomExe/Book/BookStorage.cs @@ -107,7 +107,7 @@ void CleanupUnusedSupportFiles( void CaptureInitialStateForMigration(); void RestoreStuffBeforeMigration(); void MigrateMaintenanceLevels(); - void MigrateToMediaLevel1ShrinkLargeImages(); + void MigrateToMediaLevel1ShrinkLargeImages(IProgress progress = null); void MigrateToLevel2RemoveTransparentComicalSvgs(); void MigrateToLevel3PutImgFirst(); @@ -4040,6 +4040,16 @@ public void MigrateMaintenanceLevels() Dom.UpdateMetaElement("mediaMaintenanceLevel", GetMaintenanceLevel() >= 1 ? "1" : "0"); } + /// + /// Set when an attempt to shrink this book's images failed, so that we do not repeat the whole + /// slow attempt a moment later in the same pass: Book.EnsureUpToDate calls the migration twice, + /// once by way of EnsureUpToDateMemory and once directly afterwards, and the second call used to + /// be a no-op only because the first had already bumped mediaMaintenanceLevel. The level itself + /// deliberately stays at 0, so the shrink is still retried the next time this book is loaded and + /// brought up to date -- we just do not do it twice over, and fail twice, in one pass. + /// + private bool _mediaLevel1ShrinkFailed; + /// /// In very old books (before 4.9) we did not shrink even very large images before adding them to /// books. When we encounter such a book, we go ahead and shrink them. This is probably less @@ -4047,21 +4057,37 @@ 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. /// - public void MigrateToMediaLevel1ShrinkLargeImages() + /// Where to report the (potentially very slow) shrinking, so the user + /// can see why we are busy. It is used whenever it is somewhere real to report, and also + /// whenever we could not put up a dialog even if we wanted to: off the UI thread, headless, + /// or under test. Only when we are on the UI thread AND all the caller gave us is a + /// NullProgress (or nothing) do we put up our own dialog instead and leave this unused -- + /// see the branch below. Passing nothing is equivalent to passing a NullProgress; in + /// practice only tests do, since Book.EnsureUpToDate substitutes one for a null. + public void MigrateToMediaLevel1ShrinkLargeImages(IProgress progress = null) { var levelString = Dom.GetMetaValue("mediaMaintenanceLevel", "0"); if (!int.TryParse(levelString, out int level)) level = 0; - if (level >= 1) + if (level >= 1 || _mediaLevel1ShrinkFailed) return; + var success = true; if (ImageUtils.NeedToShrinkImages(FolderPath)) { // 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,33 +4099,136 @@ 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) + // A NullProgress reports nowhere, so having one is the same as having none: it is + // what a caller passes when it has no way to show the user anything. Anything else + // is somewhere real to report, and we should use it rather than opening a window + // over the top of whatever the caller is already showing. + // + // Note the invariant this puts on such a caller: taking the caller's progress also + // means doing the shrinking synchronously on the caller's thread, so a caller that + // is on the UI thread needs a progress that pumps messages, or Bloom will be frozen + // for the whole (potentially minutes-long) shrink. Today the only UI-thread caller + // with a real progress is CollectionModel.BringBookUpToDate ("Update Book"), which + // is safe on both counts: ProgressDialogForeground runs all of BringBookUpToDate on + // the UI thread anyway, and its MultiProgress includes an ApplicationDoEventsProgress + // that pumps on every message. A future UI-thread caller passing a progress that does + // not pump would need the dialog branch below instead. + var haveSomewhereToReport = progress != null && !(progress is NullProgress); + 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. NullProgress + // is used if the caller did not pass one. + if ( + Program.RunningUnitTests + || haveSomewhereToReport + || shell == null + || shell.InvokeRequired + ) { - // TeamCity enforces not showing modal dialogs during unit tests on Windows 10. - ImageUtils.FixSizeAndTransparencyOfImagesInFolder( - FolderPath, - new List(), - new NullProgress() - ); + if (progress == null) + progress = new NullProgress(); + try + { + ImageUtils.FixSizeAndTransparencyOfImagesInFolder( + FolderPath, + new List(), + progress + ); + } + catch (Exception e) + { + ReportShrinkFailure(e, progress); + success = false; + } } else { - using (var dlg = new ProgressDialogBackground()) + // InvokeRequired was false, so we are on the shell's own thread and may create + // the dialog right here; no marshalling needed. + success = ShrinkImagesBehindProgressDialog(); + } + } + if (success) + Dom.UpdateMetaElement("mediaMaintenanceLevel", "1"); + else + _mediaLevel1ShrinkFailed = true; + } + + /// + /// 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. + /// + /// + /// 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, log it, and return false here, which both records the failure in + /// the log and leaves mediaMaintenanceLevel alone, so the shrink is attempted again next time. + /// + /// True if the images were successfully shrunk; otherwise, false. + private bool ShrinkImagesBehindProgressDialog() + { + Exception errorInWorker = null; + using (var dlg = new ProgressDialogBackground()) + { + dlg.Text = "Updating Image Files"; + dlg.ShowAndDoWork( + (dialogProgress, args) => { - dlg.Text = "Updating Image Files"; - dlg.ShowAndDoWork( - (progress, args) => - ImageUtils.FixSizeAndTransparencyOfImagesInFolder( - FolderPath, - new List(), - progress - ) - ); + try + { + ImageUtils.FixSizeAndTransparencyOfImagesInFolder( + FolderPath, + new List(), + dialogProgress + ); + } + catch (Exception e) + { + errorInWorker = e; + } } - } + ); + } + if (errorInWorker != null) + { + // No progress to report to: we only take this branch when the caller had none. + ReportShrinkFailure(errorInWorker); + return false; } + return true; + } - Dom.UpdateMetaElement("mediaMaintenanceLevel", "1"); + /// + /// Report a failed image shrink, in every place that has somewhere to report it. + /// + /// + /// The toast is passive on purpose: the book still works, its pictures are merely left large, + /// and we will try again next time, so this is not worth interrupting the user for. Note that + /// writing to the caller's progress must not use WriteError: ProgressDialogForeground shows a + /// modal "There was a problem performing that operation" when its progress records an error, + /// which would defeat the point of reporting this passively. + /// + /// The failure to report. Logged whole, so the stack trace and any inner + /// exception survive; the message alone often does not even name the offending file. + /// The caller's progress, if it had one. Without this, an operation + /// that is already showing the user a progress box would appear to have finished cleanly. + private void ReportShrinkFailure(Exception error, IProgress progress = null) + { + var message = LocalizationManager.GetString( + "ImageUtils.ShrinkingImagesFailed", + "Bloom could not make this book's pictures smaller. It will try again the next time the book is updated." + ); + progress?.WriteWarning(message); + NonFatalProblem.Report( + ModalIf.None, + PassiveIf.All, + message, + "Shrinking images failed in " + FolderPath, + exception: error + ); } private int GetMaintenanceLevel() diff --git a/src/BloomExe/CollectionTab/CollectionModel.BloomSourceImport.cs b/src/BloomExe/CollectionTab/CollectionModel.BloomSourceImport.cs index 213350446dc2..6b0448928636 100644 --- a/src/BloomExe/CollectionTab/CollectionModel.BloomSourceImport.cs +++ b/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. /// - 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(); diff --git a/src/BloomExe/web/WebProgressAdapter.cs b/src/BloomExe/web/WebProgressAdapter.cs index 9ac3212f1fea..0cf3ec27eb02 100644 --- a/src/BloomExe/web/WebProgressAdapter.cs +++ b/src/BloomExe/web/WebProgressAdapter.cs @@ -7,7 +7,7 @@ namespace Bloom.web { /// - /// 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. /// 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; }