From bb65a69193cfb263f665f0de13a34b19bd8124dc Mon Sep 17 00:00:00 2001 From: NiLuJe Date: Thu, 13 May 2021 13:05:05 +0200 Subject: [PATCH] Geom: nil guard a few rect methods (#7664) We've managed to trip a few of those on dimen fields post-init but pre-paintTo in a few weird coner-cases, a point at which dimen is often nil. ConfigDialog: Deal with that very thing in update() Fix #7656 --- frontend/apps/filemanager/filemanager.lua | 18 +++++++++--------- frontend/apps/reader/readerui.lua | 16 ++++++++-------- frontend/device/generic/device.lua | 10 +++++++++- frontend/ui/geometry.lua | 23 +++++++++++++++-------- frontend/ui/widget/configdialog.lua | 5 +++++ spec/unit/device_spec.lua | 2 +- 6 files changed, 47 insertions(+), 27 deletions(-) diff --git a/frontend/apps/filemanager/filemanager.lua b/frontend/apps/filemanager/filemanager.lua index 8919ddbe0..2ff24882a 100644 --- a/frontend/apps/filemanager/filemanager.lua +++ b/frontend/apps/filemanager/filemanager.lua @@ -539,6 +539,15 @@ function FileManager:init() self:initGesListener() self:handleEvent(Event:new("SetDimensions", self.dimen)) + + -- 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(self)) + else + -- Should never happen, given what we did above... + logger.err("FileManager instance mismatch! Opened", tostring(self), "while we still have an existing instance:", tostring(FileManager.instance), debug.traceback()) + end + FileManager.instance = self end function FileChooser:onBack() @@ -1168,15 +1177,6 @@ function FileManager:showFiles(path, focused_file) focused_file = focused_file, } UIManager:show(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 35960255e..eddd2e14d 100644 --- a/frontend/apps/reader/readerui.lua +++ b/frontend/apps/reader/readerui.lua @@ -439,6 +439,14 @@ function ReaderUI:init() -- for _, tzone in ipairs(self._ordered_touch_zones) do -- print(" "..tzone.def.id) -- end + + if ReaderUI.instance == nil then + logger.dbg("Spinning up new ReaderUI instance", tostring(self)) + else + -- Should never happen, given what we did above... + logger.err("ReaderUI instance mismatch! Opened", tostring(self), "while we still have an existing instance:", tostring(ReaderUI.instance), debug.traceback()) + end + ReaderUI.instance = self end function ReaderUI:setLastDirForFileBrowser(dir) @@ -621,14 +629,6 @@ function ReaderUI:doShowReader(file, provider) end UIManager:show(reader, "full") - - 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. diff --git a/frontend/device/generic/device.lua b/frontend/device/generic/device.lua index acd3197ce..ef958de45 100644 --- a/frontend/device/generic/device.lua +++ b/frontend/device/generic/device.lua @@ -5,6 +5,7 @@ This module defines stubs for common methods. --]] local DataStorage = require("datastorage") +local Geom = require("ui/geometry") local logger = require("logger") local util = require("util") local _ = require("gettext") @@ -182,7 +183,7 @@ function Device:init() end logger.info("initializing for device", self.model) - logger.info("framebuffer resolution:", self.screen:getSize()) + logger.info("framebuffer resolution:", self.screen:getRawSize()) if not self.input then self.input = require("device/input"):new{device = self} @@ -212,6 +213,13 @@ function Device:init() self:lockGSensor(true) end end + + -- Screen:getSize is used throughout the code, and that code usually expects getting a real Geom object... + -- But as implementations come from base, they just return a Geom-like table... + self.screen.getSize = function() + local rect = self.screen.getRawSize(self.screen) + return Geom:new{ x = rect.x, y = rect.y, w = rect.w, h = rect.h } + end end function Device:setScreenDPI(dpi_override) diff --git a/frontend/ui/geometry.lua b/frontend/ui/geometry.lua index ab2791cb2..5037b9303 100644 --- a/frontend/ui/geometry.lua +++ b/frontend/ui/geometry.lua @@ -142,8 +142,10 @@ Works for rectangles, dimensions and points @treturn Geom ]] function Geom:combine(rect_b) + -- We'll want to return a *new* object, so, start with a copy of self local combined = self:copy() if not rect_b or rect_b:area() == 0 then return combined end + if combined.x > rect_b.x then combined.x = rect_b.x end @@ -171,8 +173,9 @@ Returns a new rectangle for the part that we and a given rectangle share ]]-- --- @todo what happens if there is no rectangle shared? currently behaviour is undefined. function Geom:intersect(rect_b) - -- make a copy of self local intersected = self:copy() + if not rect_b or rect_b:area() == 0 then return intersected end + if self.x < rect_b.x then intersected.x = rect_b.x end @@ -198,6 +201,8 @@ Returns true if self does not share any area with rect_b @tparam Geom rect_b ]] function Geom:notIntersectWith(rect_b) + if not rect_b or rect_b:area() == 0 then return true end + if (self.x >= (rect_b.x + rect_b.w)) or (self.y >= (rect_b.y + rect_b.h)) or (rect_b.x >= (self.x + self.w)) @@ -228,17 +233,19 @@ function Geom:setSizeTo(rect_b) end --[[-- -Checks whether rect_b is within current rectangle +Checks whether geom is within current rectangle Works for dimensions, too. For points, it is basically an equality check. -@tparam Geom rect_b +@tparam Geom geom ]] -function Geom:contains(rect_b) - if self.x <= rect_b.x - and self.y <= rect_b.y - and self.x + self.w >= rect_b.x + rect_b.w - and self.y + self.h >= rect_b.y + rect_b.h +function Geom:contains(geom) + if not geom then return false end + + if self.x <= geom.x + and self.y <= geom.y + and self.x + self.w >= geom.x + geom.w + and self.y + self.h >= geom.y + geom.h then return true end diff --git a/frontend/ui/widget/configdialog.lua b/frontend/ui/widget/configdialog.lua index 21da70fd1..abb76114e 100644 --- a/frontend/ui/widget/configdialog.lua +++ b/frontend/ui/widget/configdialog.lua @@ -880,6 +880,7 @@ function ConfigDialog:update() config_dialog = self, } + local old_dimen = self.dialog_frame and self.dialog_frame.dimen and self.dialog_frame.dimen:copy() or Geom:new{} self.dialog_frame = FrameContainer:new{ background = Blitbuffer.COLOR_WHITE, padding_bottom = 0, -- ensured by MenuBar @@ -888,6 +889,10 @@ function ConfigDialog:update() self.config_menubar, }, } + -- Ensure we have a sane-ish Geom object *before* paintTo gets called, + -- to avoid weirdness in race-y SwipeCloseMenu calls... + self.dialog_frame.dimen = old_dimen + -- Reset the focusmanager cursor self.selected.y=#self.layout self.selected.x=self.panel_index diff --git a/spec/unit/device_spec.lua b/spec/unit/device_spec.lua index f85c8bace..b97b6e09b 100644 --- a/spec/unit/device_spec.lua +++ b/spec/unit/device_spec.lua @@ -9,7 +9,7 @@ describe("device module", function() mock_fb = { new = function() return { - getSize = function() return {w = 600, h = 800} end, + getRawSize = function() return {w = 600, h = 800} end, getWidth = function() return 600 end, getDPI = function() return 72 end, setViewport = function() end,