Revamp "flash_ui" handling (#7118)

* Wherever possible, do an actual dumb invert on the Screen BB instead of repainting the widget, *then* inverting it (which is what the "invert" flag does).
* Instead of playing with nextTick/tickAfterNext delays, explicitly fence stuff with forceRePaint
* And, in the few cases where said Mk. 7 quirk kicks in, make the fences more marked by using a well-placed WAIT_FOR_UPDATE_COMPLETE

* Fix an issue in Button where show/hide & enable/disable where actually all toggles, which meant that duplicate calls or timing issues would do the wrong thing. (This broke dimming some icons, and mistakenly dropped the background from FM chevrons, for example).
* Speaking of, fix Button's hide/show to actually restore the background properly (there was a stupid typo in the variable name)
* Still in Button, fix the insanity of the double repaint on rounded buttons. Turns out it made sense, after all (and was related to said missing background, and bad interaction with invert & text with no background).
* KeyValuePage suffered from a similar issue with broken highlights (all black) because of missing backgrounds.
* In ConfigDialog, only instanciate IconButtons once (because every tab switch causes a full instantiation; and the initial display implies a full instanciation and an initial tab switch). Otherwise, both instances linger, and catch taps, and as such, double highlights.
* ConfigDialog: Restore the "don't repaint ReaderUI" when switching between similarly sized tabs (re #6131). I never could reproduce that on eInk, and I can't now on the emulator, so I'm assuming @poire-z fixed it during the swap to SVG icons.
* KeyValuePage: Only instanciate Buttons once (again, this is a widget that goes through a full init every page). Again, caused highlight/dimming issues because buttons were stacked.
* Menu: Ditto.
* TouchMenu: Now home of the gnarliest unhilight heuristics, because of the sheer amount of different things that can happen (and/or thanks to stuff not flagged covers_fullscreen properly ;p).

* Bump base
koreader/koreader-base#1280
koreader/koreader-base#1282
koreader/koreader-base#1283
koreader/koreader-base#1284

* Bump android-luajit-launcher
koreader/android-luajit-launcher#284
koreader/android-luajit-launcher#285
koreader/android-luajit-launcher#286
koreader/android-luajit-launcher#287

frontend/apps/reader/skimtowidget.lua

@@ -123,6 +123,7 @@ function SkimToWidget:init()
         enabled = true,
         width = self.button_width,
         show_parent = self,
+        vsync = true,
         callback = function()
             self:goToPage(self.curr_page - 1)
         end,
@@ -135,6 +136,7 @@ function SkimToWidget:init()
         enabled = true,
         width = self.button_width,
         show_parent = self,
+        vsync = true,
         callback = function()
             self:goToPage(self.curr_page - 10)
         end,
@@ -147,6 +149,7 @@ function SkimToWidget:init()
         enabled = true,
         width = self.button_width,
         show_parent = self,
+        vsync = true,
         callback = function()
             self:goToPage(self.curr_page + 1)
         end,
@@ -159,6 +162,7 @@ function SkimToWidget:init()
         enabled = true,
         width = self.button_width,
         show_parent = self,
+        vsync = true,
         callback = function()
             self:goToPage(self.curr_page + 10)
         end,
@@ -193,6 +197,7 @@ function SkimToWidget:init()
         enabled = true,
         width = self.button_width,
         show_parent = self,
+        vsync = not G_reader_settings:isTrue("refresh_on_chapter_boundaries"),
         callback = function()
             local page = self.ui.toc:getNextChapter(self.curr_page)
             if page and page >=1 and page <= self.page_count then
@@ -212,6 +217,7 @@ function SkimToWidget:init()
         enabled = true,
         width = self.button_width,
         show_parent = self,
+        vsync = not G_reader_settings:isTrue("refresh_on_chapter_boundaries"),
         callback = function()
             local page = self.ui.toc:getPreviousChapter(self.curr_page)
             if page and page >=1 and page <= self.page_count then
@@ -231,6 +237,7 @@ function SkimToWidget:init()
         enabled = true,
         width = self.button_width,
         show_parent = self,
+        vsync = true,
         callback = function()
             self:goToByEvent("GotoNextBookmarkFromPage")
         end,
@@ -248,6 +255,7 @@ function SkimToWidget:init()
         enabled = true,
         width = self.button_width,
         show_parent = self,
+        vsync = true,
         callback = function()
             self:goToByEvent("GotoPreviousBookmarkFromPage")
         end,

frontend/ui/data/koptoptions.lua

@@ -320,7 +320,6 @@ The first option ("auto") tries to automatically align reflowed text as it is in
                 item_text = tableOfNumbersToTableOfStrings(FONT_SCALE_FACTORS),
                 item_align_center = 1.0,
                 spacing = 15,
-                height = 60,
                 item_font_size = FONT_SCALE_DISPLAY_SIZE,
                 args = FONT_SCALE_FACTORS,
                 values = FONT_SCALE_FACTORS,

frontend/ui/uimanager.lua

@@ -7,7 +7,8 @@ local Event = require("ui/event")
 local Geom = require("ui/geometry")
 local dbg = require("dbg")
 local logger = require("logger")
-local util = require("ffi/util")
+local ffiUtil = require("ffi/util")
+local util = require("util")
 local _ = require("gettext")
 local Input = Device.input
 local Screen = Device.screen
@@ -528,7 +529,7 @@ dbg:guard(UIManager, 'schedule',
 
 --- Schedules task in a certain amount of seconds (fractions allowed) from now.
 function UIManager:scheduleIn(seconds, action, ...)
-    local when = { util.gettime() }
+    local when = { ffiUtil.gettime() }
     local s = math.floor(seconds)
     local usecs = (seconds - s) * MILLION
     when[1] = when[1] + s
@@ -770,9 +771,42 @@ function UIManager:ToggleNightMode(night_mode)
     end
 end
 
---- Get top widget.
+--- Get top widget (name if possible, ref otherwise).
 function UIManager:getTopWidget()
-    return ((self._window_stack[#self._window_stack] or {}).widget or {}).name
+    local top = self._window_stack[#self._window_stack]
+    if not top or not top.widget then
+        return nil
+    end
+    if top.widget.name then
+        return top.widget.name
+    end
+    return top.widget
+end
+
+--- Check if a widget is still in the window stack, or is a subwidget of a widget still in the window stack
+function UIManager:isSubwidgetShown(widget, max_depth)
+    for i = #self._window_stack, 1, -1 do
+        local matched, depth = util.arrayReferences(self._window_stack[i].widget, widget, max_depth)
+        if matched then
+            return matched, depth, self._window_stack[i].widget
+        end
+    end
+    return false
+end
+
+--- Same, but only check window-level widgets (e.g., what's directly registered in the window stack), don't recurse
+function UIManager:isWidgetShown(widget)
+    for i = #self._window_stack, 1, -1 do
+        if self._window_stack[i].widget == widget then
+           return true
+        end
+    end
+    return false
+end
+
+-- Returns the region of the previous refresh
+function UIManager:getPreviousRefreshRegion()
+   return self._last_refresh_region
 end
 
 --- Signals to quit.
@@ -822,7 +856,7 @@ function UIManager:discardEvents(set_or_seconds)
     else -- we expect a number
         usecs = set_or_seconds * MILLION
     end
-    local now = { util.gettime() }
+    local now = { ffiUtil.gettime() }
     local now_us = now[1] * MILLION + now[2]
     self._discard_events_till = now_us + usecs
 end
@@ -833,7 +867,7 @@ function UIManager:sendEvent(event)
 
     -- Ensure discardEvents
     if self._discard_events_till then
-        local now = { util.gettime() }
+        local now = { ffiUtil.gettime() }
         local now_us = now[1] * MILLION + now[2]
         if now_us < self._discard_events_till then
             return
@@ -897,7 +931,7 @@ function UIManager:broadcastEvent(event)
 end
 
 function UIManager:_checkTasks()
-    local now = { util.gettime() }
+    local now = { ffiUtil.gettime() }
     local now_us = now[1] * MILLION + now[2]
     local wait_until = nil
 
@@ -1193,6 +1227,8 @@ function UIManager:_repaint()
             refresh.region.w = ALIGN_UP(refresh.region.w + (x_fixup * 2), 8)
             refresh.region.h = ALIGN_UP(refresh.region.h + (y_fixup * 2), 8)
         end
+        -- Remember the refresh region
+        self._last_refresh_region = refresh.region
         Screen[refresh_methods[refresh.mode]](Screen,
             refresh.region.x, refresh.region.y,
             refresh.region.w, refresh.region.h,
@@ -1212,6 +1248,10 @@ function UIManager:forceRePaint()
     self:_repaint()
 end
 
+function UIManager:waitForVSync()
+    Screen:refreshWaitForLast()
+end
+
 -- Used to repaint a specific sub-widget that isn't on the _window_stack itself
 -- Useful to avoid repainting a complex widget when we just want to invert an icon, for instance.
 -- No safety checks on x & y *by design*. I want this to blow up if used wrong.
@@ -1222,6 +1262,14 @@ function UIManager:widgetRepaint(widget, x, y)
     widget:paintTo(Screen.bb, x, y)
 end
 
+-- Same idea, but does a simple invertRect, without actually repainting anything
+function UIManager:widgetInvert(widget, x, y, w, h)
+    if not widget then return end
+
+    logger.dbg("Explicit widgetInvert:", widget.name or widget.id or tostring(widget), "@ (", x, ",", y, ")")
+    Screen.bb:invertRect(x, y, w or widget.dimen.w, h or widget.dimen.h)
+end
+
 function UIManager:setInputTimeout(timeout)
     self.INPUT_TIMEOUT = timeout or 200*1000
 end

frontend/ui/widget/button.lua

@@ -40,6 +40,7 @@ local Button = InputContainer:new{
     preselect = false,
     callback = nil,
     enabled = true,
+    hidden = false,
     allow_hold_when_disabled = false,
     margin = 0,
     bordersize = Size.border.button,
@@ -53,6 +54,7 @@ local Button = InputContainer:new{
     text_font_face = "cfont",
     text_font_size = 20,
     text_font_bold = true,
+    vsync = nil, -- when "flash_ui" is enabled, allow bundling the highlight with the callback, and fence that batch away from the unhighlight. Avoid delays when callback requires a "partial" on Kobo Mk. 7, c.f., ffi/framebuffer_mxcfb for more details.
 }
 
 function Button:init()
@@ -164,28 +166,26 @@ function Button:onUnfocus()
 end
 
 function Button:enable()
-    self.enabled = true
-    if self.text then
-        if self.enabled then
+    if not self.enabled then
+        if self.text then
             self.label_widget.fgcolor = Blitbuffer.COLOR_BLACK
+            self.enabled = true
         else
-            self.label_widget.fgcolor = Blitbuffer.COLOR_DARK_GRAY
+            self.label_widget.dim = false
+            self.enabled = true
         end
-    else
-        self.label_widget.dim = not self.enabled
     end
 end
 
 function Button:disable()
-    self.enabled = false
-    if self.text then
-        if self.enabled then
-            self.label_widget.fgcolor = Blitbuffer.COLOR_BLACK
-        else
+    if self.enabled then
+        if self.text then
             self.label_widget.fgcolor = Blitbuffer.COLOR_DARK_GRAY
+            self.enabled = false
+        else
+            self.label_widget.dim = true
+            self.enabled = false
         end
-    else
-        self.label_widget.dim = not self.enabled
     end
 end
 
@@ -198,17 +198,19 @@ function Button:enableDisable(enable)
 end
 
 function Button:hide()
-    if self.icon then
-        self.frame.orig_background = self[1].background
+    if self.icon and not self.hidden then
+        self.frame.orig_background = self.frame.background
         self.frame.background = nil
         self.label_widget.hide = true
+        self.hidden = true
     end
 end
 
 function Button:show()
-    if self.icon then
+    if self.icon and self.hidden then
         self.label_widget.hide = false
-        self.frame.background = self[1].old_background
+        self.frame.background = self.frame.orig_background
+        self.hidden = false
     end
 end
 
@@ -225,42 +227,74 @@ function Button:onTapSelectButton()
         if G_reader_settings:isFalse("flash_ui") then
             self.callback()
         else
-            -- For most of our buttons, we can't avoid that initial repaint...
-            self[1].invert = true
-            UIManager:widgetRepaint(self[1], self[1].dimen.x, self[1].dimen.y)
-            -- NOTE: This completely insane double repaint is needed to avoid cosmetic issues with FrameContainer's rounded corners on Text buttons...
-            --       On the upside, we now actually get to *see* those rounded corners (as the highlight), where it was a simple square before.
-            --       c.f., #4554 & #4541
             -- NOTE: self[1] -> self.frame, if you're confused about what this does vs. onFocus/onUnfocus ;).
             if self.text then
                 -- We only want the button's *highlight* to have rounded corners (otherwise they're redundant, same color as the bg).
                 -- The nil check is to discriminate the default from callers that explicitly request a specific radius.
                 if self[1].radius == nil then
                     self[1].radius = Size.radius.button
+                    -- And here, it's easier to just invert the bg/fg colors ourselves,
+                    -- so as to preserve the rounded corners in one step.
+                    self[1].background = self[1].background:invert()
+                    self.label_widget.fgcolor = self.label_widget.fgcolor:invert()
+                    -- We do *NOT* set the invert flag, because it just adds an invertRect step at the end of the paintTo process,
+                    -- and we've already taken care of inversion in a way that won't mangle the rounded corners.
+                else
+                    self[1].invert = true
                 end
+
                 UIManager:widgetRepaint(self[1], self[1].dimen.x, self[1].dimen.y)
+                -- But do make sure the invert flag is set in both cases, mainly for the early return check below
+                self[1].invert = true
+            else
+                self[1].invert = true
+                UIManager:widgetInvert(self[1], self[1].dimen.x, self[1].dimen.y)
             end
             UIManager:setDirty(nil, function()
                 return "fast", self[1].dimen
             end)
-            -- And we also often have to delay the callback to both see the flash and/or avoid tearing artefacts w/ fast refreshes...
-            UIManager:tickAfterNext(function()
-                self.callback()
-                if not self[1] or not self[1].invert or not self[1].dimen then
-                    -- widget no more there (destroyed, re-init'ed by setText(), or not inverted: nothing to invert back
-                    return
-                end
 
-                self[1].invert = false
-                -- Since we kill the corners, we only need a single repaint.
+            -- Force the repaint *now*, so we don't have to delay the callback to see the highlight...
+            if not self.vsync then
+                -- NOTE: Allow bundling the highlight with the callback when we request vsync, to prevent further delays
+                UIManager:forceRePaint() -- Ensures we have a chance to see the highlight
+            end
+            self.callback()
+            UIManager:forceRePaint() -- Ensures whatever the callback wanted to paint will be shown *now*...
+            if self.vsync then
+                -- NOTE: This is mainly useful when the callback caused a REAGL update that we do not explicitly fence already,
+                --       (i.e., Kobo Mk. 7).
+                UIManager:waitForVSync() -- ...and that the EPDC will not wait to coalesce it with the *next* update,
+                                         -- because that would have a chance to noticeably delay it until the unhighlight.
+            end
+
+            if not self[1] or not self[1].invert or not self[1].dimen then
+                -- If the widget no longer exists (destroyed, re-init'ed by setText(), or not inverted: nothing to invert back
+                return true
+            end
+
+            -- If the callback closed our parent (which ought to have been the top level widget), abort early
+            if UIManager:getTopWidget() ~= self.show_parent then
+                return true
+            end
+
+            self[1].invert = false
+            if self.text then
                 if self[1].radius == Size.radius.button then
                     self[1].radius = nil
+                    self[1].background = self[1].background:invert()
+                    self.label_widget.fgcolor = self.label_widget.fgcolor:invert()
                 end
+
                 UIManager:widgetRepaint(self[1], self[1].dimen.x, self[1].dimen.y)
-                UIManager:setDirty(nil, function()
-                    return "fast", self[1].dimen
-                end)
+            else
+                UIManager:widgetInvert(self[1], self[1].dimen.x, self[1].dimen.y)
+            end
+            -- If the button was disabled, switch to UI to make sure the gray comes through unharmed ;).
+            UIManager:setDirty(nil, function()
+                return self.enabled and "fast" or "ui", self[1].dimen
             end)
+            --UIManager:forceRePaint() -- Ensures the unhighlight happens now, instead of potentially waiting and having it batched with something else.
         end
     elseif self.tap_input then
         self:onInput(self.tap_input)