From de6f2e84a38c071aee0e24e3836f2c47950cf20d Mon Sep 17 00:00:00 2001 From: NiLuJe Date: Tue, 11 May 2021 00:49:35 +0200 Subject: [PATCH] FileManager/ReaderUI: Clarify the current instance accessor (#7658) * FileManager/ReaderUI: Clarify the current instance accessor Make it clearer that we actually store it in a *module/class* member, not an *instance* member. Also, warn if there's a close/open mismatch. --- frontend/apps/filemanager/filemanager.lua | 52 ++++++++++++++--------- frontend/apps/reader/readerui.lua | 33 ++++++++++---- frontend/ui/widget/filechooser.lua | 10 +++-- plugins/opds.koplugin/opdscatalog.lua | 9 +--- reader.lua | 1 - 5 files changed, 65 insertions(+), 40 deletions(-) diff --git a/frontend/apps/filemanager/filemanager.lua b/frontend/apps/filemanager/filemanager.lua index a7ec0611a..8919ddbe0 100644 --- a/frontend/apps/filemanager/filemanager.lua +++ b/frontend/apps/filemanager/filemanager.lua @@ -29,7 +29,6 @@ local PluginLoader = require("pluginloader") local ReadCollection = require("readcollection") local ReaderDeviceStatus = require("apps/reader/modules/readerdevicestatus") local ReaderDictionary = require("apps/reader/modules/readerdictionary") -local ReaderUI = require("apps/reader/readerui") local ReaderWikipedia = require("apps/reader/modules/readerwikipedia") local Screenshoter = require("ui/widget/screenshoter") local Size = require("ui/size") @@ -50,7 +49,6 @@ local T = BaseUtil.template local FileManager = InputContainer:extend{ title = _("KOReader"), root_path = lfs.currentdir(), - onExit = function() end, mv_bin = Device:isAndroid() and "/system/bin/mv" or "/bin/mv", cp_bin = Device:isAndroid() and "/system/bin/cp" or "/bin/cp", @@ -60,10 +58,10 @@ local FileManager = InputContainer:extend{ function FileManager:onSetRotationMode(rotation) if rotation ~= nil and rotation ~= Screen:getRotationMode() then Screen:setRotationMode(rotation) - if self.instance then - self:reinit(self.instance.path, self.instance.focused_file) - UIManager:setDirty(self.instance.banner, function() - return "ui", self.instance.banner.dimen + if FileManager.instance then + self:reinit(self.path, self.focused_file) + UIManager:setDirty(self.banner, function() + return "ui", self.banner.dimen end) end end @@ -215,6 +213,7 @@ function FileManager:setupLayout() end function file_chooser:onFileSelect(file) -- luacheck: ignore + local ReaderUI = require("apps/reader/readerui") ReaderUI:showReader(file) return true end @@ -396,7 +395,7 @@ function FileManager:setupLayout() end, }) end - self:showSetProviderButtons(file, FileManager.instance, ReaderUI, one_time_providers) + self:showSetProviderButtons(file, FileManager.instance, one_time_providers) end, }, { @@ -737,8 +736,8 @@ function FileManager:reinit(path, focused_file) end function FileManager:getCurrentDir() - if self.instance then - return self.instance.file_chooser.path + if FileManager.instance then + return FileManager.instance.file_chooser.path end end @@ -768,12 +767,18 @@ function FileManager:onClose() self:handleEvent(Event:new("SaveSettings")) G_reader_settings:flush() UIManager:close(self) - if self.onExit then - self:onExit() - end return true end +function FileManager:onCloseWidget() + if FileManager.instance == self then + logger.dbg("Tearing down FileManager", tostring(self)) + else + logger.warn("FileManager instance mismatch! Closed", tostring(self), "while the active one is supposed to be", tostring(FileManager.instance)) + end + FileManager.instance = nil +end + function FileManager:onShowingReader() -- Allows us to optimize out a few useless refreshes in various CloseWidgets handlers... self.tearing_down = true @@ -834,6 +839,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() + local ReaderUI = require("apps/reader/readerui") 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. @@ -1145,6 +1151,13 @@ function FileManager:getStartWithMenuTable() end function FileManager:showFiles(path, focused_file) + -- Warn about and close any pre-existing FM instances first... + if FileManager.instance then + logger.warn("FileManager instance mismatch! Tried to spin up a new instance, while we still have an existing one:", tostring(FileManager.instance)) + -- Close the old one first! + FileManager.instance:onClose() + end + path = path or G_reader_settings:readSetting("lastdir") or filemanagerutil.getDefaultDir() G_reader_settings:saveSetting("lastdir", path) self:setRotationMode() @@ -1153,16 +1166,17 @@ function FileManager:showFiles(path, focused_file) covers_fullscreen = true, -- hint for UIManager:_repaint() root_path = path, focused_file = focused_file, - onExit = function() - self.instance = nil - 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 + -- NOTE: ReaderUI has a _getRunningInstance method for this, because it used to store the instance reference in a private module variable. + if FileManager.instance == nil then + logger.dbg("Spinning up new FileManager instance", tostring(file_manager)) + else + -- Should never happen, given what we did above... + logger.warn("FileManager instance mismatch! Opened", tostring(file_manager), "while we still have an existing instance:", tostring(FileManager.instance)) + end + FileManager.instance = file_manager end --- A shortcut to execute mv. diff --git a/frontend/apps/reader/readerui.lua b/frontend/apps/reader/readerui.lua index b7b82c9ad..35960255e 100644 --- a/frontend/apps/reader/readerui.lua +++ b/frontend/apps/reader/readerui.lua @@ -567,12 +567,12 @@ function ReaderUI:showReaderCoroutine(file, provider) end) end -local _running_instance = nil function ReaderUI:doShowReader(file, provider) logger.info("opening file", file) - -- Keep only one instance running - if _running_instance then - _running_instance:onClose() + -- Only keep a single instance running + if ReaderUI.instance then + logger.warn("ReaderUI instance mismatch! Tried to spin up a new instance, while we still have an existing one:", tostring(ReaderUI.instance)) + ReaderUI.instance:onClose() end local document = DocumentRegistry:openDocument(file, provider) if not document then @@ -621,11 +621,21 @@ function ReaderUI:doShowReader(file, provider) end UIManager:show(reader, "full") - _running_instance = reader + + if ReaderUI.instance == nil then + logger.dbg("Spinning up new ReaderUI instance", tostring(reader)) + else + -- Should never happen, given what we did above... + logger.warn("ReaderUI instance mismatch! Opened", tostring(reader), "while we still have an existing instance:", tostring(ReaderUI.instance)) + end + ReaderUI.instance = reader end +-- NOTE: The instance reference used to be stored in a private module variable, hence the getter method. +-- We've since aligned behavior with FileManager, which uses a class member instead, +-- but kept the function to avoid changing existing code. function ReaderUI:_getRunningInstance() - return _running_instance + return ReaderUI.instance end function ReaderUI:unlockDocumentWithPassword(document, try_again) @@ -741,9 +751,16 @@ function ReaderUI:onClose(full_refresh) self:notifyCloseDocument() end UIManager:close(self.dialog, full_refresh and "full") - if _running_instance == self then - _running_instance = nil +end + +function ReaderUI:onCloseWidget() + if ReaderUI.instance == self then + logger.dbg("Tearing down ReaderUI", tostring(self)) + else + logger.warn("ReaderUI instance mismatch! Closed", tostring(self), "while the active one is supposed to be", tostring(ReaderUI.instance)) end + ReaderUI.instance = nil + self._coroutine = nil end function ReaderUI:dealWithLoadDocumentFailure() diff --git a/frontend/ui/widget/filechooser.lua b/frontend/ui/widget/filechooser.lua index 56636acc1..72dc1482b 100644 --- a/frontend/ui/widget/filechooser.lua +++ b/frontend/ui/widget/filechooser.lua @@ -470,7 +470,9 @@ function FileChooser:getNextFile(curr_file) return next_file end -function FileChooser:showSetProviderButtons(file, filemanager_instance, reader_ui, one_time_providers) +function FileChooser:showSetProviderButtons(file, filemanager_instance, one_time_providers) + local ReaderUI = require("apps/reader/readerui") + local __, filename_pure = util.splitFilePathName(file) local filename_suffix = util.getFileNameSuffix(file) @@ -540,7 +542,7 @@ function FileChooser:showSetProviderButtons(file, filemanager_instance, reader_u ok_callback = function() DocumentRegistry:setProvider(file, provider, false) - reader_ui:showReader(file, provider) + ReaderUI:showReader(file, provider) UIManager:close(self.set_provider_dialog) end, }) @@ -553,13 +555,13 @@ function FileChooser:showSetProviderButtons(file, filemanager_instance, reader_u ok_callback = function() DocumentRegistry:setProvider(file, provider, true) - reader_ui:showReader(file, provider) + ReaderUI:showReader(file, provider) UIManager:close(self.set_provider_dialog) end, }) else -- just once - reader_ui:showReader(file, provider) + ReaderUI:showReader(file, provider) UIManager:close(self.set_provider_dialog) end end, diff --git a/plugins/opds.koplugin/opdscatalog.lua b/plugins/opds.koplugin/opdscatalog.lua index 56021d5b1..42a164558 100644 --- a/plugins/opds.koplugin/opdscatalog.lua +++ b/plugins/opds.koplugin/opdscatalog.lua @@ -4,7 +4,6 @@ local ConfirmBox = require("ui/widget/confirmbox") local FrameContainer = require("ui/widget/container/framecontainer") local InputContainer = require("ui/widget/container/inputcontainer") local OPDSBrowser = require("opdsbrowser") -local ReaderUI = require("apps/reader/readerui") local UIManager = require("ui/uimanager") local logger = require("logger") local _ = require("gettext") @@ -13,7 +12,6 @@ local T = require("ffi/util").template local OPDSCatalog = InputContainer:extend{ title = _("OPDS Catalog"), - onExit = function() end, } function OPDSCatalog:init() @@ -36,6 +34,7 @@ function OPDSCatalog:init() self:onClose() + local ReaderUI = require("apps/reader/readerui") ReaderUI:showReader(downloaded_file) end }) @@ -67,18 +66,12 @@ function OPDSCatalog:showCatalog() UIManager:show(OPDSCatalog:new{ dimen = Screen:getSize(), covers_fullscreen = true, -- hint for UIManager:_repaint() - onExit = function() - --UIManager:quit() - end }) end function OPDSCatalog:onClose() logger.dbg("close OPDS catalog") UIManager:close(self) - if self.onExit then - self:onExit() - end return true end diff --git a/reader.lua b/reader.lua index 797449bef..54d1793b0 100755 --- a/reader.lua +++ b/reader.lua @@ -342,7 +342,6 @@ local function exitReader() -- Save current rotation (or the original rotation if ScreenSaver temporarily modified it) to remember it for next startup G_reader_settings:saveSetting("closed_rotation_mode", Device.orig_rotation_mode or Device.screen:getRotationMode()) - G_reader_settings:close() -- Close lipc handles