Ensure "Title Missing" shows for appropriate books (BL-16027)

[StephenMcConnel]

---

---

This change is 

[andrew-polk]

@andrew-polk reviewed 7 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on StephenMcConnel).

---

src/BloomExe/Collection/BookCollection.cs line 426 at r3 (raw file):

                        ? _bookSelection.CurrentSelection.BookInfo
                        : new BookInfo(folderPath, editable, sc);
                if (bookInfo.FileNameLocked)

Did you consider if you can/should try to use the existing logic for this?

I asked devin and it says:

---

Yes, there is very similar logic already in the codebase, and the new code in BookCollection.AddBookInfo (BookCollection.cs:426-463) is essentially reimplementing what already exists in two places:

1. Book.NameBestForUserDisplay (Book.cs:329-340)

This property already encapsulates the exact same two-branch logic the PR introduces for ThumbnailLabel:

public virtual string NameBestForUserDisplay{    get    {        if (BookInfo.FileNameLocked)            return Path.GetFileName(FolderPath);  // ← same as BookCollection.cs:429        return TitleBestForUserDisplay;            // ← same intent as BookCollection.cs:433    }}

The TitleBestForUserDisplay property (src/BloomExe/Book/Book.cs:305-315) in turn calls Book.GetBestTitleForDisplay(...) with the same arguments the PR manually constructs at src/BloomExe/Collection/BookCollection.cs:457-461.

2. BookInfo.GetBestTitleForUserDisplay (src/BloomExe/Book/BookInfo.cs:852-879)

This method on BookInfo already does a similar thing—it checks FileNameLocked, then parses AllTitles JSON and calls Book.GetBestTitleForDisplay. It doesn't need the full Book object or the HTML DOM. The PR's CollectionApi fallback at src/BloomExe/web/controllers/CollectionApi.cs:624-629 already uses this method (the wantRealTitle path).

What the PR adds that's different

The key new scenario the PR handles is when BookInfo.Title and AllTitles are both empty/null—meaning the metadata has no title at all. In that case, the existing BookInfo.GetBestTitleForUserDisplay would just return Title (which is empty), and NameBestForUserDisplay would need a full Book object. The PR's new code at src/BloomExe/Collection/BookCollection.cs:450-463 fills this gap by loading the HTML DOM as a last resort to extract a title via GetBestTitleForDisplay.

Could/should this be reused?

Yes, absolutely. The logic at BookCollection.cs:426-463 should be extracted into a method, ideally on BookInfo itself (e.g., BookInfo.ComputeThumbnailLabel(CollectionSettings, BookSelection)), since BookInfo already has GetBestTitleForUserDisplay. The same method could then be called from:

1. BookCollection.AddBookInfo (at collection load time)
2. CollectionTabView.UpdateForBookChanges (src/BloomExe/CollectionTab/CollectionTabView.cs:151)
3. The CollectionApi.HandleBooksRequest fallback (src/BloomExe/web/controllers/CollectionApi.cs:631-637)

This would eliminate the duplication of the FileNameLocked → folder name / else → Title / else → parse DOM pattern across three files. The expensive DOM-parsing fallback is genuinely new, but it could live in one place rather than being inlined in BookCollection.AddBookInfo.

Additionally, in CollectionTabView.UpdateForBookChanges at src/BloomExe/CollectionTab/CollectionTabView.cs:151, the code sets ThumbnailLabel = book.TitleBestForUserDisplay but doesn't handle the FileNameLocked case, which is handled in BookCollection.AddBookInfo. This inconsistency would be avoided if both sites called the same method.

---

src/BloomExe/CollectionTab/CollectionTabView.cs line 130 at r3 (raw file):

        private void UpdateForBookChanges(Book.Book book)
        {
            if (book.BookData.GetVariableOrNull("bookTitle", book.Language1Tag) == null)

Why do we need to save bookinfo here now? Is this fixing a separate, tangential bug?

[StephenMcConnel]

@StephenMcConnel made 2 comments and resolved 1 discussion.
Reviewable status: 4 of 8 files reviewed, 2 unresolved discussions (waiting on andrew-polk).

---

src/BloomExe/Collection/BookCollection.cs line 426 at r3 (raw file):

Previously, andrew-polk wrote…

Did you consider if you can/should try to use the existing logic for this?

I asked devin and it says:

---

Yes, there is very similar logic already in the codebase, and the new code in BookCollection.AddBookInfo (BookCollection.cs:426-463) is essentially reimplementing what already exists in two places:

1. Book.NameBestForUserDisplay (Book.cs:329-340)

This property already encapsulates the exact same two-branch logic the PR introduces for ThumbnailLabel:

public virtual string NameBestForUserDisplay{    get    {        if (BookInfo.FileNameLocked)            return Path.GetFileName(FolderPath);  // ← same as BookCollection.cs:429        return TitleBestForUserDisplay;            // ← same intent as BookCollection.cs:433    }}

The TitleBestForUserDisplay property (src/BloomExe/Book/Book.cs:305-315) in turn calls Book.GetBestTitleForDisplay(...) with the same arguments the PR manually constructs at src/BloomExe/Collection/BookCollection.cs:457-461.

2. BookInfo.GetBestTitleForUserDisplay (src/BloomExe/Book/BookInfo.cs:852-879)

This method on BookInfo already does a similar thing—it checks FileNameLocked, then parses AllTitles JSON and calls Book.GetBestTitleForDisplay. It doesn't need the full Book object or the HTML DOM. The PR's CollectionApi fallback at src/BloomExe/web/controllers/CollectionApi.cs:624-629 already uses this method (the wantRealTitle path).

What the PR adds that's different

The key new scenario the PR handles is when BookInfo.Title and AllTitles are both empty/null—meaning the metadata has no title at all. In that case, the existing BookInfo.GetBestTitleForUserDisplay would just return Title (which is empty), and NameBestForUserDisplay would need a full Book object. The PR's new code at src/BloomExe/Collection/BookCollection.cs:450-463 fills this gap by loading the HTML DOM as a last resort to extract a title via GetBestTitleForDisplay.

Could/should this be reused?

Yes, absolutely. The logic at BookCollection.cs:426-463 should be extracted into a method, ideally on BookInfo itself (e.g., BookInfo.ComputeThumbnailLabel(CollectionSettings, BookSelection)), since BookInfo already has GetBestTitleForUserDisplay. The same method could then be called from:

1. BookCollection.AddBookInfo (at collection load time)
2. CollectionTabView.UpdateForBookChanges (src/BloomExe/CollectionTab/CollectionTabView.cs:151)
3. The CollectionApi.HandleBooksRequest fallback (src/BloomExe/web/controllers/CollectionApi.cs:631-637)

This would eliminate the duplication of the FileNameLocked → folder name / else → Title / else → parse DOM pattern across three files. The expensive DOM-parsing fallback is genuinely new, but it could live in one place rather than being inlined in BookCollection.AddBookInfo.

Additionally, in CollectionTabView.UpdateForBookChanges at src/BloomExe/CollectionTab/CollectionTabView.cs:151, the code sets ThumbnailLabel = book.TitleBestForUserDisplay but doesn't handle the FileNameLocked case, which is handled in BookCollection.AddBookInfo. This inconsistency would be avoided if both sites called the same method.

Done.

---

src/BloomExe/CollectionTab/CollectionTabView.cs line 130 at r3 (raw file):

Previously, andrew-polk wrote…

Why do we need to save bookinfo here now? Is this fixing a separate, tangential bug?

If we don't save the updates made for an empty title here, then the old title can stay around in both the title and the allTitles. fields.