TextViewer: add dialog to set font size and justify text

[hius07]

---

This change is 

[poire-z]

I think you don't need a ratio (scrollToRatio would scroll to some line ratio, while your getCurrentRatio gives the ratio of a charpos among the full text length) : you could just save some current charpos, and I think there are methods in TextBoxWidget to focus on a charpos (moveCursorToCharPos, moveCursorToCharPosKeepingViewCentered...)

[poire-z]

Instead of this SpinWidget which hosts this somehow unrelated Justify toggle, this could be a popup menu (like we have in Page browser and Book map), with more features & checkboxes.

Set font size
Monospace ✔
Justify ✔

which could maybe host other features in the future (I think I once missed in TextViewer some feature available in Text editor - but I can't remember what it was :/).
Maybe with tap to apply it just here, and long-press to set it as default for TextViewer?

[poire-z]

Would sound better if named allow_set_font = true or allow_menu = true or show_menu = true.

[hius07]

[poire-z]

I initially read that as we could provide (in TextViewer callers, or via user patch) an alternative font to use as the monospace font - your comment "infont" feels like I could provide "foobarfont" or "Consolas".
But below you use self.monospace_font = not self.monospace_font, so it's actually a boolean/toggle.
So maybe rename this to use_monospace_font, or update the comment -- if true, use the "infont" monospace font

[hius07]

koreader/frontend/ui/widget/infomessage.lua

Line 50 in 13a521c

monospace_font = false,

I'll update the comment.

[poire-z]

Dunno if you witnessed some odd stuff while testing, but sure you don't need the ugly trick I had to use with this checkbox:

koreader/frontend/ui/widget/bookmapwidget.lua

Lines 1204 to 1229 in 13a521c

	text_func = function(no_size_trick)
	-- A bit tricky to update the text in the callback, as this button,
	-- being sized by ButtonTable, can't be rebuilt. We will update its
	-- text, and we want to be sure it will fit in the initial width,
	-- which may be with the checkmark or not.
	local text = _("Page browser on tap")
	if G_reader_settings:nilOrTrue("book_map_tap_to_page_browser") then
	text = text .. " \u{2713}" -- checkmark
	else
	if not no_size_trick then
	-- Initial call, make it wide enough so the checkmark text will fit
	text = text .. " \u{2003}" -- wide em space
	end
	-- Otherwise, keep it small without the checkmark, which will fit
	end
	return text
	end,
	id = "tap_to_page_browser",
	align = "left",
	callback = function()
	G_reader_settings:flipNilOrTrue("book_map_tap_to_page_browser")
	local b = button_dialog:getButtonById("tap_to_page_browser")
	b:setText(b.text_func(true), b.width)
	b:refresh()
	end,
	}},

Or is it because all such checkboxes calls :reinit(), which will rebuilt this popup menu ?
(If yes, I guess everything is rebuild and redrawn, and that the menu popup is closed as soon as you toggle/change things ? Which is a bit different than the behaviour in PageBrowser/Bookmap where we just update the content, and the popup menu stays open. But ok if it's too complicated to get this same behaviour?)

[hius07]

(I saw the trick)
Yes, the menu is closed on tap.
If we tap Font size, we see the SpinWidget (that remains open on apply) and we need to see more text to choose the size.
If we tap Monospace or Justify, we see the immediate effect on text, and what is our most frequent next action?
If it is reading, that's okay to close the menu.
If it is switching the setting back, it's better to keep it open.
If it is long-pressing to set the setting as default, it's better to keep it open.

[poire-z]

If it is switching the setting back, it's better to keep it open.

As a dev, this is my most frequent next action :)
I get your train of thoughts.
But there's also your other famous bigger train of thoughts: consistency :) (in the common behaviour of such top left menu with checkboxes).

That popup menu is small and in a corner, so it shouldn't be in the way of appreciating the change, and one tap away from closing.
But if it is much more complicated codewise to implement the "classic" behaviour, I'm ok with it.

[hius07]

Menu remains open on tapping Monospace and Justify.

[poire-z]

I see you moved the checkmark logic in Button - and that maybe we won't have to use my sizing trick in Pagebrowser and Bookmap.
But don't you also need to size it as if the checkmark was enabled (even if it is not initially) in Button?

[hius07]

Empty space is reserved with "wide em space" as you did.

[hius07]

I didn't test long texts with checkmark, I suppose the checkmark will be truncated.
Maybe someday we will rewrite the button to have the checkmark at the left, like the touchmenu items.

[poire-z]

Empty space is reserved with "wide em space" as you did.

Oh, right, didn't catch my eyes :) and " \u{2713}" or " \u{2003}").
Definitely needs some comment to tell which char these are and why we do that.

I didn't test long texts with checkmark, I suppose the checkmark will be truncated.

I guess no need to test for text longer than the screen.
Your "Monospace font" is the longest of the 3 items, so you're already in the situation to test the case I needed my trick for.

Maybe someday we will rewrite the button to have the checkmark at the left, like the touchmenu items.

Not sure I would prefer that: in such top left popup menu, where items are left aligned, there might be 3 kinds of items:

• plain test (your Font size: 20)
• text with checkmarks
• text with | - | + on the right

I think it looks better if all the text are aligned on the left. If you put the checkmark on the left for the 2nd kind, it would feel quite unaligned/ragged/odd.

[hius07]

If you put the checkmark on the left for the 2nd kind, it would feel quite unaligned/ragged/odd.

We would need a left margin for all the items (again, like in the touchmenu).
(I'm not very enthusiastic to do it right now)

[poire-z]

We would need a left margin for all the items (again, like in the touchmenu).

Yes, some indentation even when it is not needed, that may look odd, and could make that popup menu a bit wider, hiding a bit more of the content below.

It works on our TouchMenu as it takes the full width, and there's blank on the right of most items, so it may balance the blank at start. In this popup anchored at top left, it may look odd, or we may need to add the same right marging for visual niceness, making it even wider.

(I'm not very enthusiastic to do it right now)

Good ! :)

[hius07]

Do I fix your widgets here?

[poire-z]

Do I fix your widgets here?

I guess I would have liked it to result in an individual commit in master once merged (which you can't do the way you work via github web), but I guess it will be mostly code removal, so it might be ok if it all happens in one commit. So, as you feel.

Tested on my Kobo, and the checkmark and sizing look fine, and the fact that it is kept open is really practical :)
(Wondering if the font size could be a | - | + | instead of a SpinWidget - but I guess then it's harder to find out something to allow to set it as default.)

But i noticed this: if you set font size (from 20 to 16), it is displayed in 16. Then, if you toggle Justify, it stays in 16. But if you toggle Monospace font it is unexpectedly brought back to 20.

[hius07]

Here is the question: if a caller sets font_face but does not set font_size, should TextViewer use font_size from G_settings, or use the default size of font_face?

[poire-z]

The problem here is that we have a top widget (popup menu) and need to refresh the widget behind it (TextViewer).

I think that's not really it.
We have the TextViewer, not at top but as second to top (we can ignore the popup at top, it's not it that causes any issue).
You decrease the height of that TextViewer.
You need the thing below TextViewer (so, third to top :)) to repaint itself, so the little part of it shown in the few pixels that the new TextViewer does not cover get repainted.

And it's that thing below TextViewer that you would need to pass as a reference as show_parent, so only it get repainted. But if you don't have it, using "all" will do all widgets, which might do a bit more useless work, but will work.
That thing below TextViewer can be the fullscreen BookInformation widget, or another TextViewer (if viewing CSS from the ViewHTML), and I guess it can also be nothing, or seomthing else totally not accessible if the TextViewer is created from dispatcher/action/gestures...

[hius07]

we can ignore the popup at top, it's not it that causes any issue

But closing the popup menu fixes the issue without "all".

[NiLuJe]

But closing the popup menu fixes the issue without "all".

Because closing a widget marks every widget behind it as dirty, forcing a repaint ;). (The net effect being similar to "all" in the end, actually).

The stuff quoted in #11210 (comment) is tricky exactly because of that: it's the same widget instance that changes dimensions, so there's no close anywhere, hence the need for creative thinking ^^.

I'd kinda lost the plot on that distinction, but it is very much key, my bad ;).

[hius07]

"Em space" is wider than the checkmark, causes truncation, will look at it tomorrow.

[poire-z]

This "dictionary" does not impact any dictionary result I think (we don't use TextViewer for them). It would be used only for translator currently.
So, may be "lookup_result" or "lookup" ?

[hius07]

BookInfo module will get text types in the forthcoming PR introducing "set screenshot as book cover".
The rest of the modules use default "general" text type (and will be tuned after using in the field if need be).

[hius07]

Current text types:

code (16, m+, j-)
(01) ViewHtml: code
(02) ReaderTypography: "Language tags (and hyphenation dictionaries) used since start up"
(03) PatchManagement: code

lookup (20, m-, j-)
(04) Translator: translation

book_info (20, m-, j+)
(05) FilemanagerBookInfo: book description
(06) OPDSBrowser: book information

bookmark (20, m-, j+)
(07) ReaderBookmark: bookmark details

file_content (20, m-, j-)
(09) TextViewer: from Open with...
(10) ArchiveViewer: file content

general (default) (20, m-, j-)
(08) ReaderHighlight: note (small window, no menu)
(11) KeyValuePage: long value
(12) FilemanagerBookInfo: book props (except description)
(13) InputText: clipboard (small window, no menu)
(14) screen_notification_menu_table: "Past notifications"
(15) FilemanagerBookInfo: list of books with hashdocsettings

[poire-z]

Just noticed that if we launch Book description from the long-press on a file in FM, we get the text justified - but if we launch it from the Book information KeyValuePage when in Reader, we get the text non-justified.

[hius07]

if we launch it from the Book information KeyValuePage when in Reader, we get the text non-justified

It is shown as long KVP value, text_type = "general".
Must be fixed by adding entry.callback to the Description kvp item.