Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix BL-12890 Duplicate thumbnails #6232

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hatton
Copy link
Member

@hatton hatton commented Dec 1, 2023

This change is Reviewable

Copy link
Contributor

@JohnThomson JohnThomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I look at this more, I'm not satisfied.

  • My first objection was, in looking at AddBookInfo(), it felt wrong for it to trigger all the likely side effects of NotifyCollectionChanged(); if in fact we didn't do anything to the collection. So i was going to suggest we only call it if we really added something.
  • But then I realized: the way the bookInfo gets added to the collection earlier is through InsertBookInfo. And that NEVER calls NotifyCollectionChanged(). So the only way the notification gets out is in the later call to AddBookInfo.
  • So then I was going to suggest that we get rid of AddBookInfo and just use InsertBookInfo, or make AddBookInfo call Insert, and fix InsertBookInfo so that, at least if it really inserted a bookInfo rather than just replacing one, it calls NotifyCollectionChanged().
  • And something like that may be the right answer. But it means that the notification will go out at some possibly unexpected point during BBUD rather than after it is complete. Might be OK: I THINK the BookInfo gets saved and inserted as one of the last steps of Save, which is the last step of BBUD. But it feels fragile. And there may be other callers of the Insert method that really don't want notifications.
    So I think I would suggest:
  • Fix AddBookInfo like you have so it won't add a duplicate. It should also not send a notification if it does nothing.
  • Rename the current InsertBookInfo to InsertBookInfoWithoutNotifying. Let it return a boolean indicating whether the bookInfo was new.
  • Check callers of InsertBookInfo. Are there any where a notification SHOULD go out right away if we're adding a new book? If so, make a new InsertBookInfo that calls the renamed one and also does the notification if appropriate. Ones that really don't want that should call the renamed method.
  • Callers like CreateFromSourceBook, where we know a new one is going to get inserted, can unconditionally call NotifyCollectionChanged(); when all is ready for it.
  • Yet other callers may need to make their own use of the boolean returned by InsertBookInfoWithoutNotifying and, if it is true, call NotifyCollectionChanged() when appropriate.
    Now, that's a lot of work. And I think what you have will fix the bug. You probably don't have time for this suggestion. But for the sake of improving the code base, I think we should do it soon. I'm OK with merging as-is if we plan to do better soon.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @hatton)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants