diff --git a/src/BloomExe/Book/BookData.cs b/src/BloomExe/Book/BookData.cs index 2eecbb4c7061..02e4f15ed6c1 100644 --- a/src/BloomExe/Book/BookData.cs +++ b/src/BloomExe/Book/BookData.cs @@ -2544,15 +2544,23 @@ internal bool UpdateImageFromDataSet(DataSet data, SafeXmlElement node, string k /// from the image (so, for the transparency classes, Auto is restored too). Without this, /// the user's choice would be lost every time the xmatter is regenerated from the template. /// See BL-16819. + /// A null savedAttributes means the data set carries no attribute information for this + /// image at all, not that the image has no classes, so the image is left alone (as + /// MergeAttrsIntoElement does). This is the normal state of the member _dataset once a page + /// has been saved: UpdateSingleTextVariableInDataDiv recreates its entry with the new value + /// but without the attribute list, and UpdateDomFromDataset() pushes that data set to the + /// pages (e.g. from Book.SetMultilingualContentLanguages every time the Edit tab is entered). /// private static void RestoreImgClassesFromDataDiv( SafeXmlElement img, List> savedAttributes ) { + if (savedAttributes == null) + return; var savedClasses = savedAttributes - ?.Find(a => a.Item1 == "class") + .Find(a => a.Item1 == "class") ?.Item2.Unencoded.Split(new[] { ' ' }, StringSplitOptions.RemoveEmptyEntries) ?? new string[0]; foreach (var className in _imgClassesToRestoreFromDataDiv) diff --git a/src/BloomExe/BookThumbNailer.cs b/src/BloomExe/BookThumbNailer.cs index c3306edd7401..683d38cf3fe2 100644 --- a/src/BloomExe/BookThumbNailer.cs +++ b/src/BloomExe/BookThumbNailer.cs @@ -14,6 +14,7 @@ using Bloom.Publish; using Bloom.SafeXml; using SIL.IO; +using SIL.Reporting; using SIL.Windows.Forms.ImageToolbox; namespace Bloom @@ -261,20 +262,56 @@ internal static bool CreateThumbnailOfCoverImage( var destFilePath = Path.Combine(book.StoragePageFolder, options.FileName); // Writing a transparent image to a file, then reading it in again appears to be the only // way to get the thumbnail image to draw with the book's cover color background reliably. + // It is always a .png, whatever the cover image is: only a PNG can carry the alpha + // channel, and ImageUtils.MakeTransparentBackground needs a .png destination. The name + // is random rather than the cover image's own, so thumbnails being made at the same time + // (two books whose covers are both "cover.jpg", say) cannot overwrite or delete each + // other's file. transparentImageFile = Path.Combine( Path.GetTempPath(), "Bloom", "Transparent", - Path.GetFileName(imageSrc) + Path.GetRandomFileName() + ".png" ); Directory.CreateDirectory(Path.GetDirectoryName(transparentImageFile)); - if ( - RuntimeImageProcessor.MakePngBackgroundTransparentIfDesirable( - imageSrc, - transparentImageFile - ) - ) + // Honor the user's Transparency choice for the cover image (the bloom-opaque and + // bloom-transparent classes; Auto is neither) the same way the browser does when it + // displays the cover: Opaque never gets a transparent background, Transparent always + // does, and Auto gets one if the image looks like line art. The thumbnail is always + // drawn on the cover color, so the page counts as one that needs transparent images. + // See BL-16819. + var transparencyMode = + coverImgElt == null + ? ImageTransparencyMode.Auto + : HtmlDom.GetImageTransparencyMode(coverImgElt, pageNeedsTransparent: true); + // A problem image should still get a thumbnail, just without the transparency. + var madeTransparent = false; + try + { + switch (transparencyMode) + { + case ImageTransparencyMode.Force: + madeTransparent = ImageUtils.MakeTransparentBackground( + imageSrc, + transparentImageFile + ); + break; + case ImageTransparencyMode.Auto: + madeTransparent = ImageUtils.MakeTransparentBackgroundIfNeeded( + imageSrc, + transparentImageFile + ); + break; + } + } + catch (Exception e) + { + Logger.WriteEvent( + "Could not make the cover image transparent for the thumbnail: " + e.Message + ); + } + if (madeTransparent) imageSrc = transparentImageFile; using (var coverImage = PalasoImage.FromFileRobustly(imageSrc)) { diff --git a/src/BloomExe/ImageProcessing/ImageUtils.cs b/src/BloomExe/ImageProcessing/ImageUtils.cs index 9bb72c98f264..45dfb3578f81 100644 --- a/src/BloomExe/ImageProcessing/ImageUtils.cs +++ b/src/BloomExe/ImageProcessing/ImageUtils.cs @@ -3641,6 +3641,12 @@ SafeXmlElement imgElement } } + /// + /// Apply the transparency algorithm to a copy of the image at destinationPath (which must be + /// a .png path) if the image looks like line art on a white background; otherwise do nothing + /// and return false. Handles a JPEG source the same way the display pipeline does: the + /// line-art check accepts JPEGs, and the copy is re-saved as PNG so it can carry the alpha. + /// internal static bool MakeTransparentBackgroundIfNeeded( string sourcePath, string destinationPath @@ -3648,24 +3654,33 @@ string destinationPath { using (var imageInfo = PalasoImage.FromFileRobustly(sourcePath)) { - if (ShouldMakeBackgroundTransparent(imageInfo)) - { - RobustFile.Copy(sourcePath, destinationPath, true); - ApplyBloomTransparencyToFile(destinationPath); - return true; - } + if (!ShouldMakeBackgroundTransparent(imageInfo)) + return false; } - return false; + return MakeTransparentBackground(sourcePath, destinationPath); } /// /// Like but always applies the /// transparency algorithm, bypassing the line-art detection check. /// Used when an image has the bloom-transparent class (transparent=force). + /// The destination must be a .png path: only a PNG can carry the alpha channel, and + /// ApplyBloomTransparencyToFile does nothing to any other extension. A source that is not + /// itself a PNG (a JPEG cover, say) is re-saved as PNG there rather than copied. /// internal static bool MakeTransparentBackground(string sourcePath, string destinationPath) { - RobustFile.Copy(sourcePath, destinationPath, true); + if (sourcePath.EndsWith(".png", StringComparison.InvariantCultureIgnoreCase)) + { + RobustFile.Copy(sourcePath, destinationPath, true); + } + else + { + using (var imageInfo = PalasoImage.FromFileRobustly(sourcePath)) + { + RobustImageIO.SaveImage(imageInfo.Image, destinationPath, ImageFormat.Png); + } + } ApplyBloomTransparencyToFile(destinationPath); return true; } diff --git a/src/BloomExe/ImageProcessing/RuntimeImageProcessor.cs b/src/BloomExe/ImageProcessing/RuntimeImageProcessor.cs index 1d5e794ebeb9..35a29224253a 100644 --- a/src/BloomExe/ImageProcessing/RuntimeImageProcessor.cs +++ b/src/BloomExe/ImageProcessing/RuntimeImageProcessor.cs @@ -644,102 +644,5 @@ private static ColorPalette GivePaletteTransparentBackground(Image bitmap) } return palette; // assigning this back to the bitmap will actually update it. } - - /// - /// Make the image background transparent if the image is PNG and appears to be a black - /// and white drawing. - /// - /// true if an image with transparent background is created - public static bool MakePngBackgroundTransparentIfDesirable( - string originalPath, - string pathToProcessedImage - ) - { - try - { - //if it's a jpeg, we don't resize, we don't mess with transparency, nothing. These things - //are scary in .net. Just send the original back and wash our hands of it. - //If the filename extension claims to be jpeg, assume it's not lying to us and quit. - if (ImageUtils.HasJpegExtension(originalPath)) - return false; - - using (var originalImage = PalasoImage.FromFileRobustly(originalPath)) - { - // double check whether the file extension was misleading us... - if (ImageUtils.AppearsToBeJpeg(originalImage)) - return false; - - // Check whether a transparent background is actually needed. - if (!ImageUtils.ShouldMakeBackgroundTransparent(originalImage)) - return false; - - using (var processedBitmap = MakePngBackgroundTransparent(originalImage)) - { - //Hatton July 2012: - //Once or twice I saw a GDI+ error on the Save below, when the app 1st launched. - //I verified that if there is an IO error, that's what you get (a GDI+ error). - //I looked once, and the %temp%/Bloom directory wasn't there, so that's what I think caused the error. - //It's not clear why the temp/bloom directory isn't there... possibly it was there a moment ago - //but then some startup thread cleared and deleted it? (we are now running on a thread responding to the http request) - - Exception error = null; - for (var i = 0; i < 3; i++) //try three times - { - try - { - error = null; - RobustImageIO.SaveImage( - processedBitmap, - pathToProcessedImage, - originalImage.Image.RawFormat - ); - break; - } - catch (Exception e) - { - Logger.WriteEvent( - "***Error in RuntimeImageProcessor while trying to write image." - ); - Logger.WriteEvent(e.Message); - error = e; - //in setting the sleep time, keep in mind that this may be one of 20 images - //so if the problem happens to all of them, then you're looking 20*retries*sleep-time, - //which will look like hung program. - //Meanwhile, this transparency thing is actually just a nice-to-have. If we give - //up, it's ok. - Thread.Sleep(100); //wait a 1/5 second before trying again - } - } - - if (error != null) - { - throw error; //will be caught below - } - } - } - - return true; - } - // We want to gracefully degrade if this fails (as it did once, see comment in BL-2871) - // We can't help users who respond to a toast and send in an error report. - // Logging it will allow us to possibly correlate an error here with another problem that does get reported. - catch (TagLib.CorruptFileException e) - { - // Don't bother reporting this to the user, but log it in case it might be relevant for a real bug. - Logger.WriteError( - $"Problem with image metadata: MakePngBackgroundTransparentIfDesirable({originalPath}) caught an exception.", - e - ); - return false; - } - catch (Exception e) - { - Logger.WriteError( - $"Problem making image transparent: MakePngBackgroundTransparentIfDesirable({originalPath}) caught an exception.", - e - ); - return false; - } - } } } diff --git a/src/BloomTests/Book/BookDataTests.cs b/src/BloomTests/Book/BookDataTests.cs index 75b237d6c0c3..9c2f98bc22ad 100644 --- a/src/BloomTests/Book/BookDataTests.cs +++ b/src/BloomTests/Book/BookDataTests.cs @@ -3407,6 +3407,100 @@ SafeXmlElement ReopenBookAndGetCoverImage() } } + /// + /// BL-16819: UpdateDomFromDataset() pushes the member data set, gathered when the BookData + /// was constructed, back to the pages. That must not disturb the cover image's Transparency + /// choice when the data set agrees with the page. + /// + [Test] + public void UpdateDomFromDataset_CoverImageOpaque_KeepsClass() + { + var dom = new HtmlDom( + @" +
+
aor.png
+
+
+
+ +
+
+ " + ); + var data = new BookData(dom, _collectionSettings, null); + data.UpdateDomFromDataset(); + var pageImage = (SafeXmlElement) + dom.SelectSingleNodeHonoringDefaultNS( + "//div[@class='bloom-page']//img[@data-book='coverImage']" + ); + Assert.That( + pageImage.HasClass("bloom-opaque"), + Is.True, + "class attribute is '" + pageImage.GetAttribute("class") + "'" + ); + } + + /// + /// BL-16819: after a page save, the member data set's entry for the cover image has been + /// recreated with the new value but without any attribute list. UpdateDomFromDataset() (which + /// Book.SetMultilingualContentLanguages runs every time the Edit tab is entered) must then + /// leave the image's Transparency choice alone rather than treating the missing attribute + /// list as "no classes" and stripping the choice the user just saved. + /// + [Test] + public void UpdateDomFromDataset_AfterSavingOpaqueChoice_KeepsClass() + { + var dom = new HtmlDom( + @" +
+
aor.png
+
+
+
+ +
+
+ " + ); + var data = new BookData(dom, _collectionSettings, null); + var editedPageDom = new HtmlDom( + @" +
+
+ +
+
+ " + ); + data.SuckInDataFromEditedDom(editedPageDom); + var pageImage = (SafeXmlElement) + dom.SelectSingleNodeHonoringDefaultNS( + "//div[@class='bloom-page']//img[@data-book='coverImage']" + ); + Assert.That( + pageImage.HasClass("bloom-opaque"), + Is.True, + "sanity: save should put the class on the page image" + ); + data.UpdateDomFromDataset(); + pageImage = (SafeXmlElement) + dom.SelectSingleNodeHonoringDefaultNS( + "//div[@class='bloom-page']//img[@data-book='coverImage']" + ); + Assert.That( + pageImage.GetAttribute("src"), + Is.EqualTo("new.png"), + "the saved image url should be kept" + ); + Assert.That( + pageImage.HasClass("bloom-opaque"), + Is.True, + "the saved Opaque choice should be kept; class attribute is '" + + pageImage.GetAttribute("class") + + "'" + ); + } + /// /// BL-16819: the data-div is authoritative, so if it says the cover image is on Auto (no /// transparency class), a stale override on the page image must be removed. And copying the diff --git a/src/BloomTests/Book/BookTests.cs b/src/BloomTests/Book/BookTests.cs index 91625e4bc68b..155f3569d02b 100644 --- a/src/BloomTests/Book/BookTests.cs +++ b/src/BloomTests/Book/BookTests.cs @@ -296,6 +296,73 @@ public void BringBookUpToDate_EmbeddedEmptyImgTagRemoved() Assert.IsTrue(pageImage.GetAttribute("src").Equals(placeHolderFile)); } + /// + /// BL-16819: the user's Transparency choice for the cover image (here Opaque) is a class on the + /// img and, once the page is saved, on the data-div copy. Bringing the book up to date replaces + /// the xmatter with a fresh template page and refills the cover image from the data-div, and the + /// choice must survive that. + /// + [Test] + public void BringBookUpToDate_CoverImageTransparencyChoiceSurvives() + { + SetDom( + @"
+
aor.png
+
+
+
+
+
+
+ +
+
+
+
+
" + ); + var book = CreateBook(); + var dom = book.RawDom; + var pageImageXpath = + "//div[contains(@class,'bloom-page')]//img[@data-book='coverImage']"; + Assert.That( + ((SafeXmlElement)dom.SelectSingleNodeHonoringDefaultNS(pageImageXpath)).HasClass( + "bloom-opaque" + ), + Is.True, + "sanity check: the cover image starts out Opaque" + ); + + book.BringBookUpToDate(new NullProgress()); + + var dataDivImage = (SafeXmlElement) + dom.SelectSingleNodeHonoringDefaultNS( + "//div[@id='bloomDataDiv']/div[@data-book='coverImage']" + ); + Assert.That( + dataDivImage.HasClass("bloom-opaque"), + Is.True, + "the data-div copy should keep the Opaque choice" + ); + var pageImage = (SafeXmlElement)dom.SelectSingleNodeHonoringDefaultNS(pageImageXpath); + Assert.That(pageImage.GetAttribute("src"), Is.EqualTo("aor.png")); + Assert.That( + pageImage.HasClass("bloom-opaque"), + Is.True, + "the cover image should still be Opaque after the xmatter is regenerated" + ); + + // And the page as prepared for the Edit tab must show the choice too. + var coverPage = book.GetPages().First(p => p.IsXMatter); + var editDom = book.GetEditableHtmlDomForPage(coverPage); + var editImg = (SafeXmlElement)editDom.SelectSingleNodeHonoringDefaultNS(pageImageXpath); + Assert.That( + editImg.HasClass("bloom-opaque"), + Is.True, + "the page prepared for editing should keep the Opaque choice" + ); + } + // Unless it's part of a bloom-canvas that has an image description, an image // should have an alt attr that is exactly an empty string. [Test] diff --git a/src/BloomTests/BookThumbnailerTests.cs b/src/BloomTests/BookThumbnailerTests.cs index 4790626fb82b..e9c2f06e5723 100644 --- a/src/BloomTests/BookThumbnailerTests.cs +++ b/src/BloomTests/BookThumbnailerTests.cs @@ -47,6 +47,101 @@ int coverImageHeight return book; } + /// + /// BL-16819: the book thumbnail is composed directly from the cover image file, and it must + /// honor the user's Transparency choice for that image just as the browser does: Auto makes + /// the background of line art transparent (so the cover color shows behind the drawing), + /// Transparent always does, and Opaque never does. + /// + [TestCase("", true, "png")] + [TestCase("bloom-transparent", true, "png")] + [TestCase("bloom-opaque", false, "png")] + // A JPEG cannot carry transparency itself, so Auto and Transparent go through a PNG copy. + [TestCase("", true, "jpg")] + [TestCase("bloom-transparent", true, "jpg")] + [TestCase("bloom-opaque", false, "jpg")] + public void CreateThumbnailOfCoverImage_HonorsTransparencyChoice( + string transparencyClass, + bool expectCoverColorBehindImage, + string imageExtension + ) + { + var coverImageFilename = "lineArt." + imageExtension; + SetDom( + @"
+
" + + coverImageFilename + + @"
+
+
+
+
+
+
+
" + ); + var book = CreateBook(); + // Black lines on a white background: the line-art detection treats this as line art. + using (var bitmap = new Bitmap(100, 100)) + { + using (var gfx = Graphics.FromImage(bitmap)) + using (var pen = new Pen(Color.Black, 6)) + { + gfx.Clear(Color.White); + gfx.DrawEllipse(pen, 25, 25, 50, 50); + gfx.DrawLine(pen, 20, 80, 80, 20); + } + RobustImageIO.SaveImage( + bitmap, + book.FolderPath.CombineForPath(coverImageFilename), + imageExtension == "png" + ? System.Drawing.Imaging.ImageFormat.Png + : System.Drawing.Imaging.ImageFormat.Jpeg + ); + } + book.BringBookUpToDate(new NullProgress()); + book.SetCoverColor("#FF0000"); + var coverImg = (Bloom.SafeXml.SafeXmlElement) + book.RawDom.SelectSingleNodeHonoringDefaultNS( + "//div[contains(@class,'bloom-page')]//img[@data-book='coverImage']" + ); + Assert.That( + coverImg, + Is.Not.Null, + "sanity check: the cover image should be on the page" + ); + if (transparencyClass != "") + coverImg.AddClass(transparencyClass); + + var options = BookThumbNailer.GetCoverThumbnailOptions(-1, Guid.Empty); + var made = BookThumbNailer.CreateThumbnailOfCoverImage(book, options); + Assert.That(made, Is.True, "sanity check: the thumbnail should be created"); + + var thumbnailPath = book.FolderPath.CombineForPath(options.FileName); + Assert.That(RobustFile.Exists(thumbnailPath), Is.True, "sanity check: thumbnail file"); + using (var thumbnail = new Bitmap(thumbnailPath)) + { + // A pixel of the white background, well away from the drawing and from the edges + // that resizing may blur. + var background = thumbnail.GetPixel(3, 3); + Assert.That(background.A, Is.EqualTo(255), "the thumbnail itself should be opaque"); + var isCoverColor = background.R > 200 && background.G < 60 && background.B < 60; + var isWhite = background.R > 200 && background.G > 200 && background.B > 200; + Assert.That( + isCoverColor || isWhite, + Is.True, + $"expected the cover color or white behind the drawing but got {background}" + ); + Assert.That( + isCoverColor, + Is.EqualTo(expectCoverColorBehindImage), + expectCoverColorBehindImage + ? $"with '{transparencyClass}' the image background should be transparent, showing the cover color; got {background}" + : $"with '{transparencyClass}' the image background should stay white; got {background}" + ); + } + } + // Bigger images [TestCase(70, 1024, 768, 70, 52)] [TestCase(256, 1024, 768, 256, 192)]