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

Wrap names and configure number of items per page #3589

Merged
merged 5 commits into from Jan 13, 2018

Conversation

Projects
None yet
5 participants
@robert00s
Contributor

robert00s commented Jan 10, 2018

Close: #2013 and #3488

  • Allow filenames to wrap so that we can see the full name
  • Configure number of items per page (from 6 to 24) - default is 14
  • The font size is calculated depending on number of items
  • Used for File browser, History, Search Result, Table of contents (only single line), File chooser, OPDS catalog

koreader_052
koreader_053
koreader_054
koreader_055
koreader_056
koreader_057
koreader_058
koreader_059

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jan 10, 2018

Looks good at a quick skim. 👍

function FileManager:onRefresh()
self.file_chooser:refreshPath()
function FileManager:onRefresh(value)
self.file_chooser:refreshPath(value)

This comment has been minimized.

@poire-z

poire-z Jan 10, 2018

Contributor

I would replace value with update_perpage as it is named in the function you call, so we know here what value means.
But I'm not sure any value is needed, see next comment.

function FileChooser:refreshPath(update_perpage)
if update_perpage then
self.perpage = G_reader_settings:readSetting("items_per_page")
end

This comment has been minimized.

@poire-z

poire-z Jan 10, 2018

Contributor

G_reader_settings:readSetting() costs nothing, so I guess you could do without any update_perpage argument.
And may be it will solve the fact that items_per_page is lost when you go from Classic mode (the item_per_page chosen) to Mosaic mode (hardcoded 3x3= 9 per_page) and back to classic_mode (you stay with 9 item per_page).

This comment has been minimized.

@robert00s

robert00s Jan 11, 2018

Contributor

Thanks, I also notice that (items_per_page is lost when you go from Classic mode (the item_per_page chosen) to Mosaic mode).
I didn't know that G_reader_settings:readSetting() costs nothing :)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 10, 2018

Looking really nice, and working really well after a few minutes testing on the emulator.

I verified the loss of item_per_page value when switching to some coverbrowser mode and back to classic mode is solved but what I suggested in the above comments.

One small thing: on the emulator (and devices with keypad), when you use the up and down arrow keys to move items, the UnderlineContainer (color is white/transparent per defaut, you switched it to grey to have your line separator) has its color toggled to black for the currently selected item. This looks strange now, as your visited items get their underline white again, so the line separators disappear as you navigate them. This underline stuff is broken anyway (when you change page, it's all messed up), so I don't know how much this needs fixing. No problem for key-less devices I guess. I don't know if we have real devices and users that use keys and would witness this ugly thing.

(I'm not fond of the line separator in TOC and bookmarks, but may be I'll get used to it - If I and others don't :) may be the underlinecontainer color could stay white if single_line == true ?)

robert00s added some commits Jan 11, 2018

@robert00s

This comment has been minimized.

Contributor

robert00s commented Jan 11, 2018

@poire-z

(I'm not fond of the line separator in TOC and bookmarks, but may be I'll get used to it - If I and others don't :) may be the underlinecontainer color could stay white if single_line == true ?)

Done in 289f7fa

@@ -218,6 +218,7 @@ function ReaderBookmark:onShowBookmark()
width = Screen:getWidth(),
height = Screen:getHeight(),
cface = Font:getFace("x_smallinfofont"),
line_color = require("ffi/blitbuffer").COLOR_WHITE,

This comment has been minimized.

@poire-z

poire-z Jan 11, 2018

Contributor

May be you need perpage here (like you have it below in readertoc). Otherwise, if you change item_per_page via spin widget, it's not updated till next koreader restart.

Fix3: select items on key device
define item_per_page
@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 11, 2018

I just noticed that when you go from reader to filemanager, when in classic mode, you seem to get back to the page where opened document was. But this seems to be broken now when you are in any of the coverbrowser modes. (I did that in #3351, modified in #3391 - may be related or not: #3247 - it's been some time since I did that :) and it's not obvious to me after a quick glance where the problem lies, tell me if you need me to dig more into it).
edit: may be it is solved with the following:

  function FileChooser:refreshPath()
-+    self.perpage = G_reader_settings:readSetting("items_per_page")
++--    self.perpage = G_reader_settings:readSetting("items_per_page")

and

  function Menu:_recalculateDimen()
++    perpage = G_reader_settings:readSetting("items_per_page") or 14

(haven't checked much for side effects, but it looks logical, recalculateDimen is overriden by coverbrowsers modes, and refreshPath calls _realculateDimen 2 or 3 function calls down, so if perpage is not needed till then, you could delay it)

(Also, may be related, but that's a minor annoyance: when you switch display modes, you're flying to some other page - I think I did try to stay on the same page, showing the page containing the first item of previous mode's current page).

@robert00s

This comment has been minimized.

Contributor

robert00s commented Jan 13, 2018

@poire-z Your solution looks good.

@@ -34,6 +34,7 @@ local FileChooser = Menu:extend{
collate = "strcoll", -- or collate = "access",
reverse_collate = false,
path_items = {}, -- store last browsed location(item index) for each path
perpage = G_reader_settings:readSetting("items_per_page"),

This comment has been minimized.

@Frenzie

Frenzie Jan 13, 2018

Member

Shouldn't that be per_page?

This comment has been minimized.

@robert00s

robert00s Jan 13, 2018

Contributor

It's good :) We use perpage variable.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 13, 2018

It looks all fine to me now.
Regarding my comment:

(Also, may be related, but that's a minor annoyance: when you switch display modes, you're flying to some other page - I think I did try to stay on the same page, showing the page containing the first item of previous mode's current page).

I checked on the current version, and it is already flying (well, actually, it stays on the same page number, which, given that perpage is changing, brings you to another set of book covers). Thought I had fixed that, but may be I just didnt succeed. Anyway, no pb with this PR in relation to that.

@mwoz123

This comment has been minimized.

Contributor

mwoz123 commented Jan 13, 2018

Great job. Many thanks @robert00s.

Not sure if you looked /changed that but would there been a chance to had different sort order for folder?
E. G. All dirs would have ascending order, and news downloader would have descending order?

@poire-z poire-z merged commit d163f82 into koreader:master Jan 13, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 13, 2018

Dunno why the commit CI took such a long time, there was a 5 mn pause:

01/13/18-22:43:00 INFO  no dialog left to show
01/13/18-22:43:00 INFO  quitting uimanager
01/13/18-22:43:03 INFO  no dialog left to show
01/13/18-22:43:03 INFO  quitting uimanager
[       OK ] spec/front/unit/readerbookmark_spec.lua @ 180: ReaderBookmark module bookmark for PDF document should sort bookmarks with descending page numbers (5257.06 ms)
[ RUN      ] spec/front/unit/readerbookmark_spec.lua @ 191: ReaderBookmark module bookmark for PDF document should keep descending page numbers after removing bookmarks
01/13/18-22:43:04 INFO  no dialog left to show
01/13/18-22:43:04 INFO  quitting uimanager
01/13/18-22:43:06 INFO  no dialog left to show
01/13/18-22:43:06 INFO  quitting uimanager
[       OK ] spec/front/unit/readerbookmark_spec.lua @ 191: ReaderBookmark module bookmark for PDF document should keep descending page numbers after removing bookmarks (3578.97 ms)
[ RUN      ] spec/front/unit/readerbookmark_spec.lua @ 202: ReaderBookmark module bookmark for PDF document should add bookmark by highlighting
01/13/18-22:43:10 INFO  no dialog left to show
01/13/18-22:43:10 INFO  quitting uimanager
01/13/18-22:48:03 INFO  no dialog left to show
01/13/18-22:48:03 INFO  quitting uimanager
[       OK ] spec/front/unit/readerbookmark_spec.lua @ 202: ReaderBookmark module bookmark for PDF document should add bookmark by highlighting (296364.04 ms)
[ RUN      ] spec/front/unit/readerbookmark_spec.lua @ 209: ReaderBookmark module bookmark for PDF document should get previous bookmark for certain page
[       OK ] spec/front/unit/readerbookmark_spec.lua @ 209: ReaderBookmark module bookmark for PDF document should get previous bookmark for certain page (0.71 ms)

300s had be taken by:

        it("should add bookmark by highlighting", function()
            highlight_text(readerui, Geom:new{ x = 260, y = 70 }, Geom:new{ x = 260, y = 150 })
            readerui.bookmark:onShowBookmark()
            show_bookmark_menu(readerui)
            Screen:shot("screenshots/reader_bookmark_6marks_pdf.png")
            assert.are.same(6, #readerui.bookmark.bookmarks)
        end)

@robert00s robert00s deleted the robert00s:wrap_menu branch Jan 19, 2018

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 27, 2018

I don't use highlights much, but I think those who do and highlight quite long texts may have to wait quite a long time when opening the Bookmarks window.
On the emulator, if I make 3 highlights with half of the screen selected, I get to wait >5 seconds while menu.lua is trying to truncate the strings till they can fit.
With some logs added, here's what it tries (with a small string, truncated here):

01/27/18-14:48:32 WARN  trying 16 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27 14:45:43
01/27/18-14:48:32 WARN  trying 14 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27 14:45:43
01/27/18-14:48:32 WARN  trying 12 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27 14:45:43
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27 14:45:43
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27 14:4â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27 14:â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27 14â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27 1â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27 â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-27â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-2â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01-â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-01â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-0â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018-â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2018â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 201â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 20â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ 2â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @ â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite @â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suite â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suiteâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suitâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suiâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la suâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la sâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente la â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente laâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente lâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trente â¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trenteâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trentâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Trenâ¦
01/27/18-14:48:32 WARN  trying 10 with Page 27 dissoute [...] tyrannie des Treâ¦

Not sure how we can optimize that (when reaching smallest font size, use dichotomy - cut the stirng in half, if it fits, cut it to 3/4, etc... ?)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jan 27, 2018

Could you point to the lines in question to see what exactly happens right now? But yes, one of the most obvious optimizations is clearly what you just said.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 27, 2018

This whole while true loop, and particulary when we reach line 278's condition and begin to truncate text one utf8 char at a time line 279 (incidentaly, we may here truncate it a the middle of a multibyte utf8 char - I remember there's been a fix some weeks ago for a case like that):

while true do
-- Free previously made widgets to avoid memory leaks
if item_name then
item_name:free()
end
item_name = TextBoxWidget:new {
text = self.text,
face = Font:getFace(self.font, self.font_size),
width = self.content_width - mandatory_w - state_button_width - text_mandatory_padding,
alignment = "left",
bold = self.bold,
fgcolor = self.dim and Blitbuffer.COLOR_GREY or nil,
}
local height = item_name:getSize().h
if height < self.dimen.h - 2 * self.linesize then -- we fit !
break
end
-- Don't go too low, and then truncate text
if self.font_size < 12 then
self.text = self.text:sub(1, -5) .. ""
else
-- If we don't fit, decrease font size
self.font_size = self.font_size - 2
end
end

@Frenzie

This comment has been minimized.

Member

Frenzie commented Jan 27, 2018

I'm not sure if we can avoid doing a bunch of rendering the first time, which could still be greatly reduced by your suggestion, but couldn't we at the very least effectively get rid of it for all subsequent entries? I assume it's reasonable enough to take the width of one n as an average character width, based on which you have a simple max length in (UTF-8) characters.

Also you could profile if something like string.format versus concatenation in line 279 makes a difference if you were really curious. Although whichever way it goes I assume it's bupkes compared to all the rendering.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Jan 27, 2018

I try to fix it. Idea with average character width is really nice.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Jan 28, 2018

Another thing to check for: on the emulator, when i hit the F key (I often do that by error - i think it brings some menu with the fonts), it got in an infinite loop (or I got bored waiting after 1mn):

01/28/18-20:05:07 DEBUG TextBoxWidget:free called
01/28/18-20:05:07 DEBUG TextBoxWidget:free called
[... many more...]
01/28/18-20:05:07 DEBUG TextBoxWidget:free called
01/28/18-20:05:07 DEBUG TextBoxWidget:free called
/luajit: frontend/ui/widget/textboxwidget.lua:104: interrupted!
stack traceback:
  frontend/ui/widget/textboxwidget.lua:104: in function 'init'
  frontend/ui/widget/widget.lua:48: in function 'new'
  frontend/ui/widget/menu.lua:265: in function 'init'
  frontend/ui/widget/widget.lua:48: in function 'new'
  frontend/ui/widget/menu.lua:783: in function 'updateItems'
  frontend/ui/widget/menu.lua:730: in function 'init'
  frontend/ui/widget/widget.lua:48: in function 'new'
  frontend/apps/reader/modules/readerfont.lua:144: in function 'handleEvent'

@Frenzie

This comment has been minimized.

Member

Frenzie commented Aug 30, 2018

@robert00s Btw, there's been a request to make the font size steady: https://www.mobileread.com/forums/showthread.php?p=3742175#post3742175

Any thoughts? :-)

@KenMaltby

This comment has been minimized.

KenMaltby commented Aug 30, 2018

Thanks for catching that MR post, I couldn't think of any good approach, given all the good new display mode options. The only thing I would have done is suggest to the OP that he try raising an issue here.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Sep 1, 2018

Here try to add option:
single_line = true,
about line 217

local bm_menu = Menu:new{
title = _("Bookmarks"),
item_table = self.bookmarks,
is_borderless = true,
is_popout = false,
width = Screen:getWidth(),
height = Screen:getHeight(),
cface = Font:getFace("x_smallinfofont"),
perpage = G_reader_settings:readSetting("items_per_page") or 14,
line_color = require("ffi/blitbuffer").COLOR_WHITE,
on_close_ges = {
GestureRange:new{
ges = "two_finger_swipe",
range = Geom:new{
x = 0, y = 0,
w = Screen:getWidth(),
h = Screen:getHeight(),
},
direction = "east"
}
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment