Embed bold and italic variants of fonts needed in ebooks (BL-13811)

[StephenMcConnel]

This change is 

[JohnThomson]

Not sure everything I've commented on should be fixed. But worth a thought at least, I hope.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @StephenMcConnel)

---

src/BloomExe/FontProcessing/FontFileFinder.cs line 117 at r1 (raw file):

                }
                if (!string.IsNullOrEmpty(group.Normal))
                    yield return group.Normal;

It's possible that a font used only for headings, say, is also ONLY used bold/italic. Do we really want to include group.Italic, group.Bold, and group.Normal if we found a group.BoldItalic?
Then again, I can't find anywhere, past or present, where we care about more than the first result this function yields, so I'm not sure why we don't just have it return one result or null.

---

src/BloomExe/Publish/PublishHelper.cs line 215 at r1 (raw file):

            {
                return fontName.GetHashCode() ^ fontStyle.GetHashCode() ^ fontWeight.GetHashCode();
            }

I have a vague recollection that something should be done to make == and != work like Equals, though it may not matter for the limited purpose of this class.

---

src/BloomExe/Publish/PublishHelper.cs line 588 at r1 (raw file):

            var fonts = fontFamily.Split(new[] { ',' }, StringSplitOptions.RemoveEmptyEntries);
            // Fonts whose names contain spaces are quoted: remove the quotes.
            return fonts[0].Replace("\"", "");

Is it likely enough to care about that we don't have the first one in the list installed, and are using one of the others? Maybe that should be left for a later enhancement, even if it's possible.

---

src/BloomExe/Publish/PublishHelper.cs line 606 at r1 (raw file):

            Debug.WriteLine(
                $"DEBUG PublishHelper.StoreFontUsed(): font=\"{font}\", fontStyle={fontInfo.fontStyle}, fontWeight={fontInfo.fontWeight}"
            );

We might want to remove or comment out all the debug info before merging? I'm not sure. We don't do this so often in debug builds that it would be much nuisance.

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 852 at r1 (raw file):

                x => x.fontName == defaultFont && x.fontStyle == "normal" && x.fontWeight == "400"
            );
            fontsWanted.RemoveWhere(x => x.fontName == "Andika New Basic");

Something funny here. I don't remember whether we ship all four variants of Andika in our readers. But if we do, we can probably remove any font whose name is Andika. And if we don't, then we probably can't safely remove Andika New Basic Bold, say, unless we're going to embed Andika Bold instead. (And maybe we SHOULD start shipping all versions of Andika as part of our readers, if we aren't already?)

---

src/BloomExe/Publish/Epub/EpubMaker.cs line 2849 at r1 (raw file):

            FontGroup group,
            string relativePathFromCss = "",
            bool sanitizeFileName = false

We don't do anything with this argument, so why have it?

[StephenMcConnel]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @JohnThomson)

---

src/BloomExe/FontProcessing/FontFileFinder.cs line 117 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It's possible that a font used only for headings, say, is also ONLY used bold/italic. Do we really want to include group.Italic, group.Bold, and group.Normal if we found a group.BoldItalic?
Then again, I can't find anywhere, past or present, where we care about more than the first result this function yields, so I'm not sure why we don't just have it return one result or null.

I've changed the code to return only one file at most, but to use a fallback for closest match if the exact style and weight don't have a matching file. I also made this explicit in the method comments.

---

src/BloomExe/Publish/PublishHelper.cs line 215 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I have a vague recollection that something should be done to make == and != work like Equals, though it may not matter for the limited purpose of this class.

Done.

---

src/BloomExe/Publish/PublishHelper.cs line 588 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Is it likely enough to care about that we don't have the first one in the list installed, and are using one of the others? Maybe that should be left for a later enhancement, even if it's possible.

I added a comment about this possibility.

---

src/BloomExe/Publish/PublishHelper.cs line 606 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

We might want to remove or comment out all the debug info before merging? I'm not sure. We don't do this so often in debug builds that it would be much nuisance.

Done.

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 852 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Something funny here. I don't remember whether we ship all four variants of Andika in our readers. But if we do, we can probably remove any font whose name is Andika. And if we don't, then we probably can't safely remove Andika New Basic Bold, say, unless we're going to embed Andika Bold instead. (And maybe we SHOULD start shipping all versions of Andika as part of our readers, if we aren't already?)

I think we've only been shipping Andika Regular, trying for a minimal file size impact. I enhanced the check for Andika New Basic to include the style and weight.

---

src/BloomExe/Publish/Epub/EpubMaker.cs line 2849 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

We don't do anything with this argument, so why have it?

It should have been passed on the next method, which I've now done.

[JohnThomson]

Reviewed 4 of 4 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @StephenMcConnel)

---

src/BloomExe/FontProcessing/FontFileFinder.cs line 117 at r1 (raw file):

Previously, StephenMcConnel (Steve McConnel) wrote…

I've changed the code to return only one file at most, but to use a fallback for closest match if the exact style and weight don't have a matching file. I also made this explicit in the method comments.

I'm tempted to suggest changing it to GetFontFile() and just return one file or null, but (a) maybe not worth the effort, and (b) maybe when we do variable-weight we will be glad we can return more than one? Up to you; it's acceptable as-is.

---

src/BloomExe/Publish/PublishHelper.cs line 232 at r2 (raw file):

                if (ReferenceEquals(info1, null) || ReferenceEquals(info2, null))
                    return true;
                return !info1.Equals(info2);

I wouldn't bother mentioning this if there weren't other issues, but I prefer to reduce duplication by implementing != simply as "return ! info1 == info2". Not something I'd insist on if you prefer what you have.

---

src/BloomExe/Publish/PublishHelper.cs line 606 at r2 (raw file):

            // use just the first one.  We don't need to worry about the fallback fonts, just the primary one.  It
            // would be very unusual for the user not to have the primary font installed, but to have a fallback font
            // installed that.

This new comment feels incomplete

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 835 at r2 (raw file):

        /// find the files needed for those fonts.
        /// Copy the font file for the normal style of that font family from the system font folder,
        /// if permitted; or post a warning in progress if we can't embed it.

Sorry I missed this earlier, but this comment should be updated.

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 837 at r2 (raw file):

        /// if permitted; or post a warning in progress if we can't embed it.
        /// Create an extra css file (fonts.css) which tells the book to find the font files for those font families
        /// in the local folder, and insert a link to it into the book.

Is there anything we need to do to make fonts.css know about the additional font files we are copying?

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 852 at r2 (raw file):

                x => x.fontName == defaultFont && x.fontStyle == "normal" && x.fontWeight == "400"
            );
            var boldAndika =

would it work to just do
fontsWanted.RemoveWhere(x => x.fontName == "Andika New Basic" && fontsWanted.Any( y => y.Name == defaultFont && y.fontStyle == x.fontStyle && y.fontWeight == x.fontWeight);
?
(Would still need one special case to remove ANB regular unconditionally)

[StephenMcConnel]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @JohnThomson)

---

src/BloomExe/FontProcessing/FontFileFinder.cs line 117 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I'm tempted to suggest changing it to GetFontFile() and just return one file or null, but (a) maybe not worth the effort, and (b) maybe when we do variable-weight we will be glad we can return more than one? Up to you; it's acceptable as-is.

The whole point of variable weight fonts is that a single font file handles the different weights nicely.
I've changed the function to return just a single string, which might be null.

---

src/BloomExe/Publish/PublishHelper.cs line 232 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I wouldn't bother mentioning this if there weren't other issues, but I prefer to reduce duplication by implementing != simply as "return ! info1 == info2". Not something I'd insist on if you prefer what you have.

Done.

---

src/BloomExe/Publish/PublishHelper.cs line 606 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

This new comment feels incomplete

The last sentence did end abruptly and ungrammatically. I've finished off the sentence to make sense. :-)

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 835 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Sorry I missed this earlier, but this comment should be updated.

Done.

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 837 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Is there anything we need to do to make fonts.css know about the additional font files we are copying?

They're all listed in fontsWanted, and the code uses that list to generate fonts.css.

---

src/BloomExe/Publish/BloomPub/BloomPubMaker.cs line 852 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

would it work to just do
fontsWanted.RemoveWhere(x => x.fontName == "Andika New Basic" && fontsWanted.Any( y => y.Name == defaultFont && y.fontStyle == x.fontStyle && y.fontWeight == x.fontWeight);
?
(Would still need one special case to remove ANB regular unconditionally)

Done.

[JohnThomson]

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @StephenMcConnel)

---

src/BloomExe/FontProcessing/FontFileFinder.cs line 69 at r3 (raw file):

        /// <returns>enumeration of file paths (possibly none) that contain data for the specified font name, and which
        /// (as far as we can tell) we are allowed to embed.
        /// Note that the enumeration contains either one file or none.  If a file specific to the requested style

Sorry, the comment now needs updating.

[JohnThomson]

Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @StephenMcConnel)

[StephenMcConnel]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson)

---

src/BloomExe/FontProcessing/FontFileFinder.cs line 69 at r3 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Sorry, the comment now needs updating.

Done.

[JohnThomson]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @StephenMcConnel)