Merge pull request #3238 from ShayAgros/master

Fixed wrong handling of negative spacing in layout.fixed

lib/wibox/layout/fixed.lua

@@ -21,45 +21,61 @@ local fixed = {}
 -- @param height The available height.
 function fixed:layout(context, width, height)
     local result = {}
-    local pos,spacing = 0, self._private.spacing
+    local spacing = self._private.spacing or 0
-    local spacing_widget = self._private.spacing_widget
     local is_y = self._private.dir == "y"
     local is_x = not is_y
     local abspace = math.abs(spacing)
     local spoffset = spacing < 0 and 0 or spacing
+    local widgets_nr = #self._private.widgets
+    local spacing_widget
+    local x, y = 0, 0
+
+    spacing_widget = spacing ~= 0 and self._private.spacing_widget or nil
 
     for k, v in pairs(self._private.widgets) do
-        local x, y, w, h, _
+        local w, h = width - x, height - y
+
         if is_y then
-            x, y = 0, pos
+            if k ~= widgets_nr or not self._private.fill_space then
-            w, h = width, height - pos
+                h = select(2, base.fit_widget(self, context, v, w, h))
-            if k ~= #self._private.widgets or not self._private.fill_space then
-                _, h = base.fit_widget(self, context, v, w, h);
-            end
-            pos = pos + h + spacing
-        else
-            x, y = pos, 0
-            w, h = width - pos, height
-            if k ~= #self._private.widgets or not self._private.fill_space then
-                w, _ = base.fit_widget(self, context, v, w, h);
-            end
-            pos = pos + w + spacing
             end
 
-        if (is_y and pos-spacing > height) or
+            if y - spacing >= height then
-            (is_x and pos-spacing > width) then
+                -- pop the spacing widget added in previous iteration if used
+                if spacing_widget then
+                    table.remove(result)
+                end
                 break
             end
+        else
+            if k ~= widgets_nr or not self._private.fill_space then
+                w = select(1, base.fit_widget(self, context, v, w, h))
+            end
 
-        -- Add the spacing widget
+            if x - spacing >= width then
-        if k > 1 and abspace > 0 and spacing_widget then
+                -- pop the spacing widget added in previous iteration if used
+                if spacing_widget then
+                    table.remove(result)
+                end
+                break
+            end
+        end
+
+        -- Place widget
+        table.insert(result, base.place_widget_at(v, x, y, w, h))
+
+        x = is_x and x + w + spacing or x
+        y = is_y and y + h + spacing or y
+
+        -- Add the spacing widget (if needed)
+        if k < widgets_nr and spacing_widget then
             table.insert(result, base.place_widget_at(
                 spacing_widget, is_x and (x - spoffset) or x, is_y and (y - spoffset) or y,
                 is_x and abspace or w, is_y and abspace or h
             ))
         end
-        table.insert(result, base.place_widget_at(v, x, y, w, h))
     end
+
     return result
 end
 
@@ -262,40 +278,61 @@ end
 -- @param orig_width The available width.
 -- @param orig_height The available height.
 function fixed:fit(context, orig_width, orig_height)
-    local width, height = orig_width, orig_height
+    local width_left, height_left = orig_width, orig_height
-    local used_in_dir, used_max = 0, 0
+    local spacing = self._private.spacing or 0
+    local widgets_nr = #self._private.widgets
+    local is_y = self._private.dir == "y"
+    local used_max = 0
 
-    for _, v in pairs(self._private.widgets) do
+    -- when no widgets exist the function can be called with orig_width or
-        local w, h = base.fit_widget(self, context, v, width, height)
+    -- orig_height equal to nil. Exit early in this case.
-        local in_dir, max
+    if widgets_nr == 0 then
-        if self._private.dir == "y" then
+        return 0, 0
-            max, in_dir = w, h
-            height = height - in_dir
-        else
-            in_dir, max = w, h
-            width = width - in_dir
     end
+
+    for k, v in pairs(self._private.widgets) do
+        local w, h = base.fit_widget(self, context, v, width_left, height_left)
+        local max
+
+        if is_y then
+            max = w
+            height_left = height_left - h
+        else
+            max = h
+            width_left = width_left - w
+        end
+
         if max > used_max then
             used_max = max
         end
-        used_in_dir = used_in_dir + in_dir
 
-        if width <= 0 or height <= 0 then
+        if k < widgets_nr then
-            if self._private.dir == "y" then
+            if is_y then
-                used_in_dir = orig_height
+                height_left = height_left - spacing
             else
-                used_in_dir = orig_width
+                width_left = width_left - spacing
+            end
+        end
+
+        if width_left <= 0 or height_left <= 0 then
+            -- this complicated two lines determine whether we're out-of-space
+            -- because of spacing, or if the last widget doesn't fit in
+            if is_y then
+                 height_left = k < widgets_nr and height_left + spacing or height_left
+                 height_left = height_left < 0 and 0 or height_left
+            else
+                 width_left = k < widgets_nr and width_left + spacing or width_left
+                 width_left = width_left < 0 and 0 or width_left
             end
             break
         end
     end
 
-    local spacing = self._private.spacing * (#self._private.widgets-1)
+    if is_y then
-
+        return used_max, orig_height - height_left
-    if self._private.dir == "y" then
-        return used_max, used_in_dir + spacing
     end
-    return used_in_dir + spacing, used_max
+
+    return orig_width - width_left, used_max
 end
 
 function fixed:reset()

spec/wibox/layout/fixed_spec.lua

@@ -39,6 +39,8 @@ describe("wibox.layout.fixed", function()
             layout:add(first, second, third)
         end)
 
+        describe("without spacing", function()
+
             describe("with enough space", function()
                 it("fit", function()
                     assert.widget_fit(layout, { 100, 100 }, { 15, 35 })
@@ -76,12 +78,109 @@ describe("wibox.layout.fixed", function()
                     assert.widget_layout(layout, { 100, 20 }, {
                         p(first,  0,  0, 100, 10),
                         p(second, 0, 10, 100, 10),
-                    p(third,  0, 20, 100,  0),
                     })
                 end)
             end)
         end)
 
+        describe("with spacing", function()
+            local spacing_widget = utils.widget_stub(10, 10)
+
+            before_each(function()
+                layout:set_spacing_widget(spacing_widget)
+            end)
+
+            describe(", positive spacing", function()
+                before_each(function()
+                    layout:set_spacing(10)
+                end)
+
+                describe("and with enough space", function()
+                    it("fit", function()
+                        assert.widget_fit(layout, { 100, 100 }, { 15, 55 })
+                    end)
+
+                    it("layout", function()
+                        assert.widget_layout(layout, { 100, 100 }, {
+                            p(first,  0, 0, 100, 10),
+                            p(spacing_widget,  0, 10, 100, 10),
+                            p(second, 0, 20, 100, 15),
+                            p(spacing_widget,  0, 35, 100, 10),
+                            p(third, 0, 45, 100, 10),
+                        })
+                    end)
+                end)
+
+                describe("and without enough space", function()
+                    it("fit", function()
+                        assert.widget_fit(layout, { 100, 45 }, { 15, 35 })
+                    end)
+
+                    it("layout", function()
+                        assert.widget_layout(layout, { 100, 35 }, {
+                            p(first,  0, 0, 100, 10),
+                            p(spacing_widget,  0, 10, 100, 10),
+                            p(second, 0, 20, 100, 15),
+                        })
+                    end)
+                end)
+            end) -- , positive spacing
+
+            describe(", negative spacing", function()
+                before_each(function()
+                    layout:set_spacing(-5)
+                end)
+
+                describe("and with more than needed space", function()
+                    it("fit", function()
+                        assert.widget_fit(layout, { 100, 100 }, { 15, 25 })
+                    end)
+
+                    it("layout", function()
+                        assert.widget_layout(layout, { 100, 100 }, {
+                            p(first,  0, 0, 100, 10),
+                            p(spacing_widget,  0, 5, 100, 5),
+                            p(second, 0, 5, 100, 15),
+                            p(spacing_widget,  0, 15, 100, 5),
+                            p(third, 0, 15, 100, 10),
+                        })
+                    end)
+                end)
+
+                describe("and with exactly the needed space", function()
+                    it("fit", function()
+                        assert.widget_fit(layout, { 15, 25 }, { 15, 25 })
+                    end)
+
+                    it("layout", function()
+                        assert.widget_layout(layout, { 15, 25 }, {
+                            p(first,  0, 0, 15, 10),
+                            p(spacing_widget,  0, 5, 15, 5),
+                            p(second, 0, 5, 15, 15),
+                            p(spacing_widget,  0, 15, 15, 5),
+                            p(third, 0, 15, 15, 10),
+                        })
+                    end)
+                end)
+
+
+                describe("and without enough space", function()
+                    it("fit", function()
+                        assert.widget_fit(layout, { 15, 20 }, { 15, 20 })
+                    end)
+
+                    it("layout", function()
+                        assert.widget_layout(layout, { 15, 20 }, {
+                            p(first,  0, 0, 15, 10),
+                            p(spacing_widget,  0, 5, 15, 5),
+                            p(second, 0, 5, 15, 15),
+                        })
+                    end)
+                end)
+            end) -- , negative spacing
+        end) -- with spacing
+    end) -- with widgets
+
     describe("emitting signals", function()
         local layout_changed
         before_each(function()