CoverBrowser info_cache: move to core#13018
Conversation
| return item_t | ||
| end | ||
|
|
||
| -- BookInfoCache, for menues showing the file list |
There was a problem hiding this comment.
(menues => Menu instances)
Surprised to see the generic Menu holding self.book_info_cache. Our generic Menu shouldn't have any trace of higher level things like DocSettings or books.
Shouldn't/couldn't it be in the FileChooser (subclass of Menu dedicated to files which are most often books) ?
There was a problem hiding this comment.
Where to place it for History/Collections (direct subclasses of Menu)?
There was a problem hiding this comment.
I don't know, don't these all have a FileChooser around ? These methods could be defined in FileChoser, and plugged into the upper Menu (if the Menu instance is not the FileChooser instance, but still accessible from the FileChooser instance).
Other ideas:
Create a new bookawaremenu.lua with BookAwareMenu = Menu:extend{} and your additional methods (might find a better shorter name), and have History and Collections be a specialized BookAwareMenu instance instead of a generic Menu one.
Or have some function elsewhere that dress/empower a Menu instance that need them with these additional methods.
(I understand why you need these methods, I'm just trying to find a cleaner solution - from a architectural/object oriented point of view, so we don't travestize a generic Menu into something that it shouldn't be just to solve some problem at the time.)
|
New BookList widget. |
|
I have an idea to put the cache into the BookList base class, the common cache for all FileChooser, Hist, Coll instances. |
|
While you are thinking about it, give it some thoughts if something like our flat/"browse by metadata" library experiment bits could find their home in this new class. |
|
Collections already do that, maybe it's enough? |
|
It's a poor man's "browse by metadata", the user have to manually create collections, they are not updated when new books are added... |
|
But it is a real flat library, not folder-tree. |
|
Can we approve the names before next massive renaming please. (1) (2) (3) (4) (5) |
|
Sounds good to me at a glance. |
I don't see how it's duplicated (?). Can't they even abstract more things ? ie. BookList.setBookInfoCacheBeenOpened with false => BookList.setBookBeenOpened would removed thedocsetting ?
Why note BookList.getDocSettings(file) ? The point is that these higher level don't need to inherit the technical names of the lower level ones. Fine with the others. |
May be i'm seeing too wide :) After all, BookList is just a subclass of Menu, a visual widget. It's not named BookManager. |
|
|
||
| -- BookInfo | ||
|
|
||
| function BookList.setBookInfoCache(file, doc_settings) |
There was a problem hiding this comment.
Everything (in the other changed modules) reads rather well.
This one is ok, but just mentionning some minor thought, in case you prefer these options:
setBookInfoCache(doc_settings) could read (a quick read) that we set the doc_settings as the cached thing.
setBookInfoCacheFrom(doc_settings) would make it clearer that we don't, but it's long and ugly.
updateBookInfoCache(doc_settings) would avoid the "set" feeling, and "update" is sufficiently opaque that we know we update the cache from the doc_settings.
| function BookList.setBookInfoCacheProperty(file, prop_name, prop_value) | ||
| if prop_name == "been_opened" and prop_value == false then |
There was a problem hiding this comment.
Also ok, but :):
Elsewhere, we use: BookList.setBookInfoCacheProperty(file, "status", to_status).
Not fond of passing property names as strings ("status"), just from a stylistic PoV (it's prone to typo, but so it is if we pass it as a table key name).
The alternative I used in a few places is to have BookList.PROP_STATUS = "status" (or BookList.PROP_STATUS = 1) and just pass BookList.PROP_STATUS to avoid typos (we'll get a nil if a typo). But it's noisy.
We could also just pass a table BookList.updateBookInfoCache( { status = "reading" } ), but you'd need to iterate as it advertizes we could pass multiple properties. (And the name conflicts with my previous comment suggestion :)).
Anyway, just some thoughts in case you prefer one of these if you were not satisfied with the one you decided in the heat of the moment :)
…me recent changes in commit 1750499, CoverBrowser info_cache: move to core (koreader#13018). Update them manually
… changes in commit 1750499, CoverBrowser info_cache: move to core (koreader#13018). Restore the modifications made for them
…ce the book is being read and we change the status to tbr, even though the tbr mark appears, the progress bar remains. This probably was introduced in commit 1750499, CoverBrowser info_cache: move to core (koreader#13018). Reset the book info from the cache and refresh history or fm
…ce the book is being read and we change the status to tbr, even though the tbr mark appears, the progress bar remains. This probably was introduced in commit 1750499, CoverBrowser info_cache: move to core (koreader#13018). Cleaning and fix some comments
Less calls of
DocSettings:open().This change is