Show full ToC entry on hold

[NiLuJe]

Fix #6728

---

This change is 

[NiLuJe]

Open for suggestions on a better title for TextViewer, because it looks tiny and lost right now ;p.

[NiLuJe]

Now with a slightly less insanely large popup (it's a TextViewer, so, there'll be a scrollbar if needed anyway):

[poire-z]

Some issue:

May be disable justification, and increase the height ?

[NiLuJe]

@poire-z: What's the screen dimension and layout here, and was that before or after the Landscape fix? ^^

[poire-z]

610x600. Same after landscape fix.

(Somehow, using an InfoMessage would work around that: it would adjust to the height needed, and even reduce the font size if the text is too long.)

[NiLuJe]

@poire-z: an (iconless) InfoMessage looks like this:

That's perfectly fine by me, FWIW. Will it deal with LTR & stuff properly?

[NiLuJe]

diff --git a/frontend/apps/reader/modules/readertoc.lua b/frontend/apps/reader/modules/readertoc.lua
index e9551e20..494d8afa 100644
--- a/frontend/apps/reader/modules/readertoc.lua
+++ b/frontend/apps/reader/modules/readertoc.lua
@@ -7,9 +7,9 @@ local Event = require("ui/event")
 local Font = require("ui/font")
 local GestureRange = require("ui/gesturerange")
 local Geom = require("ui/geometry")
+local InfoMessage = require("ui/widget/infomessage")
 local InputContainer = require("ui/widget/container/inputcontainer")
 local Menu = require("ui/widget/menu")
-local TextViewer = require("ui/widget/textviewer")
 local UIManager = require("ui/uimanager")
 local logger = require("logger")
 local _ = require("gettext")
@@ -584,15 +584,11 @@ function ReaderToc:onShowToc()
     end
 
     function toc_menu:onMenuHold(item)
-        local textviewer = TextViewer:new{
-            title = _("Chapter title"),
+        local infomessage = InfoMessage:new{
+            show_icon = false,
             text = item.text,
-            lang = self.ui.document:getProps().language,
-            width = math.floor(self.width * 0.8),
-            height = self.height > self.width and math.floor(self.height * 0.25) or math.floor(self.height * 0.33),
-            justified = false,
         }
-        UIManager:show(textviewer)
+        UIManager:show(infomessage)
         return true
     end

[poire-z]

Works for me:

May be not with RTL (as it uses the language of the UI I guess):

But our TOC is already not following the book language, and sticking to UI layout-direction.

[poire-z]

We could pass a few more params to InfoMessage that it would pass to the final TextBoxWidget, like TextViewer does:

--- a/frontend/apps/reader/modules/readertoc.lua
+++ b/frontend/apps/reader/modules/readertoc.lua
@@ -9,2 +9,3 @@ local GestureRange = require("ui/gesturerange")
 local Geom = require("ui/geometry")
+local InfoMessage = require("ui/widget/infomessage")
 local InputContainer = require("ui/widget/container/inputcontainer")
@@ -584,2 +585,13 @@ function ReaderToc:onShowToc()

+    function toc_menu:onMenuHold(item)
+        local infomessage = InfoMessage:new{
+            show_icon = false,
+            text = item.text,
+            lang = self.ui.document:getProps().language,
+            auto_para_direction = true,
+        }
+        UIManager:show(infomessage)
+        return true
+    end
+
     toc_menu.close_callback = function()
--- a/frontend/ui/widget/infomessage.lua
+++ b/frontend/ui/widget/infomessage.lua
@@ -62,2 +62,7 @@ local InfoMessage = InputContainer:new{
     dismiss_callback = function() end,
+    -- In case we'd like to use it to display some text we know a few more things about:
+    lang = nil,
+    para_direction_rtl = nil,
+    auto_para_direction = nil,
+
 }
@@ -125,2 +130,5 @@ function InfoMessage:init()
             dialog = self,
+            lang = self.lang,
+            para_direction_rtl = self.para_direction_rtl,
+            auto_para_direction = self.auto_para_direction,
         }
@@ -131,2 +139,5 @@ function InfoMessage:init()
             width = text_width,
+            lang = self.lang,
+            para_direction_rtl = self.para_direction_rtl,
+            auto_para_direction = self.auto_para_direction,
         }

and we can get:

We get some (unwanted?) indentation for sub chapters:

Should we remove that indentation?

[poire-z]

Got worried with the smaller font size for the arabic bold 5.2 above...
But it's fine normally:

I just get that kind of glitch if I resize the window (so, that may be considered a bug, but I'll pretend I never witnessed nor mentionned it :)

[poire-z]

^^but may be let that for the day we fix the TOC layout for when book direction is different than UI direction as it might feel strange to have the difference like on the screenshot above.

[NiLuJe]

^^but may be let that for the day we fix the TOC layout for when book direction is different than UI direction as it might feel strange to have the difference like on the screenshot above.

Yeah, right now it'd feel somewhat confusing if the popup was laid out differently than the ToC page ;).

Good to know, though :).

[hius07]

Should we remove that indentation?

Yes, I believe.

[poire-z]

I just get that kind of glitch if I resize the window (so, that may be considered a bug, but I'll pretend I never witnessed nor mentionned it :)

OK, that was my bug, so I'll fix it :/ Something to do with size = Screen:scaleBySize(orig_size), and orig_size being stored in the font object (so we can re-use it to fetch a bold font or a smaller font). We'll need to update it when size/dpi change.

[NiLuJe]

And took care of trimming toc_indent ;).

[NiLuJe]

@poire-z: Since I can never remember what the return actually does for those, is returning true in the MenuHold handler the right thing to do, here?

[poire-z]

I guess it's fine.
return true => stop propagation to other less-priority onHold handlers.
I don't think there's any in that context (unless the document under the TOC window would get it an do a dict lookup on the word at that position ? :) might try it just so we know it could propagate up to there.

But anyway, as we don't know about other valuable handler in this context, and that you always do something (showing the InfoMessage), return true is the right thing to do.

[poire-z]

(I think there might be more bits of code that use gsub(...%S...) to do some trim() and that could be migrated to your new util.*trim() - but may be keep that for a later PR :) (and/or remove the statistics.koplugin one from here to have them all in a dedicated commit?)

(May be not many:
readerstyletweak.lua, generic/device.lua looks updatable
readerhighlight.lua and readerdictionary.lua do trim as part of other stuff, so keeping the gsub there with the other gsub might be clearer.)

[Frenzie]

The comments here are slightly weird from an ldoc perspective. ;-)

[Frenzie]

lgtm, but ideally the comments on the new util functions would be a bit better ;-)

[NiLuJe]

@Frenzie: Tweaked it a bit ;).

@poire-z: Done, thanks ;). I'm not quite sure what exactly the SSID one was attempting to do in the first place, but it wasn't quite a trim, but, oh, well ;).