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

Coverbrowser: event for cache refreshing #10956

Merged
merged 7 commits into from Oct 3, 2023

Conversation

hius07
Copy link
Member

@hius07 hius07 commented Sep 29, 2023

Self-explained.


This change is Reviewable

Comment on lines 170 to 168
if ui.coverbrowser then -- refresh cache db
ui.coverbrowser:deleteBookInfo(file)
end
UIManager:broadcastEvent(Event:new("DeleteBookInfo", file)) -- refresh coverbrowser cache db
UIManager:broadcastEvent(Event:new("BookMetadataChanged", self.prop_updated))
Copy link
Contributor

Choose a reason for hiding this comment

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

I seem to remember you told me deletebookinfo had to be done before bookmetadatachanged - which is what is happening here :) but couldn't it be done in the same BookMetadataChanged event ?

Also, no better name than DeleteBookInfo for this event ? DeleteBookInfo will be the specific action the coverbrowser plugin will do - but no other thing will act on this event. With a more generic name, like BookMetadataAboutToBeChanged (or something nicer :), coverbrowser would delete book info, but other code/pluging could do something else.

Copy link
Member Author

Choose a reason for hiding this comment

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

couldn't it be done in the same BookMetadataChanged event ?

I do not know how to arrange the required order of event handling by different modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right, that was the reason.
No idea either, and having 2 events is probably the right solution.
I just feel it needs a better name BookMetadataPreChanged, BookMetadataAboutToBeChanged ...

Copy link
Member Author

Choose a reason for hiding this comment

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

I cannot find other good idea for the name.
The event is specially for CoverBrowser, sent after metadata changed.

Copy link
Contributor

Choose a reason for hiding this comment

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

BookMetadataObsolete ? BookMetadataStalled ?
(meaning: the things you may have are now invalid - another event will come soon with what's been updated)

Copy link
Member

@NiLuJe NiLuJe Sep 30, 2023

Choose a reason for hiding this comment

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

If it's a cache we're talking about, the usual technical term to trash it because it no longer matches live data is "invalidate"; so InvalidateMetadataCache (I think document/book is fairly obviously implied there and can be elided ;)).

Copy link
Member Author

Choose a reason for hiding this comment

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

I like InvalidateMetadataCache but the general question is: should the event name show what happened or what should be done?

Copy link
Contributor

Choose a reason for hiding this comment

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

It should express what happened, when something specific happened, so handlers can do the different things that they think should be done after than specific thing happened.
In some cases when no specific thing happened and it's just a continuation of some handling, the event can be an action (ie. UpdateToc, UpdateFooter).
I like InvalidateMetadataCache too.

@hius07 hius07 merged commit 0ac258f into koreader:master Oct 3, 2023
3 checks passed
@hius07 hius07 deleted the coverbrowser-refresh-cache branch October 3, 2023 06:24
@hius07 hius07 added this to the 2023.10 milestone Oct 3, 2023
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.

None yet

3 participants