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

New algorithm to show items in menu widget #5496

Merged
merged 5 commits into from Oct 22, 2019

Conversation

@robert00s
Copy link
Contributor

robert00s commented Oct 16, 2019

See: #5388
New algorithm:
When text is too long or too height:

  • first: we try to decrease number of lines (eg from 3 to 2 and from 2 to 1 line)
  • second: when 1 line is too height we try to decrease font size (-1)
    And at the end we try to add or remove at least one letter to better fit text.

Needs to be tested. Shouldn't be merge before 2019.10.

Close: #5388

@robert00s robert00s added this to the 2019.11 milestone Oct 16, 2019
local offset_prev = item_name_orig.vertical_string_list[num_lines - 1].offset
text_last_line = table.concat(item_name_orig.charlist, '', offset_prev, offset)
Comment on lines 350 to 351

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 16, 2019

Contributor

One of the things I intend to do is to move that kind of stuff into TextBoxWidget.
It already has a "private" TextBoxWidget:_getLineText(vertical_string) which does what you do, so I guess it just have to have a "public" TextBoxWidget:getLineText(line_num) to give you that.
So, not asking you to do it :) This is fine for now.

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member

Random single quotes instead of double.

-- try to add chars to better align
while item_name.width > text_size_increase + ellipsis_size and offset < max_offset do
Comment on lines 362 to 363

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 16, 2019

Contributor

What's the reason for this to be needed?
I wondered for a few minutes - but is it just because TextBoxWidget wrap by words, so the last line you keep may be shorter than the textboxwidget width because a wrap happened and a long word has been pushed onto the next line. So, here, you're trying to grab some chars from that long word to put it at end of last line, before the "...", so the "..." touches the screen right border.
Is it that? Is that the only reason for this to be needed and work?

This comment has been minimized.

Copy link
@robert00s

robert00s Oct 16, 2019

Author Contributor

That's exactly how it works :)

-- remove chars when text is too long
while item_name.width <= text_size + ellipsis_size do
Comment on lines +370 to +371

This comment has been minimized.

Copy link
@poire-z

poire-z Oct 16, 2019

Contributor

And the, why does it happen you need to remove chars? (because you added some and made sure they fitted with the ellipsis)?
Or does this happen and is only needed when you actually did not have to add some chars? So, just to get rid of the orignal chars on that last line, to make room for the ellipsis?

This comment has been minimized.

Copy link
@robert00s

robert00s Oct 16, 2019

Author Contributor

Second sentence. It's to make a room for ellipsis.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 16, 2019

I'm now a bit more familiar with that code, after having contemplated it for some time before you stopped me :)

I think this multi-line display of Menu is only used by File browser when in Classic display mode.
(Are there others?)
So, I probably won't mind this big change, but there is a big change :)

Now, you're truncating text at 1 or 2 lines. If 2 lines fit, well, all done, we'll display the text truncated.
If 1 line does not fit (which I guess is an extrem case), only then you decrease the font size (in that case, I'm not sure by looking at the code, but aren't you left with the text truncated at some point, and when decreasing font size, you won't grab more text (as it is truncated) as they can fit on the single line, now that it has a smaller font size? - or is it what your code at my 2nd comment is also trying to fix? - if yes, it might be a bit convoluted, as you could just recreate a TextBoxWidget with the original text to get the most that fits with that final font size you decided on.)

So, the big change is that we won't get anymore the full text with a smaller font size, as we do currently.

Before:
image

After:
image
The 2nd line is shown with the font size like the 1st and the 3rd, so showing a bit less (truncated) that what we previously show untruncated.

Before:
image

After:
image

Again, I don't really mind this change if it applies only to File browser classic - as we mostly get the previous behaviour with the Detailed with filename mode:
image

(I prefer the previous behaviour, but I understand some people may prefer a fixed size - even if less is shown. They currently can not get that. With this PR, they would have an option for that.)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 16, 2019

Another thing I noticed earlier.
In the multiline code, we decrease the "mandatory" (the item on the right) font size until it fits.
We don't do that in the single_line code, although everything is the same regarding the mandatory.
Moreover, we try to adjust the infont_size vs perpage, so it's just right and no decrease should be needed:

--font size between 12 and 18 for better matching
local infont_size = math.floor(18 - (self.perpage - 6) / 3)
--default font size between 14 and 24 for better matching
local font_size = G_reader_settings:readSetting("items_font_size") or math.floor(24 - ((self.perpage - 6)/ 18) * 10 )

I think the same should apply for the multiline main name font size - so that decreasing of font size, in this PR that displays only one line (but not before this PR), might be just a precaution in case the above formulae is a bit wrong and would find out a too large font size.

So, we could get rid of the mandatory font size loop in any case.
And if going with this PR, the main name font size loop may also be killed, to make the code simpler :)

@robert00s

This comment has been minimized.

Copy link
Contributor Author

robert00s commented Oct 16, 2019

I think this multi-line display of Menu is only used by File browser when in Classic display mode.
(Are there others?)

It's also used in Bookmark.

So, the big change is that we won't get anymore the full text with a smaller font size, as we do currently.

Yes. I wanted to stay with fixed size, as long as possible. I think it looks prettier than every element looks similar (but it's only my opinion :))

(I prefer the previous behaviour, but I understand some people may prefer a fixed size - even if less is shown. They currently can not get that. With this PR, they would have an option for that.)

If user want to see more text he/she can always decrease font size in items -> font size or decrease items per page

At the end. This is only my propose. If you have better implementation or another idea I don't insist on my solution :)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 16, 2019

It's also used in Bookmark.

Oh, right. Seeing more of the text is more useful there :) (But I'm not a heavy bookmarker, so not minding either).

I think it looks prettier than every element looks similar (but it's only my opinion :))

Valid opinion (and it was my argument in #5435 - so I suddently agree with you :)

At the end. This is only my propose. If you have better implementation or another idea I don't insist on my solution :)

No better idea (nor any real interest in having one).
Let's see what other people think. (Pinging @gerhaher who seems to use classic mode - but with many per-page item, so used to seeing only one line.)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 16, 2019

While testing bookmarks, I got a reproducible infinite loop (I have to Ctrl-C) with just this PR:
Select 10 lines of text in a book, highlight. Go to top menu > Bookmarks:

[many of these for many seconds:]
10/16/19-21:45:00 DEBUG TextBoxWidget:free called
10/16/19-21:45:00 DEBUG TextBoxWidget:free called
10/16/19-21:45:00 DEBUG TextBoxWidget:free called
10/16/19-21:45:00 DEBUG TextBoxWidget:free called
10/16/19-21:45:00 DEBUG TextBoxWidget:free called
10/16/19-21:45:00 DEBUG TextBoxWidget:free called
10/16/19-21:45:00 DEBUG TextBoxWidget:free called
10/16/19-21:45:00 DEBUG TextBoxWidget:free called
10/16/19-21:45:00 DEBUG TextBoxWidget:free called
^C./luajit: interrupted!
stack traceback:
       [C]: in function 'insert'
       frontend/cache.lua:128: in function 'check'
       frontend/ui/rendertext.lua:88: in function 'getGlyph'
       frontend/ui/rendertext.lua:231: in function 'renderUtf8Text'
       frontend/ui/widget/textboxwidget.lua:380: in function '_renderText'
       frontend/ui/widget/textboxwidget.lua:125: in function 'init'
       frontend/ui/widget/widget.lua:48: in function 'new'
       frontend/ui/widget/menu.lua:281: in function 'init'
       frontend/ui/widget/widget.lua:48: in function 'new'
       frontend/ui/widget/menu.lua:1040: in function 'updateItems'
       frontend/ui/widget/menu.lua:951: in function 'init'
       frontend/ui/widget/widget.lua:48: in function 'new'
       frontend/apps/reader/modules/readerbookmark.lua:216: in function 'onShowBookmark'
       frontend/apps/reader/modules/readerbookmark.lua:40: in function 'callback'
       frontend/ui/widget/touchmenu.lua:729: in function 'action'
       frontend/ui/uimanager.lua:752: in function '_checkTasks'
       frontend/ui/uimanager.lua:1035: in function 'handleInput'
       frontend/ui/uimanager.lua:1121: in function 'run'
       ./reader.lua:260: in main chunk
       [C]: at 0x55bbe69d6782

(600x610 emulator, 12 items per page, Font size: 16)

@gerhaher

This comment has been minimized.

Copy link

gerhaher commented Oct 17, 2019

Let's see what other people think. (Pinging @gerhaher who seems to use classic mode - but with many per-page item, so used to seeing only one line.)

Yeah, as you said, I (sometimes) use classic, but then always 24 items/page, so I never see the multi-lines.

But I guess I find equal font size looks prettier.

@robert00s

This comment has been minimized.

Copy link
Contributor Author

robert00s commented Oct 17, 2019

While testing bookmarks, I got a reproducible infinite loop (I have to Ctrl-C) with just this PR:
Select 10 lines of text in a book, highlight. Go to top menu > Bookmarks:

Should be fixed now. It was problem with ending new line '\n'

Let's see what other people think. (Pinging @gerhaher who seems to use classic mode - but with many per-page item, so used to seeing only one line.)

Yep, we see only one line (with per-page=24) but we use algorithm for "multiline" so we shoudn't now see items truncated too short like in #5388. See my first and second screenshot :)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 20, 2019

@Frenzie @NiLuJe @pazos : any opinion on this change?

I don't mind it (previous algo was @robert00s too, so his choice :)
I admit it's nicer looking than before.
(The only real added value of the previous algo was that in bookmarks, the size of the font for each item was a bit of an indicator of the length of the highlighted text :)

I find some bits in this code a bit convoluted - but I don't want to request any change, as I'll probably soon have to look at TextBoxWidget, so, now that we know what kind of stuff callers expect, we can integrate a bit of that code into new TextBoxWidget methods, and clean a bit of this in Menu (just like in I did for TextWidget in #5503).

Copy link
Member

Frenzie left a comment

@poire-z The screenshots look fine to me.

end
-- remove last line and try again to fit
offset = item_name.vertical_string_list[num_lines].offset - 1
-- remove ending '\n' (new line) to prevent infinity loop

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member

Random single quotes instead of double.

-- remove last line and try again to fit
offset = item_name.vertical_string_list[num_lines].offset - 1
-- remove ending '\n' (new line) to prevent infinity loop
if item_name.charlist[offset] == '\n' then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member

Random single quotes instead of double.

if item_name.charlist[offset] == '\n' then
offset = offset - 1
end
self.text = table.concat(item_name.charlist, '', 1, offset)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member

Random single quotes instead of double.

local offset_prev = item_name_orig.vertical_string_list[num_lines - 1].offset
text_last_line = table.concat(item_name_orig.charlist, '', offset_prev, offset)

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member

Random single quotes instead of double.

local max_offset = #item_name_orig.charlist
-- try to add chars to better align
while item_name.width > text_size_increase + ellipsis_size and offset < max_offset
and item_name_orig.charlist[offset] ~= '\n' do

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member

Random single quotes instead of double.

-- when finally after manipulation we have all original text we don't need to add ellipsis
self.text = table.concat(item_name_orig.charlist, '', 1, offset)
else
-- remove ending '\n' (new line) to prevent increase number of lines

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member

Random single quotes instead of double.

self.text = table.concat(item_name_orig.charlist, '', 1, offset)
else
-- remove ending '\n' (new line) to prevent increase number of lines
if item_name_orig.charlist[offset] == '\n' then

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member

Random single quotes instead of double.

-- If we don't fit, decrease font size
self.font_size = self.font_size - 2
-- add ellipsis to show that text was truncated
self.text = table.concat(item_name_orig.charlist, '', 1, offset) .. ""

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member

Random single quotes instead of double.

item_name_orig:free()
end
--final item_name that fits
item_name = TextBoxWidget:new {

This comment has been minimized.

Copy link
@Frenzie

Frenzie Oct 20, 2019

Member
Suggested change
item_name = TextBoxWidget:new {
item_name = TextBoxWidget:new{
@poire-z poire-z merged commit c7ecd08 into koreader:master Oct 22, 2019
1 check passed
1 check passed
ci/circleci: build Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Oct 22, 2019

Testing that again, I'm still frustrated with the result :)
(Not bothered personally - its only use I care about is the TOC display, which uses the single_line=true code and is not affected by this change.)

If user want to see more text he/she can always decrease font size in items -> font size or decrease items per page

The problem with that is that if you want to see more of the long filenames/highlights, you'll get tiny font size for all items, even short ones, and for all things using our Menu class:
image
image
image
(I could be fine with that for Bookmarks, half-fine for File browser, but not at all with that TOC :)

When you raise font size, you're then switched to single line (but using the multi-lines code):
image
image
image

Raise it again, and you get something better:
image
image
image
(I guess everybody would be fine with that last one - do we all have the font size adjusted that way, or smaller like on previous set of screenshots?)

So, the multi line code switches now (very early when raising font size) to displaying a single line (so, with a static font size).
And it could as well have used the single line code then (that had no problem with oscillation).
And we could have kept the previous multi-line code for auto font sizing - and so, have it as another menu option:

Items per page >
Font size >
[x] Multi-lines (with previous auto-decreasing font size)
(or named [x] Auto-adjust font size

Then, only the first set of my screenshots above would be impossible - but would some people really want that ?!

Also, if I were not really interested in only having my TOC right, I'd be bothered that the same settings apply to different things (TOC, Bookmarks, Classic file browser), where I might want to see things laid out differently. But strangely, with the previous behaviour, this felt like not being an issue.)

Let's see if/how people feel about that.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Nov 16, 2019

I'll probably soon have to look at TextBoxWidget, so, now that we know what kind of stuff callers expect, we can integrate a bit of that code into new TextBoxWidget methods, and clean a bit of this in Menu

Done in #5509
I also resurrected the previous algorithm (that I prefered), which is available via an added menu option Items > Reduce font size to show more text.

@robert00s robert00s deleted the robert00s:menu_item_align 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
4 participants
You can’t perform that action at this time.