Merge pull request #709 from psychon/implement-set-children-sanely

Implement set children sanely

lib/wibox/layout/align.lua

@@ -175,11 +175,9 @@ end
 -- This layout only accept three children, all others will be ignored
 -- @tparam table children A table composed of valid widgets
 function align:set_children(children)
-    if not children then return self:reset() end
-    self.first  = children[1]
-    self.second = children[2]
-    self.third  = children[3]
-    self:emit_signal("widget::layout_changed")
+    self:set_first(children[1])
+    self:set_second(children[2])
+    self:set_third(children[3])
 end
 
 --- Fit the align layout into the given space. The align layout will

lib/wibox/layout/constraint.lua

@@ -54,8 +54,7 @@ end
 -- This layout only accept one children, all others will be ignored
 -- @tparam table children A table composed of valid widgets
 function constraint:set_children(children)
-    self.widget = children and children[1]
-    self:emit_signal("widget::layout_changed")
+    self:set_widget(children[1])
 end
 
 --- Set the strategy to use for the constraining. Valid values are 'max',

lib/wibox/layout/fixed.lua

@@ -5,6 +5,7 @@
 -- @classmod wibox.layout.fixed
 ---------------------------------------------------------------------------
 
+local unpack = unpack or table.unpack -- luacheck: globals unpack (compatibility with Lua 5.1)
 local base  = require("wibox.widget.base")
 local table = table
 local pairs = pairs
@@ -110,9 +111,10 @@ end
 --- Replace the layout children
 -- @tparam table children A table composed of valid widgets
 function fixed:set_children(children)
-    if not children then return self:reset() end
-    self.widgets = children
-    self:emit_signal("widget::layout_changed")
+    self:reset()
+    if #children > 0 then
+        self:add(unpack(children))
+    end
 end
 
 --- Replace the first instance of `widget` in the layout with `widget2`

lib/wibox/layout/margin.lua

@@ -82,8 +82,7 @@ end
 -- This layout only accept one children, all others will be ignored
 -- @tparam table children A table composed of valid widgets
 function margin:set_children(children)
-    self.widget = children and children[1]
-    self:emit_signal("widget::layout_changed")
+    self:set_widget(children[1])
 end
 
 --- Set all the margins to val.

lib/wibox/layout/mirror.lua

@@ -64,8 +64,7 @@ end
 -- This layout only accept one children, all others will be ignored
 -- @tparam table children A table composed of valid widgets
 function mirror:set_children(children)
-    self.widget = children and children[1]
-    self:emit_signal("widget::layout_changed")
+    self:set_widget(children[1])
 end
 
 --- Reset this layout. The widget will be removed and the axes reset.

lib/wibox/layout/rotate.lua

@@ -76,8 +76,7 @@ end
 -- This layout only accept one children, all others will be ignored
 -- @tparam table children A table composed of valid widgets
 function rotate:set_children(children)
-    self.widget = children and children[1]
-    self:emit_signal("widget::layout_changed")
+    self:set_widget(children[1])
 end
 
 --- Reset this layout. The widget will be removed and the rotation reset.

lib/wibox/layout/scroll.lua

@@ -267,8 +267,7 @@ end
 -- This layout only accept one children, all others will be ignored
 -- @tparam table children A table composed of valid widgets
 function scroll:set_children(children)
-    self.widget = children and children[1]
-    self:emit_signal("widget::layout_changed")
+    self:set_widget(children[1])
 end
 
 --- Specify the expand mode that is used for extra space.

lib/wibox/widget/background.lua

@@ -109,8 +109,7 @@ end
 -- This layout only accept one children, all others will be ignored
 -- @tparam table children A table composed of valid widgets
 function background:set_children(children)
-    self.widget = children and children[1]
-    self:emit_signal("widget::layout_changed")
+    self:set_widget(children[1])
 end
 
 --- Set the background to use

lib/wibox/widget/base.lua

@@ -338,13 +338,15 @@ local function parse_table(t, leave_empty)
     local attributes, widgets = {}, {}
     for k,v in pairs(t) do
         if type(k) == "number" then
-            -- As `ipairs` doesn't always work on sparse tables, update the
-            -- maximum
-            if k > max then
-                max = k
-            end
+            if v then
+                -- As `ipairs` doesn't always work on sparse tables, update the
+                -- maximum
+                if k > max then
+                    max = k
+                end
 
-            widgets[k] = v
+                widgets[k] = v
+            end
         else
             attributes[k] = v
         end
@@ -391,6 +393,7 @@ local function drill(ids, content)
                 e, id2 = drill(ids, v)
                 widgets[k] = e
             end
+            base.check_widget(widgets[k])
 
             -- Place the widget in the access table
             if id2 then
@@ -559,7 +562,8 @@ end
 --- Do some sanity checking on widget. This function raises a lua error if
 -- widget is not a valid widget.
 function base.check_widget(widget)
-    assert(type(widget) == "table")
+    assert(type(widget) == "table", "Type should be table, but is " .. tostring(type(widget)))
+    assert(widget.is_widget, "Argument is not a widget!")
     for _, func in pairs({ "add_signal", "connect_signal", "disconnect_signal" }) do
         assert(type(widget[func]) == "function", func .. " is not a function")
     end

spec/wibox/test_utils.lua

@@ -84,6 +84,7 @@ return {
         local w = object()
         w:add_signal("widget::redraw_needed")
         w:add_signal("widget::layout_changed")
+        w.is_widget = true
         w.visible = true
         w.opacity = 1
         if width or height then

spec/wibox/widget/base_spec.lua

@@ -0,0 +1,92 @@
+---------------------------------------------------------------------------
+-- @author Uli Schlachter
+-- @copyright 2016 Uli Schlachter
+---------------------------------------------------------------------------
+
+local base = require("wibox.widget.base")
+local no_parent = base.no_parent_I_know_what_I_am_doing
+
+describe("wibox.widget.base", function()
+    local widget1, widget2
+    before_each(function()
+        widget1 = base.make_widget()
+        widget2 = base.make_widget()
+        widget1.layout = function()
+            return { base.place_widget_at(widget2, 0, 0, 1, 1) }
+        end
+    end)
+
+    describe("caches", function()
+        it("garbage collectable", function()
+            local alive = setmetatable({ widget1, widget2 }, { __mode = "kv" })
+            assert.is.equal(2, #alive)
+
+            widget1, widget2 = nil, nil
+            collectgarbage("collect")
+            assert.is.equal(0, #alive)
+        end)
+
+        it("simple cache clear", function()
+            local alive = setmetatable({ widget1, widget2 }, { __mode = "kv" })
+            base.layout_widget(no_parent, { "fake context" }, widget1, 20, 20)
+            assert.is.equal(2, #alive)
+
+            widget1, widget2 = nil, nil
+            collectgarbage("collect")
+            assert.is.equal(0, #alive)
+        end)
+
+        it("self-reference cache clear", function()
+            widget2.widget = widget1
+
+            local alive = setmetatable({ widget1, widget2 }, { __mode = "kv" })
+            base.layout_widget(no_parent, { "fake context" }, widget1, 20, 20)
+            assert.is.equal(2, #alive)
+
+            widget1, widget2 = nil, nil
+            collectgarbage("collect")
+            assert.is.equal(0, #alive)
+        end)
+    end)
+
+    describe("setup", function()
+        it("Filters out 'false'", function()
+            -- Regression test: There was a bug where "nil"s where correctly
+            -- skipped, but "false" entries survived
+            local layout1, layout2 = base.make_widget(), base.make_widget()
+            local called = false
+            function layout1:set_widget(w)
+                called = true
+                assert.equals(w, layout2)
+            end
+            function layout2:set_children(children)
+                assert.is_same({nil, widget1, nil, widget2}, children)
+            end
+            layout2.allow_empty_widget = true
+            layout1:setup{ layout = layout2, false, widget1, nil, widget2 }
+            assert.is_true(called)
+        end)
+
+        it("Attribute 'false' works", function()
+            -- Regression test: I introduced a bug with the above fix
+            local layout1, layout2 = base.make_widget(), base.make_widget()
+            local called1, called2 = false, false
+            function layout1:set_widget(w)
+                called1 = true
+                assert.equals(w, layout2)
+            end
+            function layout2:set_children(children)
+                assert.is_same({}, children)
+            end
+            function layout2:set_foo(foo)
+                called2 = true
+                assert.is_false(foo)
+            end
+            layout1:setup{ layout = layout2, foo = false }
+            assert.is_true(called1)
+            assert.is_true(called2)
+        end)
+    end)
+end)
+
+-- vim: filetype=lua:expandtab:shiftwidth=4:tabstop=8:softtabstop=4:textwidth=80