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

Add book to favorites #5527

Merged
merged 7 commits into from Nov 5, 2019
Merged

Add book to favorites #5527

merged 7 commits into from Nov 5, 2019

Conversation

@robert00s
Copy link
Contributor

robert00s commented Oct 24, 2019

New function that support organize books to favorites.
We can:

  • add files to favorites
  • remove files from favorites
  • view list of favorites books
  • sort list of favorites books
  • open books from favorites
  • change view mode (like in history)
    Now we support only one collection (named favorites) but code is preparted to extend to more collections.

Some screenshots:

Close: #5099

@Frenzie Frenzie added this to the 2019.11 milestone Oct 24, 2019
Copy link
Member

Frenzie left a comment

Looks very interesting! :-)

{
text = _("Open with…"),
enabled = lfs.attributes(file, "mode") == "file" and (DocumentRegistry:getProviders(file) == nil
or #(DocumentRegistry:getProviders(file)) > 1),
enabled = DocumentRegistry:getProviders(file) ~= nil

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

What's the relation between this change and #5497? At a glance it looks like an accidental reversal?

This comment has been minimized.

Copy link
@robert00s

robert00s Oct 24, 2019

Author Contributor

Yes, you're right. This was taken from commit before #5497.

end,
enabled = DocumentRegistry:getProviders(file) ~= nil,
callback = function()
if require("readcollection"):checkItemExist(file, "favorites") then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

While the results of require are cached, it should still be preferable to do local ReadCollection = require(…). (And it should be fine at the top of the file I figure?)

@@ -695,9 +720,10 @@ function FileManager:pasteHere(file)
self:moveFile(DocSettings:getSidecarDir(orig), dest) -- dest is always a directory
end
if self:moveFile(orig, dest) then
--update history
--update history and collections

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

If we're updating comments, they like to be proper sentences. ;-)

Suggested change
--update history and collections
-- Update history and collections.
@@ -789,6 +816,8 @@ function FileManager:deleteFile(file)
end
doc_settings:purge()
end
--remove from collections

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

Seems slightly redundant with the code.

Suggested change
--remove from collections
@@ -804,6 +833,8 @@ function FileManager:renameFile(file)
if util.basename(file) ~= self.rename_dialog:getInputText() then
local dest = util.joinPath(util.dirname(file), self.rename_dialog:getInputText())
if self:moveFile(file, dest) then
-- update collections

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member
Suggested change
-- update collections
end

function FileManagerCollection:addToMainMenu(menu_items)
-- insert table to main tab of filemanager menu

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

Well, technically just wherever MenuSorter is told to put it by the user. ;-)

But this is the same as every addToMainMenu function; I don't think a comment adds much?

Suggested change
-- insert table to main tab of filemanager menu
end

function FileManagerCollection:updateItemTable()
-- try to stay on current page

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member
Suggested change
-- try to stay on current page
-- Try to stay on current page.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 24, 2019

Member

Actually that should be the current page.

Copy link
Member

Frenzie left a comment

I believe @poire-z is on vacation for a week or two; any comments @NiLuJe ?

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Oct 25, 2019

Feature sounds nice, but I wonder if it wouldn't be better suited to the SQL db than another LuaSetting?

@robert00s

This comment has been minimized.

Copy link
Contributor Author

robert00s commented Oct 26, 2019

@NiLuJe
I think that we can move all (or almost all) lua settings to SQL database. But we need to thinks how to do it (how many databases, which settings should be moved, what about table data (serialize?). )
For now I don't want start that job. We have to discuss this with a larger group.

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Oct 26, 2019

I meant specifically the BookInfo db, because that felt vaguely related (and possibly much more flexible), but fair enough ;).

This isn't designed to handle large collections anyway, so that's fine by me. I can see it being used to track a TBR pile, for instance.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 30, 2019

I meant specifically the BookInfo db, because that felt vaguely related

I thought we discussed that previously, but couldn't find anything longer that #5100 (comment):

I somehow like/wanted that the bookinfo.sdb is just a cache and can be trashed, and that we don't need to sync things in multiple places :)

I still do :)
And I am fine this being saved as classic lua settings (it it is, will read this PR later :)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 31, 2019

Regarding the new button (Add/Remove to/from favorites) added to the onHold dialog:
I'd rather see Book information on the right, more easily accessible than if it were on the left):
image

Also, the CoverBrowser plugin do re-order buttons, as it adds one, and hack the Book info one - so you might want to fix it so it does the same things with your new buttons hierarchy. For now, it crashes:

./luajit: plugins/coverbrowser.koplugin/covermenu.lua:226: attempt to index a nil value
stack traceback:
        plugins/coverbrowser.koplugin/covermenu.lua:226: in function 'onFileHold'
        frontend/ui/widget/filechooser.lua:357: in function 'onMenuHold'
        plugins/coverbrowser.koplugin/listmenu.lua:752: in function 'handleEvent'
        frontend/ui/widget/container/inputcontainer.lua:259: in function 'handleEvent'

-- Replace Book information callback to use directly our bookinfo
orig_buttons[4][3].callback = function()
FileManagerBookInfo:show(file, bookinfo)
UIManager:close(self.file_dialog)
end
-- Fudge the "Purge .sdr" button ([1][3]) callback to also trash the cover_info_cache
local orig_purge_callback = orig_buttons[1][3].callback
orig_buttons[1][3].callback = function()
-- Wipe the cache
if self.cover_info_cache and self.cover_info_cache[file] then
self.cover_info_cache[file] = nil
end
-- And then purge the sidecar folder as expected
orig_purge_callback()
end
-- Add some new buttons to original buttons set
table.insert(orig_buttons, {
{ -- Mark the book as read/unread

I don't know where you'll want to put your new button among:
image

local SortWidget = require("ui/widget/sortwidget")
local sort_item
sort_item = SortWidget:new{
title = _("Sort favorites book"),

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Contributor

Sort favorite books or just Sort favorites

text = _("Book information"),
enabled = FileManagerBookInfo:isSupported(item.file),
callback = function()
FileManagerBookInfo:show(item.file)
Comment on lines +81 to +84

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Contributor

Later, you might want to tweak the onHold dialog when the user uses a CoverBrowser mode, like we did for History in:

-- Similar to onFileHold setup just above, but for History,
-- which is plugged in main.lua _FileManagerHistory_updateItemTable()
function CoverMenu:onHistoryMenuHold(item)

so we have the other buttons like View book description, View full size cover...

main = {
"history",
"collections",
"open_last_document",
"----------------------------",
Comment on lines 129 to 133

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Contributor

For both reader and filebrowser hamburger menu, I'd rather see history and open_last/previous_document grouped together. As collections/favorites is not related to history/recent file, may be:

        "history",
        "open_last_document",
        "----------------------------",
        "collections",
        "----------------------------",
local series_mode = nil -- defaults to not display series
local collection_display_mode = false -- not initialized yet
Comment on lines 46 to 47

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Contributor

Just swap these 2. series_mode is a bit unrelated to the now 3 real display modes.

coll_menu._updateItemsBuildUI = MosaicMenu._updateItemsBuildUI
-- Set MosaicMenu behaviour:
coll_menu._do_cover_images = display_mode ~= "mosaic_text"
-- no need for do_hint_opened with Collections

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Contributor

Why not?

-- no need for do_hint_opened with Collections

end
coll_menu._do_hint_opened = BookInfoManager:getSetting("history_hint_opened")

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Contributor

I would naively just set coll_menu._do_hint_opened = true , or = false (as you wrote above no need for do_hint_opened with Collections). Bur probably not the setting that is named after / targeted for History.

return -- starting in classic mode, nothing to patch
end

-- We only need to replace one FileManagerHistory method

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Contributor

one FileManagerCollection method

local getFriendlySize = require("util").getFriendlySize
local lfs = require("libs/libkoreader-lfs")
local realpath = require("ffi/util").realpath
local util = require("frontend/util")

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Contributor

No need for frontend/ in our require.

function ReadCollection:read(collections)
local collection_handle = LuaSettings:open(collection_file)
local coll = collection_handle:readSetting(collections) or {}
Comment on lines 12 to 14

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 31, 2019

Contributor

That file was a bit hard to understand :)

I guess that collections (plural) here , collection (singular) below is a collection name, and you pass "favorites" as a string when you call it (your single default collection name for now, as I understand we'll be able to manage more later).
So, may be use collection_name everywhere as the variable holding that collection name string, to make it easier to understand.
And use collections (plural) when you're dealing with the whole datastructure. And collection (singular) when dealing with a sub-table of a specific collection_name.

And may be, to avoid passing "favorites" everywhere else, just pass nothing, and in here:

local DEFAULT_COLLECTION_NAME = "favorites"
[...]
function ReadCollection:something(collection_name)
  if not collection_name then collection_name = DEFAULT_COLLECTION_NAME end
@robert00s

This comment has been minimized.

Copy link
Contributor Author

robert00s commented Nov 2, 2019

@poire-z Something like that? :)

obraz

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 2, 2019

Mhhh, I don't like it :)

I'd rather keep all these as-is at the bottom, as they are all related to cover browser, and the last one needs the full width for the long description (couldn't find anything smaller, and I think it needs to be that explicite).
image

So, either, for the middle group, 2 lines (3 + 2 or 2 + 3), or 3 lines (2 + 1 + 2 or 2 + 2 + 1) like:

Open with...     |      Book information
Convert | Mark as read | Add to favorite

or

Open with...     |      Book information
Convert | Add to favorite | Mark as read

or

Open with... | Book information
     Add to favorite
Convert      |     Mark as read

Given that Convert is only enabled with .md files (Mardown only for now, but there hasn't been any more format added for ages), and these are rare, I would happily just not display it in these menus, and add it only when meeting a converter supported file (even as a single full width button).
That way, we would not need to have 3 buttons in a row.
@Frenzie : any opinion?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 2, 2019

Sure, you can hide convert if you need to win some space.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 2, 2019

OK, So may be we can go with this:
Only when converting is supported, add a line with a single button Convert above Open with....

That would give in classic mode:

Copy  |  Paste  | Purge .sdr
Cut   |  Delete | Rename
========================
     Convert (if convertable)
Open with... | Book information
       Add to favorite

That would give in CoverBrowser modes:

Copy  |  Paste  | Purge .sdr
Cut   |  Delete | Rename
========================
     Convert (if convertable)
Open with... | Book information
Mark as read |  Add to favorite
View fscover | View book description
Ignore cover | Ignore metadata
Refresh cached book information

You'll have to tweak the buttons move and adding code in covermenu.lua to adapt to this new layout, and the present or absent Convert.
So, may be change my above drawings and move Convert below Add to favorite, if that can make the above easier, as Book info and the place where to add Mark as read will always have the same indice:

Copy  |  Paste  | Purge .sdr
Cut   |  Delete | Rename
========================
Open with... | Book information
       Add to favorite
     Convert (if convertable)
Copy  |  Paste  | Purge .sdr
Cut   |  Delete | Rename
========================
Open with... | Book information
Mark as read |  Add to favorite
     Convert (if convertable)
View fscover | View book description
Ignore cover | Ignore metadata
Refresh cached book information
@robert00s

This comment has been minimized.

Copy link
Contributor Author

robert00s commented Nov 3, 2019

@poire-z
Thanks for review and tips. I did all things :)

@poire-z
poire-z approved these changes Nov 3, 2019
Copy link
Contributor

poire-z left a comment

I've checked the onHold menus' buttons, and everything seems allright :)

Can you just move the 3 "Show hints..." in the Other settings> menu in a new submenu Display hints> (just above Series>), just so that this Other settings> menu is a single page?
image

local collection_handle = LuaSettings:open(collection_file)
local coll = collection_handle:readSetting(collections) or {}
function ReadCollection:read(collection_name)
if not collection_name then collection_name = DEFAULT_COLLECTION_NAME end

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 3, 2019

Contributor

Haven't read carefully this whole file :) But are you sure you don't need this if not collection_name then collection_name = DEFAULT_COLLECTION_NAME end in all the other functions below (prepareList, removeItem, addItem, checkItemExist...) ?

This comment has been minimized.

Copy link
@robert00s

robert00s Nov 3, 2019

Author Contributor

Because all that functions begin with
local coll = self:read(collection_name)
so we don't need to use
if not collection_name then collection_name = DEFAULT_COLLECTION_NAME end
second time.

@@ -265,6 +270,54 @@ function CoverBrowser:addToMainMenu(menu_items)
separator = true,
},
},
},
{
text = _("Collection display mode"),

This comment has been minimized.

Copy link
@poire-z

poire-z Nov 3, 2019

Contributor

May be plural? Collections display mode.
Even if for now, we only have a single collection named Favorites. So, to avoid confusion, may be Favorites display mode until we get more :).
(Then, the only place we would see the word "Collection" is when holding on a file in the Favorites list, which would be ok - if I didn't miss any other.)

@poire-z
poire-z approved these changes Nov 3, 2019
@robert00s

This comment has been minimized.

Copy link
Contributor Author

robert00s commented Nov 3, 2019

rev4:

  • prevent crash when we hold on folder,
  • hold on folder in cover mode should show original buttondialog (from classic mode),
  • should fix #5557

@poire-z
Please check changes in last commit :)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 3, 2019

Looks simple enough :)
I'm ok with letting the original dialog on directories.
I'd be inclined to hide the test into bookinfomanager.lua,, and do it that way (so we can now that for other stuff if needed):

--- a/plugins/coverbrowser.koplugin/bookinfomanager.lua
+++ b/plugins/coverbrowser.koplugin/bookinfomanager.lua
@@ -268,5 +268,6 @@ function BookInfoManager:getBookInfo(filepath, get_cover)
     -- returns a bookinfo like-object enough for a correct display and
     -- to not trigger extraction, so we don't clutter DB with such files.
-    if not DocumentRegistry:hasProvider(filepath) then
+    local is_directory = lfs.attributes(filepath, "mode") == "directory"
+    if is_directory or not DocumentRegistry:hasProvider(filepath) then
         return {
             directory = directory,
@@ -278,4 +279,6 @@ function BookInfoManager:getBookInfo(filepath, get_cover)
             ignore_meta = "Y",
             ignore_cover = "Y",
+            -- for CoverMenu to *not* extend the onHold dialog:
+            _is_directory = is_directory,
             -- for ListMenu to show the filename *with* suffix:
             _no_provider = true
diff --git a/plugins/coverbrowser.koplugin/covermenu.lua b/plugins/coverbrowser.koplugin/covermenu.lua
index fd4bbf0b..19abde4c 100644
--- a/plugins/coverbrowser.koplugin/covermenu.lua
+++ b/plugins/coverbrowser.koplugin/covermenu.lua
@@ -205,6 +205,6 @@ function CoverMenu:updateItems(select_number)

                 local bookinfo = BookInfoManager:getBookInfo(file)
-                if not bookinfo then
-                    -- If no bookinfo (yet) about this file, let the original dialog be
+                if not bookinfo or bookinfo._is_directory then
+                    -- If no bookinfo (yet) about this file, or it's a directory, let the original dialog be
                     return true
                 end
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 4, 2019

(Little luacheck warning in case you didn't see it: plugins/coverbrowser.koplugin/covermenu.lua:9:7: unused variable 'lfs')

@poire-z
poire-z approved these changes Nov 4, 2019
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 5, 2019

@Frenzie : can we merge this PR? Or do you prefer we wait after the 2019.11 release (coming soon?), because of translations (this PR adds a few new strings like "Favorites" that are quite top level in the various menus) - or will translators have enough time to translate them?

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Nov 5, 2019

Sure, go ahead if you guys think it's ready. About 4 days is enough for quite a few translations.

@poire-z poire-z merged commit 371e333 into koreader:master Nov 5, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@boriar

This comment has been minimized.

Copy link

boriar commented Nov 8, 2019

I know that at the moment this is only a favorite list but, will it be a full collection's support?

@robert00s robert00s deleted the robert00s:collections2 branch Nov 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.