From a158c3adab565812509b7812365e6520335006de Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Mon, 13 Feb 2017 16:08:30 -0500 Subject: [PATCH 1/2] wibar: Fix a race condition when adding new wibars Fix #1523 --- lib/awful/wibar.lua | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/lib/awful/wibar.lua b/lib/awful/wibar.lua index fccc444bd..bfd081790 100644 --- a/lib/awful/wibar.lua +++ b/lib/awful/wibar.lua @@ -108,7 +108,7 @@ local function get_position(wb) return wb._position or "top" end -local function set_position(wb, position) +local function set_position(wb, position, skip_reattach) -- Detach first to avoid any uneeded callbacks if wb.detach_callback then wb.detach_callback() @@ -137,16 +137,20 @@ local function set_position(wb, position) wb.width = math.ceil(beautiful.get_font_height(wb.font) * 1.5) end - -- Changing the position will also cause the other margins to be invalidated. - -- For example, adding a wibar to the top will change the margins of any left - -- or right wibars. To solve, this, they need to be re-attached. - reattach(wb) - -- Set the new position wb._position = position -- Attach to the new position attach(wb, position) + + -- A way to skip reattach is required when first adding a wibar as it's not + -- in the `wiboxes` table yet and can't be added until it's attached. + if not skip_reattach then + -- Changing the position will also cause the other margins to be invalidated. + -- For example, adding a wibar to the top will change the margins of any left + -- or right wibars. To solve, this, they need to be re-attached. + reattach(wb) + end end --- Stretch the wibar. @@ -350,10 +354,16 @@ function awfulwibar.new(arg) if arg.visible == nil then w.visible = true end - w:set_position(position) + -- `w` needs to be inserted in `wiboxes` before reattach or its own offset + -- will not be taken into account by the "older" wibars when `reattach` is + -- called. `skip_reattach` is required. + w:set_position(position, true) table.insert(wiboxes, w) + -- Force all the wibars to be moved + reattach(w) + w:connect_signal("property::visible", function() reattach(w) end) return w From e221f8414cfbce9ae0992445d89b77bf16dfc54b Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Mon, 13 Feb 2017 16:08:59 -0500 Subject: [PATCH 2/2] tests: Validate wibar geometry in the struts test Prevent #1523 from regressing --- tests/test-struts.lua | 145 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 145 insertions(+) diff --git a/tests/test-struts.lua b/tests/test-struts.lua index bed5ceb59..5e4e17da1 100644 --- a/tests/test-struts.lua +++ b/tests/test-struts.lua @@ -9,6 +9,49 @@ local parent, small local twibar, bwibar, lwibar, rwibar = screen.primary.mywibox +-- Pretty print issues +local function print_expected() + local wa, sa = mouse.screen.workarea, mouse.screen.geometry + local ret = table.concat{ + "\nSCREEN: ", sa.x, " ", sa.y," ", sa.width, " ", sa.height + } + ret = table.concat{ret, + "\nWORKAREA: ", wa.x, " ", wa.y," ", wa.width, " ", wa.height + } + ret = table.concat{ret, + "\nSTRUTS: l:", + wa.x, " t:", + wa.y," r:", + sa.width - wa.width - wa.x, " b:", + sa.height - wa.height - wa.y + } + + for k, w in ipairs {lwibar, rwibar, twibar, bwibar} do + ret = table.concat{ + ret , "\n " , k, ": ", w.position, " [", + w.width, ", ", + w.height , + "]" + } + end + return ret +end + +-- All wibars on top and bottom are full width and all on left and right +-- must honor the workarea height +local function validate_wibar_geometry() + for _, w in ipairs {lwibar, rwibar, twibar, bwibar} do + assert(w and w.position and w.screen) + if w.visible then + if w.position == "top" or w.position == "bottom" then + assert(w.width == w.screen.geometry.width, print_expected()) + else + assert(w.height == w.screen.workarea.height, print_expected()) + end + end + end +end + -- Test the struts without using wibars table.insert(steps, function() local sgeo = screen.primary.geometry @@ -99,6 +142,9 @@ table.insert(steps, function() assert(parent:geometry().width == small:geometry().width ) assert(small:geometry().height == 24) + -- Hide (and hopefully GC) the old "small" + small.visible = false + -- Do the same, but with placement compositing small = wibox { bg = "#ff0000", @@ -174,6 +220,8 @@ table.insert(steps, function() lwibar = wibar {position = "left" , bg = "#0000ff"} rwibar = wibar {position = "right" , bg = "#ff00ff"} + validate_wibar_geometry() + return true end) @@ -254,6 +302,8 @@ table.insert(steps, function() assert(lwibar.position == "top") assert(lwibar.y == twibar.height) + validate_wibar_geometry() + return true end) @@ -265,6 +315,8 @@ table.insert(steps, function() rwibar.ontop = true assert(bwibar.position == "right") + validate_wibar_geometry() + return true end) @@ -273,6 +325,8 @@ table.insert(steps, function() rwibar.position = "top" + validate_wibar_geometry() + return true end) @@ -281,6 +335,8 @@ table.insert(steps, function() bwibar.position = "top" + validate_wibar_geometry() + return true end) @@ -289,8 +345,10 @@ table.insert(steps, function() for _, w in ipairs {twibar, lwibar, rwibar, bwibar} do w.position = "right" + validate_wibar_geometry() end + return true end) @@ -364,29 +422,116 @@ table.insert(steps, function() -- Test some simple struts c:struts { left = 50 } test_workarea(c.screen.geometry, c.screen.workarea, 50, 0, 0, 0) + validate_wibar_geometry() -- A tag switch should make the client no longer apply screen.primary.tags[2]:view_only() test_workarea(c.screen.geometry, c.screen.workarea, 0, 0, 0, 0) + validate_wibar_geometry() -- But sticky clients always 'count' c.sticky = true test_workarea(c.screen.geometry, c.screen.workarea, 50, 0, 0, 0) + validate_wibar_geometry() c.sticky = false test_workarea(c.screen.geometry, c.screen.workarea, 0, 0, 0, 0) + validate_wibar_geometry() -- And switch back to the right tag c.first_tag:view_only() test_workarea(c.screen.geometry, c.screen.workarea, 50, 0, 0, 0) + validate_wibar_geometry() -- What if we move the client to another tag? c:tags{ screen.primary.tags[2] } test_workarea(c.screen.geometry, c.screen.workarea, 0, 0, 0, 0) + validate_wibar_geometry() -- Move it back to the selected tag c:tags{ screen.primary.tags[1] } test_workarea(c.screen.geometry, c.screen.workarea, 50, 0, 0, 0) + validate_wibar_geometry() + + return true +end) + +local s + +table.insert(steps, function() + c = client.get()[1] + assert(c) + s = c.screen + c:kill() + + bwibar:remove() + lwibar:remove() + rwibar:remove() + twibar:remove() + + return true +end) + +table.insert(steps, function() + if client.get()[1] then + return + end + + test_workarea(s.geometry, s.workarea, 0, 0, 0, 0) + validate_wibar_geometry() + + local wdg = { + layout = wibox.layout.align.vertical, + wibox.widget.textbox("BEGIN"), + nil, + wibox.widget.textbox("END") + } + + local wdg2 = { + layout = wibox.layout.align.horizontal, + wibox.widget.textbox("BEGIN"), + nil, + wibox.widget.textbox("END") + } + + lwibar = wibar{position = "top", screen = s, height = 15, + visible = true, bg = "#660066"} + lwibar:setup(wdg2) + + rwibar = wibar{position = "top", screen = s, height = 15, + visible = true, bg = "#660000"} + rwibar:setup(wdg2) + + bwibar = wibar{position = "left", screen = s, ontop = true, width = 64, + visible = true, bg = "#006600"} + bwibar:setup(wdg) + + twibar = wibar{position = "bottom", screen = s, + height = 15, bg = "#666600"} + twibar:setup(wdg2) + + test_workarea(s.geometry, s.workarea, 64, 0, 30, 15) + validate_wibar_geometry() + + bwibar:remove() + + test_workarea(s.geometry, s.workarea, 0, 0, 30, 15) + validate_wibar_geometry() + + lwibar:remove() + + test_workarea(s.geometry, s.workarea, 0, 0, 15, 15) + validate_wibar_geometry() + + rwibar:remove() + + test_workarea(s.geometry, s.workarea, 0, 0, 0, 15) + validate_wibar_geometry() + + twibar:remove() + + test_workarea(s.geometry, s.workarea, 0, 0, 0, 0) + validate_wibar_geometry() return true end)