From 41a8fabf4c64bf62e48290f07b1bc0f897cb61ad Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Thu, 17 Sep 2015 15:01:50 +0200 Subject: [PATCH] Explicitly track dependencies between widgets Before this, dependencies between widgets where implicitly discovered by recursive calls to base.fit_widget() and base.layout_widget(). However, it is too easy to get this wrong (just call one of these functions from outside of a widget's :fit() / :layout() function) and the resulting mess would be hard to debug. Thus, this commit changes the API so that callers have to identify themselves and we can explicitly record the dependency between the widgets involved. This also fixes a bug where no dependencies were tracked for widgets after :set_visible(false). Whoops... Sorry for breaking the API for adding this. Signed-off-by: Uli Schlachter --- lib/wibox/hierarchy.lua | 3 +- lib/wibox/layout/align.lua | 16 +++--- lib/wibox/layout/constraint.lua | 2 +- lib/wibox/layout/fixed.lua | 6 +-- lib/wibox/layout/flex.lua | 2 +- lib/wibox/layout/margin.lua | 2 +- lib/wibox/layout/mirror.lua | 2 +- lib/wibox/layout/rotate.lua | 2 +- lib/wibox/widget/background.lua | 2 +- lib/wibox/widget/base.lua | 92 ++++++++++++++------------------- spec/wibox/test_utils.lua | 3 +- 11 files changed, 59 insertions(+), 73 deletions(-) diff --git a/lib/wibox/hierarchy.lua b/lib/wibox/hierarchy.lua index 7ad7f4fd0..bf70adbf4 100644 --- a/lib/wibox/hierarchy.lua +++ b/lib/wibox/hierarchy.lua @@ -12,12 +12,13 @@ local matrix = require("gears.matrix") local cairo = require("lgi").cairo local base = require("wibox.widget.base") +local no_parent = base.no_parent_I_know_what_I_am_doing local hierarchy = {} local function hierarchy_new(context, widget, width, height, redraw_callback, layout_callback, callback_arg, matrix_to_parent, matrix_to_device) - local children = base.layout_widget(context, widget, width, height) + local children = base.layout_widget(no_parent, context, widget, width, height) local draws_x1, draws_y1, draws_x2, draws_y2 = 0, 0, width, height local result = { _matrix = matrix_to_parent, diff --git a/lib/wibox/layout/align.lua b/lib/wibox/layout/align.lua index e31fb7242..b0f8d88ac 100644 --- a/lib/wibox/layout/align.lua +++ b/lib/wibox/layout/align.lua @@ -40,7 +40,7 @@ function align:layout(context, width, height) -- if the second widget doesn't exist, we will prioritise the first one -- instead if self._expand ~= "inside" and self.second then - local w, h = base.fit_widget(context, self.second, width, height) + local w, h = base.fit_widget(self, context, self.second, width, height) size_second = self.dir == "y" and h or w -- if all the space is taken, skip the rest, and draw just the middle -- widget @@ -59,7 +59,7 @@ function align:layout(context, width, height) -- into the remaining space if self._expand ~= "outside" then if self.dir == "y" then - _, h = base.fit_widget(context, self.first, width, size_remains) + _, h = base.fit_widget(self, context, self.first, width, size_remains) size_first = h -- for "inside", the third widget will get a chance to use the -- remaining space, then the middle widget. For "none" we give @@ -70,7 +70,7 @@ function align:layout(context, width, height) size_remains = size_remains - h end else - w, _ = base.fit_widget(context, self.first, size_remains, height) + w, _ = base.fit_widget(self, context, self.first, size_remains, height) size_first = w if self._expand == "inside" or not self.second then size_remains = size_remains - w @@ -90,13 +90,13 @@ function align:layout(context, width, height) local w, h, _ = width, height, nil if self._expand ~= "outside" then if self.dir == "y" then - _, h = base.fit_widget(context, self.third, width, size_remains) + _, h = base.fit_widget(self, context, self.third, width, size_remains) -- give the middle widget the rest of the space for "inside" mode if self._expand == "inside" then size_remains = size_remains - h end else - w, _ = base.fit_widget(context, self.third, size_remains, height) + w, _ = base.fit_widget(self, context, self.third, size_remains, height) if self._expand == "inside" then size_remains = size_remains - w end @@ -125,10 +125,10 @@ function align:layout(context, width, height) end else if self.dir == "y" then - _, h = base.fit_widget(context, self.second, width, size_second) + _, h = base.fit_widget(self, context, self.second, width, size_second) y = floor( (height - h)/2 ) else - w, _ = base.fit_widget(context, self.second, size_second, height) + w, _ = base.fit_widget(self, context, self.second, size_second, height) x = floor( (width -w)/2 ) end end @@ -175,7 +175,7 @@ function align:fit(context, orig_width, orig_height) local used_in_other = 0 for k, v in pairs{self.first, self.second, self.third} do - local w, h = base.fit_widget(context, v, orig_width, orig_height) + local w, h = base.fit_widget(self, context, v, orig_width, orig_height) local max = self.dir == "y" and w or h if max > used_in_other then diff --git a/lib/wibox/layout/constraint.lua b/lib/wibox/layout/constraint.lua index 9b88f97ed..2e9a39baa 100644 --- a/lib/wibox/layout/constraint.lua +++ b/lib/wibox/layout/constraint.lua @@ -27,7 +27,7 @@ function constraint:fit(context, width, height) w = self._strategy(width, self._width) h = self._strategy(height, self._height) - w, h = base.fit_widget(context, self.widget, w, h) + w, h = base.fit_widget(self, context, self.widget, w, h) else w, h = 0, 0 end diff --git a/lib/wibox/layout/fixed.lua b/lib/wibox/layout/fixed.lua index fc2519934..f745834b4 100644 --- a/lib/wibox/layout/fixed.lua +++ b/lib/wibox/layout/fixed.lua @@ -26,7 +26,7 @@ function fixed:layout(context, width, height) x, y = 0, pos w, h = width, height - pos if k ~= #self.widgets or not self._fill_space then - _, h = base.fit_widget(context, v, w, h); + _, h = base.fit_widget(self, context, v, w, h); end pos = pos + h + spacing in_dir = h @@ -34,7 +34,7 @@ function fixed:layout(context, width, height) x, y = pos, 0 w, h = width - pos, height if k ~= #self.widgets or not self._fill_space then - w, _ = base.fit_widget(context, v, w, h); + w, _ = base.fit_widget(self, context, v, w, h); end pos = pos + w + spacing in_dir = w @@ -65,7 +65,7 @@ function fixed:fit(context, orig_width, orig_height) local used_in_dir, used_max = 0, 0 for k, v in pairs(self.widgets) do - local w, h = base.fit_widget(context, v, width, height) + local w, h = base.fit_widget(self, context, v, width, height) local in_dir, max if self.dir == "y" then max, in_dir = w, h diff --git a/lib/wibox/layout/flex.lua b/lib/wibox/layout/flex.lua index 47d983c45..8ceab56f0 100644 --- a/lib/wibox/layout/flex.lua +++ b/lib/wibox/layout/flex.lua @@ -73,7 +73,7 @@ function flex:fit(context, orig_width, orig_height) local sub_width = self.dir == "y" and orig_width or orig_width / #self.widgets for k, v in pairs(self.widgets) do - local w, h = base.fit_widget(context, v, sub_width, sub_height) + local w, h = base.fit_widget(self, context, v, sub_width, sub_height) local max = self.dir == "y" and w or h if max > used_in_other then diff --git a/lib/wibox/layout/margin.lua b/lib/wibox/layout/margin.lua index edb2c8f55..c84fb3900 100644 --- a/lib/wibox/layout/margin.lua +++ b/lib/wibox/layout/margin.lua @@ -53,7 +53,7 @@ function margin:fit(context, width, height) local extra_h = self.top + self.bottom local w, h = 0, 0 if self.widget then - w, h = base.fit_widget(context, self.widget, width - extra_w, height - extra_h) + w, h = base.fit_widget(self, context, self.widget, width - extra_w, height - extra_h) end return w + extra_w, h + extra_h end diff --git a/lib/wibox/layout/mirror.lua b/lib/wibox/layout/mirror.lua index fcd942304..836993f3c 100644 --- a/lib/wibox/layout/mirror.lua +++ b/lib/wibox/layout/mirror.lua @@ -45,7 +45,7 @@ function mirror:fit(context, ...) if not self.widget then return 0, 0 end - return base.fit_widget(context, self.widget, ...) + return base.fit_widget(self, context, self.widget, ...) end --- Set the widget that this layout mirrors. diff --git a/lib/wibox/layout/rotate.lua b/lib/wibox/layout/rotate.lua index b4772963f..9dd865202 100644 --- a/lib/wibox/layout/rotate.lua +++ b/lib/wibox/layout/rotate.lua @@ -54,7 +54,7 @@ function rotate:fit(context, width, height) if not self.widget then return 0, 0 end - return transform(self, base.fit_widget(context, self.widget, transform(self, width, height))) + return transform(self, base.fit_widget(self, context, self.widget, transform(self, width, height))) end --- Set the widget that this layout rotates. diff --git a/lib/wibox/widget/background.lua b/lib/wibox/widget/background.lua index cca1c1d0d..b1dcd9894 100644 --- a/lib/wibox/widget/background.lua +++ b/lib/wibox/widget/background.lua @@ -52,7 +52,7 @@ function background:fit(context, width, height) return 0, 0 end - return base.fit_widget(context, self.widget, width, height) + return base.fit_widget(self, context, self.widget, width, height) end --- Set the widget that is drawn on top of the background diff --git a/lib/wibox/widget/base.lua b/lib/wibox/widget/base.lua index 562670663..51d3cfc8d 100644 --- a/lib/wibox/widget/base.lua +++ b/lib/wibox/widget/base.lua @@ -19,41 +19,12 @@ local base = {} -- {{{ Caches -local call_stack = {} -- Indexes are widgets, allow them to be garbage-collected local widget_dependencies = setmetatable({}, { __mode = "k" }) --- Don't do this in unit tests -if awesome and awesome.connect_signal then - -- Reset the call stack at each refresh. This fixes things up in case there was - -- an error in some callback and thus put_cache() wasn't called (if this - -- happens, we possibly recorded too many deps, but so what?) - awesome.connect_signal("refresh", function() - call_stack = {} - end) -end - --- When you call get_cache_and_record_deps(), the widget is recorded in a stack --- until the following put_cache(). All other calls to --- get_cache_and_record_deps() that happen during this will cause a dependency --- between the widgets that are involved to be recorded. This information is --- used by clear_caches() to also clear all caches of dependent widgets. - --- Get the caches for a widget and record its dependencies. All following --- cache-uses will record this widgets as a dependency. This returns a function +-- Get the cache of the given kind for this widget. This returns a gears.cache -- that calls the callback of kind `kind` on the widget. -local function get_cache_and_record_deps(widget, kind) - -- Record dependencies (each entry in the call stack depends on `widget`) - local deps = widget_dependencies[widget] or {} - for _, w in pairs(call_stack) do - deps[w] = true - end - widget_dependencies[widget] = deps - - -- Add widget to call stack - table.insert(call_stack, widget) - - -- Create cache if needed +local function get_cache(widget, kind) if not widget._widget_caches[kind] then widget._widget_caches[kind] = cache.new(function(...) return widget[kind](widget, ...) @@ -62,24 +33,36 @@ local function get_cache_and_record_deps(widget, kind) return widget._widget_caches[kind] end --- Each call to the above function should be followed by a call to this --- function. Everything in-between is recorded as a dependency (it's --- complicated...). -local function put_cache(widget) - assert(#call_stack ~= 0) - if table.remove(call_stack) ~= widget then - put_cache(widget) +-- Special value to skip the dependency recording that is normally done by +-- base.fit_widget() and base.layout_widget(). The caller must ensure that no +-- caches depend on the result of the call and/or must handle the childs +-- widget::layout_changed signal correctly when using this. +base.no_parent_I_know_what_I_am_doing = {} + +-- Record a dependency from parent to child: The layout of parent depends on the +-- layout of child. +local function record_dependency(parent, child) + if parent == base.no_parent_I_know_what_I_am_doing then + return end + + base.check_widget(parent) + base.check_widget(child) + + local deps = widget_dependencies[child] or {} + deps[parent] = true + widget_dependencies[child] = deps end -- Clear the caches for `widget` and all widgets that depend on it. -local function clear_caches(widget) - for w in pairs(widget_dependencies[widget] or {}) do - widget_dependencies[w] = {} - w._widget_caches = {} - end +local clear_caches +function clear_caches(widget) + local deps = widget_dependencies[widget] or {} widget_dependencies[widget] = {} widget._widget_caches = {} + for w in pairs(deps) do + clear_caches(w) + end end -- }}} @@ -92,12 +75,15 @@ end --- Fit a widget for the given available width and height. This calls the -- widget's `:fit` callback and caches the result for later use. Never call -- `:fit` directly, but always through this function! +-- @param parent The parent widget which requests this information. -- @param context The context in which we are fit. -- @param widget The widget to fit (this uses widget:fit(width, height)). -- @param width The available width for the widget -- @param height The available height for the widget -- @return The width and height that the widget wants to use -function base.fit_widget(context, widget, width, height) +function base.fit_widget(parent, context, widget, width, height) + record_dependency(parent, widget) + if not widget.visible then return 0, 0 end @@ -108,12 +94,10 @@ function base.fit_widget(context, widget, width, height) local w, h = 0, 0 if widget.fit then - local cache = get_cache_and_record_deps(widget, "fit") - w, h = cache:get(context, width, height) - put_cache(widget) + w, h = get_cache(widget, "fit"):get(context, width, height) else -- If it has no fit method, calculate based on the size of children - local children = base.layout_widget(context, widget, width, height) + local children = base.layout_widget(parent, context, widget, width, height) for _, info in ipairs(children or {}) do local x, y, w2, h2 = matrix.transform_rectangle(info._matrix, 0, 0, info._width, info._height) @@ -131,12 +115,15 @@ end -- widget's `:layout` callback and caches the result for later use. Never call -- `:layout` directly, but always through this function! However, normally there -- shouldn't be any reason why you need to use this function. +-- @param parent The parent widget which requests this information. -- @param context The context in which we are laid out. -- @param widget The widget to layout (this uses widget:layout(context, width, height)). -- @param width The available width for the widget -- @param height The available height for the widget -- @return The result from the widget's `:layout` callback. -function base.layout_widget(context, widget, width, height) +function base.layout_widget(parent, context, widget, width, height) + record_dependency(parent, widget) + if not widget.visible then return end @@ -146,10 +133,7 @@ function base.layout_widget(context, widget, width, height) local height = math.max(0, height) if widget.layout then - local cache = get_cache_and_record_deps(widget, "layout") - local result = cache:get(context, width, height) - put_cache(widget) - return result + return get_cache(widget, "layout"):get(context, width, height) end end @@ -383,7 +367,7 @@ function base.make_widget(proxy, widget_name) if proxy then ret.fit = function(_, context, width, height) - return base.fit_widget(context, proxy, width, height) + return base.fit_widget(ret, context, proxy, width, height) end ret.layout = function(_, context, width, height) return { base.place_widget_at(proxy, 0, 0, width, height) } diff --git a/spec/wibox/test_utils.lua b/spec/wibox/test_utils.lua index 14f51d3f1..08d2b01bc 100644 --- a/spec/wibox/test_utils.lua +++ b/spec/wibox/test_utils.lua @@ -9,6 +9,7 @@ local matrix_equals = require("gears.matrix").equals local base = require("wibox.widget.base") local say = require("say") local assert = require("luassert") +local no_parent = base.no_parent_I_know_what_I_am_doing -- {{{ Own widget-based assertions local function widget_fit(state, arguments) @@ -19,7 +20,7 @@ local function widget_fit(state, arguments) local widget = arguments[1] local given = arguments[2] local expected = arguments[3] - local w, h = base.fit_widget({ "fake context" }, widget, given[1], given[2]) + local w, h = base.fit_widget(no_parent, { "fake context" }, widget, given[1], given[2]) local fits = expected[1] == w and expected[2] == h if state.mod == fits then