ReaderUI: Saner FM/RD lifecycle

* Ensure that going from one to the other tears down the former and
    its plugins before instantiating the latter and its plugins.

UIManager: Unify Event sending & broadcasting
  * Make the two behave the same way (walk the widget stack from top to
    bottom), and properly handle the window stack shrinking shrinking
    *and* growing.
    Previously, broadcasting happened bottom-to-top and didn't really
    handle the list shrinking/growing, while sending only handled the list
    shrinking by a single element, and hopefully that element being the one
    the event was just sent to.

These two items combined allowed us to optimize suboptimal
refresh behavior with Menu and other Menu classes when
opening/closing a document.
e.g., the "opening document" Notification is now properly regional,
and the "open last doc" option no longer flashes like a crazy person
anymore.

Plugins: Allow optimizing Menu refresh with custom menus, too.

Requires moving Menu's close_callback *after* onMenuSelect, which, eh,
probably makes sense, and is probably harmless in the grand scheme of
things.
reviewable/pr7635/r1
NiLuJe 3 years ago
parent 47cc7bb1a6
commit e7acec1526

@ -21,7 +21,7 @@ function DropBox:showFiles(url, password)
return DropBoxApi:showFiles(url, password)
end
function DropBox:downloadFile(item, password, path, close)
function DropBox:downloadFile(item, password, path, callback_close)
local code_response = DropBoxApi:downloadFile(item.url, password, path)
if code_response == 200 then
local __, filename = util.splitFilePathName(path)
@ -34,7 +34,13 @@ function DropBox:downloadFile(item, password, path, close)
text = T(_("File saved to:\n%1\nWould you like to read the downloaded book now?"),
BD.filepath(path)),
ok_callback = function()
close()
local Event = require("ui/event")
UIManager:broadcastEvent(Event:new("SetupShowReader"))
if callback_close then
callback_close()
end
ReaderUI:showReader(path)
end
})

@ -20,7 +20,7 @@ function Ftp:run(address, user, pass, path)
return FtpApi:listFolder(url, path)
end
function Ftp:downloadFile(item, address, user, pass, path, close)
function Ftp:downloadFile(item, address, user, pass, path, callback_close)
local url = FtpApi:generateUrl(address, util.urlEncode(user), util.urlEncode(pass)) .. item.url
logger.dbg("downloadFile url", url)
path = util.fixUtf8(path, "_")
@ -43,7 +43,13 @@ function Ftp:downloadFile(item, address, user, pass, path, close)
text = T(_("File saved to:\n%1\nWould you like to read the downloaded book now?"),
BD.filepath(path)),
ok_callback = function()
close()
local Event = require("ui/event")
UIManager:broadcastEvent(Event:new("SetupShowReader"))
if callback_close then
callback_close()
end
ReaderUI:showReader(path)
end
})

@ -17,7 +17,7 @@ function WebDav:run(address, user, pass, path)
return WebDavApi:listFolder(address, user, pass, path)
end
function WebDav:downloadFile(item, address, username, password, local_path, close)
function WebDav:downloadFile(item, address, username, password, local_path, callback_close)
local code_response = WebDavApi:downloadFile(address .. WebDavApi:urlEncode( item.url ), username, password, local_path)
if code_response == 200 then
local __, filename = util.splitFilePathName(local_path)
@ -30,7 +30,13 @@ function WebDav:downloadFile(item, address, username, password, local_path, clos
text = T(_("File saved to:\n%1\nWould you like to read the downloaded book now?"),
BD.filepath(local_path)),
ok_callback = function()
close()
local Event = require("ui/event")
UIManager:broadcastEvent(Event:new("SetupShowReader"))
if callback_close then
callback_close()
end
ReaderUI:showReader(local_path)
end
})

@ -222,7 +222,6 @@ function FileManager:setupLayout()
end
function file_chooser:onFileSelect(file) -- luacheck: ignore
FileManager.instance:onClose()
ReaderUI:showReader(file)
return true
end
@ -507,6 +506,7 @@ function FileManager:setupLayout()
end
end
-- NOTE: The only thing that will *ever* instantiate a new FileManager object is our very own showFiles below!
function FileManager:init()
self:setupLayout()
@ -781,6 +781,21 @@ function FileManager:onClose()
return true
end
function FileManager:onShowingReader()
-- Allows us to optimize out a few useless refreshes in various CloseWidgets handlers...
self.tearing_down = true
-- Clear the dither flag to prevent it from infecting the queue and re-inserting a full-screen refresh...
self.dithered = nil
self:onClose()
end
-- Same as above, except we don't close it yet. Useful for plugins that need to close custom Menus before calling showReader.
function FileManager:onSetupShowReader()
self.tearing_down = true
self.dithered = nil
end
function FileManager:onRefresh()
self.file_chooser:refreshPath()
return true
@ -826,9 +841,7 @@ function FileManager:openRandomFile(dir)
text = T(_("Do you want to open %1?"), BD.filename(BaseUtil.basename(random_file))),
choice1_text = _("Open"),
choice1_callback = function()
FileManager.instance:onClose()
ReaderUI:showReader(random_file)
end,
-- @translators Another file. This is a button on the open random file dialog. It presents a file with the choices Open/Another.
choice2_text = _("Another"),
@ -1152,6 +1165,10 @@ function FileManager:showFiles(path, focused_file)
end
}
UIManager:show(file_manager)
-- NOTE: This is a bit clunky. This ought to be private and accessed via a getCurrentInstance method, àla ReaderUI.
-- But, it points to the *current* FM instance, and is nil'ed on exit.
-- As such, code outside of FileManager can just check/use FileManager.instance (which they do. extensively).
self.instance = file_manager
end

@ -166,12 +166,7 @@ function FileManagerCollection:onShowColl(collection)
self:updateItemTable()
self.coll_menu.close_callback = function()
-- Close it at next tick so it stays displayed
-- while a book is opening (avoids a transient
-- display of the underlying File Browser)
UIManager:nextTick(function()
UIManager:close(self.coll_menu)
end)
UIManager:close(self.coll_menu)
end
UIManager:show(self.coll_menu)
return true

@ -42,10 +42,24 @@ function FileSearcher:readDir()
-- Don't traverse hidden folders if we're not showing them
if attributes.mode == "directory" and f ~= "." and f ~= ".." and (G_reader_settings:isTrue("show_hidden") or not util.stringStartsWith(f, ".")) then
table.insert(new_dirs, fullpath)
table.insert(self.files, {name = f, text = f.."/", attr = attributes, callback = function() FileManager:showFiles(fullpath) end})
table.insert(self.files, {
name = f,
text = f.."/",
attr = attributes,
callback = function()
FileManager:showFiles(fullpath)
end,
})
-- Always ignore macOS resource forks, too.
elseif attributes.mode == "file" and not util.stringStartsWith(f, "._") and (show_unsupported or DocumentRegistry:hasProvider(fullpath)) then
table.insert(self.files, {name = f, text = f, attr = attributes, callback = function() ReaderUI:showReader(fullpath) end})
table.insert(self.files, {
name = f,
text = f,
attr = attributes,
callback = function()
ReaderUI:showReader(fullpath)
end,
})
end
end
end

@ -155,12 +155,7 @@ function FileManagerHistory:onShowHist()
self:updateItemTable()
self.hist_menu.close_callback = function()
-- Close it at next tick so it stays displayed
-- while a book is opening (avoids a transient
-- display of the underlying File Browser)
UIManager:nextTick(function()
UIManager:close(self.hist_menu)
end)
UIManager:close(self.hist_menu)
end
UIManager:show(self.hist_menu)
return true

@ -122,13 +122,17 @@ function FileManagerMenu:onOpenLastDoc()
})
return
end
local ReaderUI = require("apps/reader/readerui")
ReaderUI:showReader(last_file)
-- only close menu if we were called from the menu
-- Only close menu if we were called from the menu
if self.menu_container then
-- Mimic's FileManager's onShowingReader refresh optimizations
self.ui.tearing_down = true
self.ui.dithered = nil
self:onCloseFileManagerMenu()
end
local ReaderUI = require("apps/reader/readerui")
ReaderUI:showReader(last_file)
end
function FileManagerMenu:setUpdateItemTable()
@ -784,7 +788,7 @@ function FileManagerMenu:onShowMenu(tab_index)
}
end
main_menu.close_callback = function ()
main_menu.close_callback = function()
self:onCloseFileManagerMenu()
end

@ -188,7 +188,7 @@ function ReaderFont:onShowFontMenu()
dimen = Screen:getSize(),
main_menu,
}
main_menu.close_callback = function ()
main_menu.close_callback = function()
UIManager:close(menu_container)
end
-- show menu

@ -376,7 +376,7 @@ function ReaderMenu:onShowMenu(tab_index)
}
end
main_menu.close_callback = function ()
main_menu.close_callback = function()
self.ui:handleEvent(Event:new("CloseReaderMenu"))
end

@ -480,6 +480,21 @@ function ReaderUI:showFileManager(file)
end
end
function ReaderUI:onShowingReader()
-- Allows us to optimize out a few useless refreshes in various CloseWidgets handlers...
self.tearing_down = true
self.dithered = nil
self:onClose()
end
-- Same as above, except we don't close it yet. Useful for plugins that need to close custom Menus before calling showReader.
function ReaderUI:onSetupShowReader()
self.tearing_down = true
self.dithered = nil
end
--- @note: Will sanely close existing FileManager/ReaderUI instance for you!
function ReaderUI:showReader(file, provider)
logger.dbg("show reader ui")
@ -497,6 +512,10 @@ function ReaderUI:showReader(file, provider)
self:showFileManager(file)
return
end
-- We can now signal the existing ReaderUI/FileManager instances that it's time to go bye-bye...
UIManager:broadcastEvent(Event:new("ShowingReader"))
-- prevent crash due to incompatible bookmarks
--- @todo Split bookmarks from metadata and do per-engine in conversion.
provider = provider or DocumentRegistry:getProvider(file)
@ -550,7 +569,7 @@ end
local _running_instance = nil
function ReaderUI:doShowReader(file, provider)
logger.info("opening file", file)
-- keep only one instance running
-- Keep only one instance running
if _running_instance then
_running_instance:onClose()
end
@ -591,12 +610,17 @@ function ReaderUI:doShowReader(file, provider)
end
Device:notifyBookState(title, document)
UIManager:show(reader)
_running_instance = reader
-- This is mostly for the few callers that bypass the coroutine shenanigans and call doShowReader directly,
-- instead of showReader...
-- Otherwise, showReader will have taken care of that *before* instantiating a new RD,
-- in order to ensure a sane ordering of plugins teardown -> instantiation.
local FileManager = require("apps/filemanager/filemanager")
if FileManager.instance then
FileManager.instance:onClose()
end
UIManager:show(reader)
_running_instance = reader
end
function ReaderUI:_getRunningInstance()
@ -756,6 +780,11 @@ end
function ReaderUI:reloadDocument(after_close_callback)
local file = self.document.file
local provider = getmetatable(self.document).__index
-- Mimic onShowingReader's refresh optimizations
self.tearing_down = true
self.dithered = nil
self:handleEvent(Event:new("CloseReaderMenu"))
self:handleEvent(Event:new("CloseConfigMenu"))
self.highlight:onClose() -- close highlight dialog if any
@ -764,15 +793,22 @@ function ReaderUI:reloadDocument(after_close_callback)
-- allow caller to do stuff between close an re-open
after_close_callback(file, provider)
end
self:showReader(file, provider)
end
function ReaderUI:switchDocument(new_file)
if not new_file then return end
-- Mimic onShowingReader's refresh optimizations
self.tearing_down = true
self.dithered = nil
self:handleEvent(Event:new("CloseReaderMenu"))
self:handleEvent(Event:new("CloseConfigMenu"))
self.highlight:onClose() -- close highlight dialog if any
self:onClose(false)
self:showReader(new_file)
end

@ -14,6 +14,11 @@ local ReadHistory = {
last_read_time = 0,
}
local function selectCallback(path)
local ReaderUI = require("apps/reader/readerui")
ReaderUI:showReader(path)
end
local function buildEntry(input_time, input_file)
local file_path = realpath(input_file) or input_file -- keep orig file path of deleted files
local file_exists = lfs.attributes(file_path, "mode") == "file"
@ -43,8 +48,7 @@ local function buildEntry(input_time, input_file)
return lfs.attributes(file_path, "mode") == "file"
end,
callback = function()
local ReaderUI = require("apps/reader/readerui")
ReaderUI:showReader(input_file)
selectCallback(input_file)
end
}
end
@ -245,8 +249,7 @@ function ReadHistory:updateItemByPath(old_path, new_path)
self.hist[i].text = new_path:gsub(".*/", "")
self:_flush()
self.hist[i].callback = function()
local ReaderUI = require("apps/reader/readerui")
ReaderUI:showReader(new_path)
selectCallback(new_path)
end
break
end

@ -1076,7 +1076,10 @@ function UIManager:discardEvents(set_or_seconds)
end
--[[--
Transmits an @{ui.event.Event|Event} to active widgets.
Transmits an @{ui.event.Event|Event} to active widgets, top to bottom.
Stops at the first handler that returns `true`.
Note that most complex widgets are based on @{ui.widget.container.WidgetContainer|WidgetContainer},
which itself will take care of propagating an event to its members.
@param event an @{ui.event.Event|Event} object
]]
@ -1113,26 +1116,34 @@ function UIManager:sendEvent(event)
end
end
-- if the event is not consumed, active widgets (from top to bottom) can access it.
-- NOTE: _window_stack can shrink when widgets are closed (CloseWidget & Close events).
-- If the event was not consumed (no handler returned true), active widgets (from top to bottom) can access it.
-- NOTE: _window_stack can shrink/grow when widgets are closed (CloseWidget & Close events) or opened.
-- Simply looping in reverse would only cover the list shrinking, and that only by a *single* element,
-- something we can't really guarantee, hence the more dogged iterator below,
-- which relies on a hash check of already processed widgets (LuaJIT actually hashes the table's GC reference),
-- rather than a simple loop counter, and will in fact iterate *at least* #items ^ 2 times.
-- Thankfully, that list should be very small, so the overhead should be minimal.
local checked_widgets = {top_widget}
for i = #self._window_stack, 1, -1 do
local i = #self._window_stack
while i > 0 do
local widget = self._window_stack[i]
if checked_widgets[widget] == nil then
-- active widgets has precedence to handle this event
checked_widgets[widget] = true
-- Widget's active widgets have precedence to handle this event
-- NOTE: While FileManager only has a single (screenshotter), ReaderUI has many active_widgets (each ReaderUI module gets added to the list).
if widget.widget.active_widgets then
checked_widgets[widget] = true
for _, active_widget in ipairs(widget.widget.active_widgets) do
if active_widget:handleEvent(event) then return end
end
end
if widget.widget.is_always_active then
-- active widgets will handle this event
-- Widget itself is flagged always active, let it handle the event
-- NOTE: is_always_active widgets currently are widgets that want to show a VirtualKeyboard or listen to Dispatcher events
checked_widgets[widget] = true
if widget.widget:handleEvent(event) then return end
end
i = #self._window_stack
else
i = i - 1
end
end
end
@ -1143,18 +1154,18 @@ Transmits an @{ui.event.Event|Event} to all registered widgets.
@param event an @{ui.event.Event|Event} object
]]
function UIManager:broadcastEvent(event)
-- the widget's event handler might close widgets in which case
-- a simple iterator like ipairs would skip over some entries
local i = 1
while i <= #self._window_stack do
local prev_widget = self._window_stack[i].widget
self._window_stack[i].widget:handleEvent(event)
local top_widget = self._window_stack[i]
if top_widget == nil then
-- top widget closed itself
break
elseif top_widget.widget == prev_widget then
i = i + 1
-- Unlike sendEvent, we send the event to *all* (window-level) widgets (i.e., we don't stop, even if a handler returns true).
-- NOTE: Same defensive approach to _window_stack changing from under our feet as above.
local checked_widgets = {}
local i = #self._window_stack
while i > 0 do
local widget = self._window_stack[i]
if checked_widgets[widget] == nil then
checked_widgets[widget] = true
widget.widget:handleEvent(event)
i = #self._window_stack
else
i = i - 1
end
end
end

@ -392,6 +392,8 @@ function DictQuickLookup:init()
self:onHoldClose(true)
-- close current ReaderUI in 1 sec, and create a new one
UIManager:scheduleIn(1.0, function()
UIManager:broadcastEvent(Event:new("SetupShowReader"))
if self.ui then
-- close Highlight menu if any still shown
if self.ui.highlight and self.ui.highlight.highlight_dialog then
@ -399,6 +401,7 @@ function DictQuickLookup:init()
end
self.ui:onClose()
end
self.ui:showReader(epub_path)
end)
end,

@ -540,7 +540,6 @@ function FileChooser:showSetProviderButtons(file, filemanager_instance, reader_u
ok_callback = function()
DocumentRegistry:setProvider(file, provider, false)
filemanager_instance:onClose()
reader_ui:showReader(file, provider)
UIManager:close(self.set_provider_dialog)
end,
@ -554,14 +553,12 @@ function FileChooser:showSetProviderButtons(file, filemanager_instance, reader_u
ok_callback = function()
DocumentRegistry:setProvider(file, provider, true)
filemanager_instance:onClose()
reader_ui:showReader(file, provider)
UIManager:close(self.set_provider_dialog)
end,
})
else
-- just once
filemanager_instance:onClose()
reader_ui:showReader(file, provider)
UIManager:close(self.set_provider_dialog)
end

@ -955,6 +955,12 @@ function Menu:init()
end
end
function Menu:onShowingReader()
-- Clear the dither flag to prevent it from infecting the queue and re-inserting a full-screen refresh...
self.dithered = nil
end
Menu.onSetupShowReader = Menu.onShowingReader
function Menu:onCloseWidget()
--- @fixme
-- we cannot refresh regionally using the dimen field
@ -965,7 +971,14 @@ function Menu:onCloseWidget()
-- the filemanager menu.
-- NOTE: For the same reason, don't make it flash,
-- because that'll trigger when we close the FM and open a book...
UIManager:setDirty(nil, "partial")
-- Don't do anything if we're in the process of tearing down FM or RD, or if we don't actually have a live instance of 'em...
local FileManager = require("apps/filemanager/filemanager")
local ReaderUI = require("apps/reader/readerui")
local reader_ui = ReaderUI:_getRunningInstance()
if (FileManager.instance and not FileManager.instance.tearing_down) or (reader_ui and not reader_ui.tearing_down) then
UIManager:setDirty(nil, "ui")
end
end
function Menu:updatePageInfo(select_number)
@ -1192,10 +1205,10 @@ function Menu:onMenuSelect(item)
return true
end
end
self:onMenuChoice(item)
if self.close_callback then
self.close_callback()
end
self:onMenuChoice(item)
else
-- save menu title for later resume
self.item_table.title = self.title

@ -592,8 +592,15 @@ function TouchMenu:init()
end
function TouchMenu:onCloseWidget()
-- NOTE: We don't pass a region in order to ensure a full-screen flash to avoid ghosting
UIManager:setDirty(nil, "flashui")
-- NOTE: We don't pass a region in order to ensure a full-screen flash to avoid ghosting,
-- but we only need to do that if we actually have a FM or RD below us.
-- Don't do anything when we're switching between the two, or if we don't actually have a live instance of 'em...
local FileManager = require("apps/filemanager/filemanager")
local ReaderUI = require("apps/reader/readerui")
local reader_ui = ReaderUI:_getRunningInstance()
if (FileManager.instance and not FileManager.instance.tearing_down) or (reader_ui and not reader_ui.tearing_down) then
UIManager:setDirty(nil, "flashui")
end
end
function TouchMenu:_recalculatePageLayout()

@ -110,8 +110,8 @@ function AutoSuspend:init()
self.auto_suspend_timeout_seconds = G_reader_settings:readSetting("auto_suspend_timeout_seconds") or default_auto_suspend_timeout_seconds
UIManager.event_hook:registerWidget("InputEvent", self)
-- We need an instance-specific function reference to schedule, because when going from FM to RD,
-- we instantiate the RD instance *before* tearing down the old FM instance.
-- We need an instance-specific function reference to schedule, because in some rare cases,
-- we may instantiate a new plugin instance *before* tearing down the old one.
self.task = function(shutdown_only)
self:_schedule(shutdown_only)
end

@ -283,9 +283,13 @@ function CalibreSearch:bookCatalog(t, option)
entry.text = string.format("%s - %s", book.title, book.authors[1])
end
entry.callback = function()
local Event = require("ui/event")
UIManager:broadcastEvent(Event:new("SetupShowReader"))
self.search_menu:onClose()
local ReaderUI = require("apps/reader/readerui")
ReaderUI:showReader(book.rootpath .. "/" .. book.lpath)
self.search_menu:onClose()
end
table.insert(catalog, entry)
end

@ -97,6 +97,9 @@ function MoveToArchive:commonProcess(is_move_process, moved_done_text)
self.settings:saveSetting("last_copied_from_dir", self.last_copied_from_dir)
self.settings:flush()
local Event = require("ui/event")
UIManager:broadcastEvent(Event:new("SetupShowReader"))
self.ui:onClose()
if is_move_process then
FileManager:moveFile(document_full_path, self.archive_dir_path)
@ -110,10 +113,10 @@ function MoveToArchive:commonProcess(is_move_process, moved_done_text)
ReadCollection:updateItemByPath(document_full_path, dest_file)
UIManager:show(ConfirmBox:new{
text = moved_done_text,
ok_callback = function ()
ok_callback = function()
ReaderUI:showReader(dest_file)
end,
cancel_callback = function ()
cancel_callback = function()
self:openFileBrowser(self.last_copied_from_dir)
end,
})

@ -31,7 +31,11 @@ function OPDSCatalog:init()
ok_text = _("Read now"),
cancel_text = _("Read later"),
ok_callback = function()
local Event = require("ui/event")
UIManager:broadcastEvent(Event:new("SetupShowReader"))
self:onClose()
ReaderUI:showReader(downloaded_file)
end
})

@ -284,6 +284,7 @@ else
if start_with == "last" and last_file then
local ReaderUI = require("apps/reader/readerui")
UIManager:nextTick(function()
-- Instantiate RD
ReaderUI:showReader(last_file)
end)
exit_code = UIManager:run()
@ -292,10 +293,11 @@ else
local home_dir =
G_reader_settings:readSetting("home_dir") or Device.home_dir or lfs.currentdir()
UIManager:nextTick(function()
-- Instantiate FM
FileManager:showFiles(home_dir)
end)
-- Always open history on top of filemanager so closing history
-- doesn't result in exit.
-- Always open FM modules on top of filemanager, so closing 'em doesn't result in an exit
-- because of an empty widget stack, and so they can interact with the FM instance as expected.
if start_with == "history" then
local FileManagerHistory = require("apps/filemanager/filemanagerhistory")
UIManager:nextTick(function()

@ -326,33 +326,76 @@ describe("UIManager spec", function()
UIManager:broadcastEvent("foo")
assert.is.same(#UIManager._window_stack, 0)
-- Remember that the stack is processed top to bottom!
-- Test making a hole in the middle of the stack.
UIManager._window_stack = {
{
widget = {
handleEvent = function()
UIManager._window_stack[1] = nil
UIManager._window_stack[2] = nil
UIManager._window_stack[3] = nil
assert.truthy(true)
end
}
},
{
widget = {
handleEvent = function()
assert.falsy(true)
end
}
},
{
widget = {
handleEvent = function()
assert.falsy(true)
end
}
},
{
widget = {
handleEvent = function()
table.remove(UIManager._window_stack, #UIManager._window_stack - 2)
table.remove(UIManager._window_stack, #UIManager._window_stack - 2)
table.remove(UIManager._window_stack, #UIManager._window_stack - 1)
end
}
},
{
widget = {
handleEvent = function()
assert.truthy(true)
end
}
},
}
UIManager:broadcastEvent("foo")
assert.is.same(2, #UIManager._window_stack)
-- Test inserting a new widget in the stack
local new_widget = {
widget = {
handleEvent = function()
assert.truthy(true)
end
}
}
UIManager._window_stack = {
{
widget = {
handleEvent = function()
assert.falsy(true);
table.insert(UIManager._window_stack, new_widget)
end
}
},
{
widget = {
handleEvent = function()
assert.falsy(true);
assert.truthy(true)
end
}
},
}
UIManager:broadcastEvent("foo")
assert.is.same(0, #UIManager._window_stack)
assert.is.same(3, #UIManager._window_stack)
end)
it("should handle stack change when closing widgets", function()

Loading…
Cancel
Save