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

Optimize menu #3657

Merged
merged 5 commits into from Feb 4, 2018

Conversation

Projects
None yet
4 participants
@robert00s
Contributor

robert00s commented Feb 2, 2018

Should fix this problem: #3589 (comment) and #3655

@robert00s robert00s referenced this pull request Feb 2, 2018

Closed

slow bookmarks? #3655

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 2, 2018

I had a look at your code, and I think it is really clever to use #item_name.vertical_string_list.

I then spent more than 30 mn trying to get what the next few lines do :) to the point of rewritting it with more explicite (to me) variable names and comments:

local it_height = item_name:getSize().h
if it_height < max_item_height then -- we fit !
    break
end
-- Don't go too low, and then decrease lines
if self.font_size <= 12 then
    it_nb_lines = #item_name.vertical_string_list
    -- I get the idea of the following line, but as it is written, I'm still puzzled, may be just because of the '- 1'
    can_fit_lines = math.floor(it_nb_lines * (max_item_height - 1) / it_height) -- why the -1 ?
    if can_fit_lines >= it_nb_lines  then -- ok, we got the same nb of lines that it made up
        can_fit_lines = it_nb_lines - 1  -- so, substract one
    end
    -- ok, from now on I get it
    offset = item_name.vertical_string_list[can_fit_lines + 1].offset
    -- self.text = table.concat(table.move(item_name.charlist, 1, offset - 4, 1, {})) .. "…"
    -- I didn't know 'table.move' , but I think you can do the same with only 'table.concat' :
    self.text = table.concat(item_name.charlist, '', 1, offset - 4) .. "~E"

But there's chance for it to be stuck in an infinite loop (if the -4 chars you remove have a width inferior to the width of your "…".

Then, I tested your PR on the emulator, no error, and it is really fast (instant vs 20 seconds before).

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 2, 2018

May be easier to read written as:

line_height = it_height / #item_name.vertical_string_list -- should be an integer
can_fit_nb_lines = math.floor(max_item_height / line_height)

or with your variable names:

if self.font_size <= 12 then
    local line_height = height / #item_name.vertical_string_list -- should be an integer
    lines = math.floor(max_item_height / line_height)
    offset = item_name.vertical_string_list[lines + 1].offset
    self.text = table.concat(item_name.charlist, '', 1, offset - 4) .. ""
else

May be you could do that (without the -4 and "…") only one time, to do the big text-length-reduction (set a flag to not redo it once it's done) and keep using the one-char-at-a-time from then, to be sure we keep reducing it and we don't fall in an infitite loop?

Or, do that one time, and then keep table.concat'ing item_name.charlist with a -4 that would increase each time (-1, -2, -3, -4...)- so you can still benefit from the item_name.charlist being already utf8 chars (and you don't risk to cut in the middle of a multibyte chars like previously).

@robert00s

This comment has been minimized.

Contributor

robert00s commented Feb 2, 2018

My propose:

            flag_fit = false
            [...]
            if height < max_item_height or flag_fit then -- we fit !
                break
            end
            -- Don't go too low, and then decrease lines
            if self.font_size <= 12 then
                local line_height = height / #item_name.vertical_string_list -- should be an integer
                lines = math.floor(max_item_height / line_height)
                offset = item_name.vertical_string_list[lines + 1].offset
                self.text = table.concat(item_name.charlist, '', 1, offset - 4) .. "…"
                flag_fit = true
            else

Or, do that one time, and then keep table.concat'ing item_name.charlist with a -4 that would increase each time (-1, -2, -3, -4...)

I do not want to do this because there is a risk of performance degradation.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Feb 2, 2018

I also think to create bookmark menu with single line like table of contents.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 2, 2018

What you proposed should be fine.

If you really wanted to get a correct number instead of the hardcoded -4 without looping for rendering the whole textboxwidget, you could do something like:

offset = item_name.vertical_string_list[lines + 1].offset
-- or should it be (to get the last char of real last used line) ? :
offset = item_name.vertical_string_list[lines + 1].offset - 1

ellipsis_size = RenderText:sizeUtf8Text("" etc)
removed_char_width= 0
while removed_char_width < ellipsis_size  do
  -- the width of each char has already been calculated by TextBoxWidget
  removed_char_width = removed_char_width + item_name.char_width_list[offset].w
  offset = offset - 1
end
self.text = table.concat(item_name.charlist, '', 1, offset) .. ""

(untested, there may be some +1/-1 bug somewhere :)

I also think to create bookmark menu with single line like table of contents.

May be later, but for now, it's real good stuff to test the robustness of this code :)

@Frenzie

This comment has been minimized.

Member

Frenzie commented Feb 2, 2018

Speaking of testing, unless the code coverage is missing something it seems that the specific lines related to this aren't currently covered by unit tests. Which is probably no surprise because they're new.

https://codecov.io/gh/koreader/koreader/src/master/frontend/ui/widget/menu.lua#L234...254

Also widget_menu_spec.lua is a bit rudimentary, so that should probably come as no surprise…

it("should convert item table from touch menu properly", function()
local cb1 = function() end
local cb2 = function() end
local re = Menu.itemTableFromTouchMenu({
navi = {
icon = 'foo/bar.png',
{ text = 'foo', callback = cb1 },
{ text = 'bar', callback = cb2 },
},
exit = {
icon = 'foo/bar2.png',
callback = cb2
},
})
assert.are.same(re, {
{
text = 'navi',
sub_item_table = {
icon = 'foo/bar.png',
{ text = 'foo', callback = cb1 },
{ text = 'bar', callback = cb2 },
}
},
{
text = 'exit',
callback = cb2,
}
})
end)

Just something to consider for a future PR, unless you feel really motivated to add it to this one. ;-) Note that you might have to externalize some functionality into separate functions so they can be called more easily, but architecturally that's generally a good thing anyway.

@codecov-io

This comment has been minimized.

codecov-io commented Feb 2, 2018

Codecov Report

Merging #3657 into master will decrease coverage by 0.03%.
The diff coverage is 43.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3657      +/-   ##
==========================================
- Coverage   52.83%   52.79%   -0.04%     
==========================================
  Files         202      202              
  Lines       26804    26878      +74     
==========================================
+ Hits        14161    14190      +29     
- Misses      12643    12688      +45
Impacted Files Coverage Δ
frontend/document/djvudocument.lua 71.6% <ø> (ø) ⬆️
frontend/document/credocument.lua 83.71% <ø> (ø) ⬆️
frontend/document/picdocument.lua 42.1% <ø> (ø) ⬆️
frontend/apps/filemanager/filemanager.lua 46.25% <0%> (-0.47%) ⬇️
frontend/document/pdfdocument.lua 90.44% <100%> (+0.05%) ⬆️
frontend/apps/reader/readerui.lua 75.33% <22.22%> (-2.66%) ⬇️
frontend/document/documentregistry.lua 71.02% <50.87%> (-23.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 016c17e...4960dd7. Read the comment docs.

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 2, 2018

Just got (with your latter commit), I don't know how, while quitting, but can't reproduce it:

./luajit: frontend/ui/widget/menu.lua:282: attempt to index a nil value
stack traceback:
 frontend/ui/widget/menu.lua:282: in function 'init'
 frontend/ui/widget/widget.lua:48: in function 'new'
 frontend/ui/widget/menu.lua:788: in function 'updateItems'

Guess they are case where item_name.vertical_string_list[lines + 1] does not exist...

if self.font_size <= 12 then
local line_height = height / #item_name.vertical_string_list -- should be an integer
local lines = math.floor(max_item_height / line_height)
local offset = item_name.vertical_string_list[lines + 1].offset - 2

This comment has been minimized.

@poire-z

poire-z Feb 2, 2018

Contributor

I don' t think it needs a -2, not even the -1 i thought. It looks to me like it work best without any substraction.
So, may be here, to avoid the thing I got once but can't reproduce:

                local offset
                if item_name.vertical_string_list[lines + 1] then
                    offset = item_name.vertical_string_list[lines + 1].offset
                else -- shouldn't happen, but just in case
                    offset = #item_name.char_width_list
                end

This comment has been minimized.

@robert00s

robert00s Feb 3, 2018

Contributor

Thanks, I made changes according to your suggestions.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Feb 3, 2018

@Frenzie
I add unit tests to my todo list :)

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 3, 2018

It looks and works fine for me now.

But just try to hit f on the emulator: it no more loops infinitely thanks to this PR, but what used to be a good looking non-fullscreen list is now a supersmall unreadable list (which should contain fonts name if I remember correctly).
It's not a super needed feature, but you may want to look at why that happens, if this does not reveal some other hidden problem.

function ReaderFont:onShowFontMenu()
-- build menu widget
local main_menu = Menu:new{
title = self.font_menu_title,
item_table = self.face_table,
width = Screen:getWidth() - 100,
}

May be it's just one of your new parameters to a Menu:new{} that is missing. You may want to check if there are no other such hidden/rarely used Menu.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Feb 3, 2018

We need to change this line from
offset = item_name.vertical_string_list[lines + 1].offset
to
offset = item_name.vertical_string_list[lines + 1].offset - 2

item_name.vertical_string_list[lines + 1].offset point to first char of next line. -1 point to space so last character of line is point by item_name.vertical_string_list[lines + 1].offset - 2

Sometimes without -2 we get one more line over max lines.

koreader_074

@robert00s

This comment has been minimized.

Contributor

robert00s commented Feb 3, 2018

But just try to hit f on the emulator: it no more loops infinitely thanks to this PR, but what used to be a good looking non-fullscreen list is now a supersmall unreadable list (which should contain fonts name if I remember correctly).

I look at it.

@robert00s

This comment has been minimized.

Contributor

robert00s commented Feb 3, 2018

Now should be better:

koreader_075

@poire-z

This comment has been minimized.

Contributor

poire-z commented Feb 3, 2018

item_name.vertical_string_list[lines + 1].offset point to first char of next line.-1 point to space so last character of line is point by item_name.vertical_string_list[lines + 1].offset - 2

Thanks for looking at it and the explanation (I was too lazy to really check what's at these offsets). So ok about the -2.
All fine for me now.

@poire-z poire-z merged commit 9c373f1 into koreader:master Feb 4, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@robert00s robert00s deleted the robert00s:menu_bookmarks branch Feb 4, 2018

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