diff --git a/lib/awful/ewmh.lua b/lib/awful/ewmh.lua index d110cd31..c1f2d058 100644 --- a/lib/awful/ewmh.lua +++ b/lib/awful/ewmh.lua @@ -122,20 +122,41 @@ function ewmh.activate(c, context, hints) -- luacheck: no unused args end end +-- Get tags that are on the same screen as the client. This should _almost_ +-- always return the same content as c:tags(). +local function get_valid_tags(c, s) + local tags, new_tags = c:tags(), {} + + for _, t in ipairs(tags) do + if screen[s] == t.screen then + table.insert(new_tags, t) + end + end + + return new_tags +end + --- Tag a window with its requested tag. -- -- It is the default signal handler for `request::tag` on a `client`. -- -- @signalhandler awful.ewmh.tag -- @client c A client to tag --- @tag[opt] t A tag to use. If omitted, then the client is made sticky. +-- @tparam[opt] tag|boolean t A tag to use. If true, then the client is made sticky. -- @tparam[opt={}] table hints Extra information function ewmh.tag(c, t, hints) --luacheck: no unused -- There is nothing to do - if not t and #c:tags() > 0 then return end + if not t and #get_valid_tags(c, c.screen) > 0 then return end if not t then - c:to_selected_tags() + if c.transient_for then + c.screen = c.transient_for.screen + if not c.sticky then + c:tags(c.transient_for:tags()) + end + else + c:to_selected_tags() + end elseif type(t) == "boolean" and t then c.sticky = true else diff --git a/lib/awful/tag.lua b/lib/awful/tag.lua index 7be30a82..a0dd00a3 100644 --- a/lib/awful/tag.lua +++ b/lib/awful/tag.lua @@ -12,6 +12,7 @@ local util = require("awful.util") local ascreen = require("awful.screen") local beautiful = require("beautiful") local object = require("gears.object") +local timer = require("gears.timer") local pairs = pairs local ipairs = ipairs local table = table @@ -259,15 +260,14 @@ end -- stickied tags to. -- @tparam[opt=false] boolean force Move even non-sticky clients to the fallback -- tag. --- @return Returns true if the tag is successfully deleted, nil otherwise. +-- @return Returns true if the tag is successfully deleted. -- If there are no clients exclusively on this tag then delete it. Any -- stickied clients are assigned to the optional 'fallback_tag'. -- If after deleting the tag there is no selected tag, try and restore from -- history or select the first tag on the screen. function tag.object.delete(self, fallback_tag, force) - -- abort if the taf isn't currently activated - if not self.activated then return end + if not self.activated then return false end local target_scr = get_screen(tag.getproperty(self, "screen")) local tags = target_scr.tags @@ -275,7 +275,7 @@ function tag.object.delete(self, fallback_tag, force) local ntags = #tags -- We can't use the target tag as a fallback. - if fallback_tag == self then return end + if fallback_tag == self then return false end -- No fallback_tag provided, try and get one. if fallback_tag == nil then @@ -284,7 +284,7 @@ function tag.object.delete(self, fallback_tag, force) -- Abort if we would have un-tagged clients. local clients = self:clients() - if ( #clients > 0 and ntags <= 1 ) or fallback_tag == nil then return end + if #clients > 0 and fallback_tag == nil then return false end -- Move the clients we can off of this tag. for _, c in pairs(clients) do @@ -309,9 +309,11 @@ function tag.object.delete(self, fallback_tag, force) tag.setproperty(tags[i], "index", i-1) end - -- If no tags are visible, try and view one. - if target_scr.selected_tag == nil and ntags > 0 then - tag.history.restore(nil, 1) + -- If no tags are visible (and we did not delete the lasttag), try and + -- view one. The > 1 is because ntags is no longer synchronized with the + -- current count. + if target_scr.selected_tag == nil and ntags > 1 then + tag.history.restore(target_scr, 1) if target_scr.selected_tag == nil then local other_tag = tags[tags[1] == self and 2 or 1] if other_tag then @@ -465,6 +467,7 @@ function tag.object.set_screen(t, s) -- Change the screen tag.setproperty(t, "screen", s) + tag.setproperty(t, "index", #s:get_tags(true)) -- Make sure the client's screen matches its tags for _,c in ipairs(t:clients()) do @@ -473,10 +476,8 @@ function tag.object.set_screen(t, s) end -- Update all indexes - for _,screen in ipairs {old_screen, s} do - for i,t2 in ipairs(screen.tags) do - tag.setproperty(t2, "index", i) - end + for i,t2 in ipairs(old_screen.tags) do + tag.setproperty(t2, "index", i) end -- Restore the old screen history if the tag was selected @@ -1277,21 +1278,25 @@ function tag.attached_connect_signal(screen, ...) end -- Register standard signals. -capi.client.connect_signal("manage", function(c) - -- If we are not managing this application at startup, - -- move it to the screen where the mouse is. - -- We only do it for "normal" windows (i.e. no dock, etc). - if not awesome.startup and c.type ~= "desktop" and c.type ~= "dock" then - if c.transient_for then - c.screen = c.transient_for.screen - if not c.sticky then - c:tags(c.transient_for:tags()) +capi.client.connect_signal("property::screen", function(c) + -- First, the delayed timer is necessary to avoid a race condition with + -- awful.rules. It is also messing up the tags before the user have a chance + -- to set them manually. + timer.delayed_call(function() + local tags, new_tags = c:tags(), {} + + for _, t in ipairs(tags) do + if t.screen == c.screen then + table.insert(new_tags, t) end - else - c.screen = ascreen.focused() end - end - c:connect_signal("property::screen", function() c:to_selected_tags() end) + + if #new_tags == 0 then + c:emit_signal("request::tag", nil, {reason="screen"}) + elseif #new_tags < #tags then + c:tags(new_tags) + end + end) end) -- Keep track of the number of urgent clients. diff --git a/lib/wibox/drawable.lua b/lib/wibox/drawable.lua index ddeba52c..0212efae 100644 --- a/lib/wibox/drawable.lua +++ b/lib/wibox/drawable.lua @@ -59,6 +59,8 @@ local function get_widget_context(self) end local function do_redraw(self) + if type(self.drawable) ~= "drawable" then return end --FIXME See #1070 + local surf = surface.load_silently(self.drawable.surface, false) -- The surface can be nil if the drawable's parent was already finalized if not surf then return end diff --git a/objects/client.c b/objects/client.c index 838112d1..7a6cfa67 100644 --- a/objects/client.c +++ b/objects/client.c @@ -2304,6 +2304,10 @@ luaA_client_swap(lua_State *L) * * Use the `first_tag` field to access the first tag of a client directly. * + * **Signal:** + * + * * *property::tags* + * * @tparam table tags_table A table with tags to set, or `nil` to get the * current tags. * @treturn table A table with all tags. @@ -2342,7 +2346,10 @@ luaA_client_tags(lua_State *L) lua_pushnil(L); while(lua_next(L, 2)) tag_client(L, c); + lua_pop(L, 1); + + luaA_object_emit_signal(L, -1, "property::tags", 0); } lua_newtable(L); diff --git a/tests/_multi_screen.lua b/tests/_multi_screen.lua new file mode 100644 index 00000000..fab8f367 --- /dev/null +++ b/tests/_multi_screen.lua @@ -0,0 +1,512 @@ +-- Helper module to replicate a set of steps across multiple screen scenarios +-- Ideas for improvements: +-- * Export an iterator so the suits can call the screen change step by step +-- themselves instead of the current all or nothing +-- * Offer multiple scenarios such as screen add, remove, resize/swap instead +-- of having to test all corner cases all the time. +local wibox = require("wibox") +local surface = require("gears.surface") +local wallpaper = require("gears.wallpaper") +local color = require("gears.color") +local shape = require("gears.shape") + +local module = {} + +-- Get the original canvas size before it is "lost" +local canvas_w, canvas_h = root.size() + +local half_w, half_h = math.floor(canvas_w/2), math.floor(canvas_h/2) +local quarter_w = math.floor(canvas_w/4) + +-- Create virtual screens. +-- The steps will be executed on each set of screens. +local dispositions = { + -- A simple classic, 2 identical screens side by side + { + function() + return { + x = 0, + y = 0, + width = half_w, + height = canvas_h, + } + end, + function() + return { + x = half_w, + y = 0, + width = half_w, + height = canvas_h, + } + end, + }, + + -- Take the previous disposition and swap the screens + { + function() + return { + x = half_w, + y = 0, + width = half_w, + height = canvas_h, + } + end, + function() + return { + x = 0, + y = 0, + width = half_w, + height = canvas_h, + } + end, + keep = true, + }, + + -- Take the previous disposition and resize the leftmost one + { + function() + return { + x = quarter_w, + y = 0, + width = 3*quarter_w, + height = canvas_h, + } + end, + function() + return { + x = 0, + y = 0, + width = quarter_w, + height = half_h, + } + end, + keep = true, + }, + + -- Take the previous disposition and remove the leftmost screen + { + function() + return { + x = quarter_w, + y = 0, + width = 3*quarter_w, + height = canvas_h, + } + end, + function() + return nil + end, + keep = true, + }, + + -- Less used, but still quite common vertical setup + { + function() + return { + x = 0, + y = 0, + width = canvas_w, + height = half_h, + } + end, + function() + return { + x = 0, + y = half_h, + width = canvas_w, + height = half_h, + } + end + }, + + -- Another popular setup, 22" screens with a better 31" in the middle. + -- So, 2 smaller vertical screen with a larger central horizontal one. + { + -- Left + function() + return { + x = 0, + y = 0, + width = quarter_w, + height = canvas_h, + } + end, + -- Right, this test non-continous screen index on purpose, as this setup + -- Often requires dual-GPU, it _will_ happen. + function() + return { + x = canvas_w-quarter_w, + y = 0, + width = quarter_w, + height = canvas_h, + } + end, + -- Center + function() + return { + x = quarter_w, + y = 0, + width = half_w, + height = math.floor(canvas_h*(9/16)), + } + end, + }, + + -- Same as the previous one, but with the gap centered + { + -- Left + function() + return { + x = 0, + y = 0, + width = quarter_w, + height = canvas_h, + } + end, + -- Right, this test non-continous screen index on purpose, as this setup + -- Often requires dual-GPU, it _will_ happen. + function() + return { + x = canvas_w-quarter_w, + y = 0, + width = quarter_w, + height = canvas_h, + } + end, + -- Center + function() + return { + x = quarter_w, + y = math.ceil((canvas_h-(canvas_h*(9/16)))/2), + width = half_w, + height = math.floor(canvas_h*(9/16)), + } + end, + + -- Keep the same screens as the last test, just move them + keep = true, + }, + + -- AMD Eyefinity / (old) NVIDIA MOSAIC style config (symmetric grid) + { + function() + return { + x = 0, + y = 0, + width = half_w, + height = half_h, + } + end, + function() + return { + x = half_w, + y = 0, + width = half_w, + height = half_h, + } + end, + function() + return { + x = 0, + y = half_h, + width = half_w, + height = half_h, + } + end, + function() + return { + x = half_w, + y = half_h, + width = half_w, + height = half_h, + } + end, + }, + + -- Corner case 1: Non-continuous screens + -- If there is nothing and client is drag&dropped into that area, some + -- geometry callbacks may break in nil index. + { + function() + return { + x = 0, + y = 0, + width = quarter_w, + height = canvas_h, + } + end, + function() + return { + x = canvas_w-quarter_w, + y = 0, + width = quarter_w, + height = canvas_h, + } + end, + }, + + -- Corner case 2: Nothing at 0x0. + -- As some position may fallback to 0x0 this need to be tested often. It + -- also caused issues such as #154 + { + function() + return { + x = 0, + y = half_w, + width = half_w, + height = half_w, + } + end, + function() + return { + x = half_w, + y = 0, + width = half_w, + height = canvas_h, + } + end + }, + + -- Corner case 3: Many very small screens. + -- On the embedded side of the compuverse, it is possible + -- to buy 32x32 RGB OLED screens. They are usually used to display single + -- status icons, but why not use them as a desktop! This is a critical + -- market for AwesomeWM. Here's a 256px wide strip of tiny screens. + -- This may cause wibars to move to the wrong screen accidentally + { + function() return { x = 0 , y = 0, width = 32, height = 32, } end, + function() return { x = 32 , y = 0, width = 32, height = 32, } end, + function() return { x = 64 , y = 0, width = 32, height = 32, } end, + function() return { x = 96 , y = 0, width = 32, height = 32, } end, + function() return { x = 128, y = 0, width = 32, height = 32, } end, + function() return { x = 160, y = 0, width = 32, height = 32, } end, + function() return { x = 192, y = 0, width = 32, height = 32, } end, + function() return { x = 224, y = 0, width = 32, height = 32, } end, + }, + + -- Corner case 4: A screen taller than more than 1 other screen. + -- this may cause some issues with client resize and drag&drop move + { + function() + return { + x = 0, + y = 0, + width = half_w, + height = canvas_h, + } + end, + function() + return { + x = half_w, + y = 0, + width = half_w, + height = math.floor(canvas_h/3), + } + end, + function() + return { + x = half_w, + y = math.floor(canvas_h/3), + width = half_w, + height = math.floor(canvas_h/3), + } + end, + function() + return { + x = half_w, + y = 2*math.floor(canvas_h/3), + width = half_w, + height = math.floor(canvas_h/3), + } + end + }, + + -- The overlapping corner case isn't supported. There is valid use case, + -- such as a projector with a smaller resolution than the laptop screen + -- in non-scaling mirror mode, but it isn't worth the complexity it brings. +} + +local function check_tag_indexes() + for s in screen do + for i, t in ipairs(s.tags) do + assert(t.index == i) + assert(t.screen == s) + end + end +end + +local colors = { + "#000030", + "#300000", + "#043000", + "#302E00", + "#002C30", + "#300030", + "#301C00", + "#140030", +} + +-- Paint it black +local function clear_screen() + for s in screen do + local sur = surface.widget_to_surface( + wibox.widget { + bg = "#000000", + widget = wibox.container.background + }, + s.geometry.width, + s.geometry.height + ) + wallpaper.fit(sur, s, "#000000") + end +end + +-- Make it easier to debug the tests by showing the screen geometry when the +-- tests are executed. +local function show_screens() + wallpaper.set(color("#000000")) -- Should this clear the wallpaper? It doesn't + + -- Add a wallpaper on each screen + for i=1, screen.count() do + local s = screen[i] + + local w = wibox.widget { + { + text = table.concat{ + "Screen: ",i,"\n", + s.geometry.width,"x",s.geometry.height, + "+",s.geometry.x,",",s.geometry.y + }, + valign = "center", + align = "center", + widget = wibox.widget.textbox, + }, + bg = colors[i], + fg = "#ffffff", + shape_border_color = "#ff0000", + shape_border_width = 1, + shape = shape.rectangle, + widget = wibox.container.background + } + local sur = surface.widget_to_surface( + w, + s.geometry.width, + s.geometry.height + ) + wallpaper.fit(sur, s) + end +end + +local function add_steps(real_steps, new_steps) + for _, dispo in ipairs(dispositions) do + -- Cleanup + table.insert(real_steps, function() + if #client.get() == 0 then return true end + + for _, c in ipairs(client.get()) do + c:kill() + end + end) + + table.insert(real_steps, function() + clear_screen() + local keep = dispo.keep or false + local old = {} + local geos = {} + + check_tag_indexes() + + if keep then + for _, sf in ipairs(dispo) do + local geo = sf and sf() or nil + + -- If the function return nothing, assume the screen need to + -- be destroyed. + table.insert(geos, geo or false) + end + + -- Removed screens need to be explicit. + assert(#geos >= screen.count()) + + -- Keep a cache to avoid working with invalid data + local old_screens = {} + for s in screen do + table.insert(old_screens, s) + end + + for i, s in ipairs(old_screens) do + -- Remove the screen (if no new geometry is given + if not geos[i] then + s:fake_remove() + else + local cur_geo = s.geometry + for _, v in ipairs {"x", "y", "width", "height" } do + cur_geo[v] = geos[i][v] or cur_geo[v] + end + s:fake_resize( + cur_geo.x, + cur_geo.y, + cur_geo.width, + cur_geo.height + ) + end + end + + -- Add the remaining screens + for i=#old_screens + 1, #geos do + local geo = geos[i] + screen.fake_add(geo.x, geo.y, geo.width, geo.height) + end + + check_tag_indexes() + else + -- Move all existing screens out of the way (to avoid temporary overlapping) + for s in screen do + s:fake_resize(canvas_w, canvas_h, 1, 1) + table.insert(old, s) + end + + -- Add the new screens + for _, sf in ipairs(dispo) do + local geo = sf() + screen.fake_add(geo.x, geo.y, geo.width, geo.height) + table.insert(geos, geo) + end + + -- Remove old screens + for _, s in ipairs(old) do + s:fake_remove() + end + end + + show_screens() + + -- Check the result is correct + local expected_count = 0 + for _,v in ipairs(geos) do + expected_count = expected_count + (v and 1 or 0) + end + + assert(expected_count == screen.count()) + for k, geo in ipairs(geos) do + if geo then + local sgeo = screen[k].geometry + assert(geo.x == sgeo.x) + assert(geo.y == sgeo.y) + assert(geo.width == sgeo.width ) + assert(geo.height == sgeo.height) + end + end + + return true + end) + + for _, step in ipairs(new_steps) do + table.insert(real_steps, step) + end + end +end + +return setmetatable(module, { + __call = function(_,...) return add_steps(...) end +}) diff --git a/tests/run.sh b/tests/run.sh index 51702184..b04d62c6 100755 --- a/tests/run.sh +++ b/tests/run.sh @@ -194,7 +194,7 @@ start_awesome() { # Count errors. errors=0 # Seconds after when awesome gets killed. -timeout_stale=60 +timeout_stale=120 # FIXME This should be no more than 60s for f in $tests; do echo "== Running $f ==" diff --git a/tests/test-awful-client.lua b/tests/test-awful-client.lua index ca36318c..93155bcc 100644 --- a/tests/test-awful-client.lua +++ b/tests/test-awful-client.lua @@ -1,4 +1,5 @@ local awful = require("awful") +local test_client = require("_client") awful.util.deprecate = function() end @@ -64,9 +65,6 @@ assert(c:tags()[1] == t2 or c:tags()[2] == t2) awful.client.toggletag(t2, c) assert(c:tags()[1] == t and c:tags()[1] ~= t2 and c:tags()[2] == nil) --- Test movetoscreen ---FIXME I don't have the hardware to test - -- Test floating assert(c.floating ~= nil and type(c.floating) == "boolean") c.floating = true @@ -79,4 +77,87 @@ end end } +local has_error + +table.insert(steps, function() + -- Make sure there is no extra callbacks that causes double screen changes + -- (regress #1028 #1052) + client.connect_signal("property::screen",function(c) + if c.class and c.class ~= "screen"..c.screen.index then + has_error = table.concat { + "An unexpected screen change did occur\n", + debug.traceback(),"\n", + c.class, " Screen: ",c.screen.index + } + end + end) + + return true +end) + +local multi_screen_steps = {} + +-- Add a test client on each screen. +table.insert(multi_screen_steps, function() + has_error = false + for i=1, screen.count() do + local s = screen[i] + test_client("screen"..i, nil, { + screen = s, + }) + end + + return true +end) + +-- Test if the clients have been placed on the right screen +table.insert(multi_screen_steps, function() + if #client.get() ~= screen.count() then return end + + if has_error then print(has_error) end + assert(not has_error) + + for i=1, screen.count() do + local s = screen[i] + + -- Just in case + for _, t in ipairs(s.selected_tags) do + assert(t.screen == s) + end + + local t = s.selected_tag + assert(t and t.screen == s) + + -- In case the next step has failed, find what went wrong + for _, c in ipairs(t:clients()) do + -- This is _supposed_ to be impossible, but can be forced by buggy + -- lua code. + if c.screen ~= t.screen then + local tags = c:tags() + for _, t2 in ipairs(tags) do + assert(t2.screen == c.screen) + end + + assert(false) + end + assert(c.screen.index == i) + end + + -- Check that the right client is placed, the loop above will trigger + -- asserts already, but in case the tag has more than 1 client and + -- everything is messed up, those asserts are still necessary. + assert(#t:clients() == 1) + assert(t:clients()[1].class == "screen"..i) + assert(t:clients()[1].screen == s) + + -- Make sure it stays where it belongs + awful.placement.maximize(t:clients()[1]) + assert(t:clients()[1] and t:clients()[1].screen == t.screen) + end + + return true +end) + +require("_multi_screen")(steps, multi_screen_steps) + require("_runner").run_steps(steps) diff --git a/tests/test-awful-rules.lua b/tests/test-awful-rules.lua index 7dc871a2..dd136d16 100644 --- a/tests/test-awful-rules.lua +++ b/tests/test-awful-rules.lua @@ -11,6 +11,13 @@ local tests = {} local tb_height = awful.util.round(beautiful.get_font_height() * 1.5) -- local border_width = beautiful.border_width +-- Detect "manage" race conditions +local real_apply = awful.rules.apply +function awful.rules.apply(c) + assert(#c:tags() == 0) + return real_apply(c) +end + local function test_rule(rule) rule.rule = rule.rule or {} diff --git a/tests/test-awful-tag.lua b/tests/test-awful-tag.lua index 3f684ad6..d58a9898 100644 --- a/tests/test-awful-tag.lua +++ b/tests/test-awful-tag.lua @@ -128,4 +128,117 @@ end end } +local multi_screen_steps = {} + +-- Clear the state (this step will be called once per disposition) +table.insert(multi_screen_steps, function() + if #client.get() == 0 then + -- Remove all existing tags created by the rc.lua callback + for s in screen do + while #s.tags > 0 do + local t = s.tags[1] + -- Test #1051 against regressions + assert(t.index == 1) + + assert(t.screen == s) + + t:delete() + assert(not t.activated) + assert(t.screen == nil) + end + end + + return true + end + + for _, c in ipairs(client.get()) do + c:kill() + end +end) + +-- Add a bunch of tags on each screens. +table.insert(multi_screen_steps, function() + for i=1, screen.count() do + local s = screen[i] + -- Add 5 tags per screen + for idx=1, 5 do + awful.tag.add("S"..i..":"..idx,{screen = s}) + end + end + + -- Check if everything adds up + for i=1, screen.count() do + local tags = screen[i].tags + assert(#tags == 5) + + for k, t in ipairs(tags) do + assert(t.index == k) + assert(t.name == "S"..i..":"..k) + end + end + + -- Select a tag on each screen + for i=1, screen.count() do + screen[i].tags[1].selected = true + end + + return true +end) + +local function check_tag_indexes() + for s in screen do + for i, t in ipairs(s.tags) do + assert(t.index == i) + assert(t.screen == s) + end + end +end + +-- Move tags across screens (without clients) +table.insert(multi_screen_steps, function() + -- This screen disposition cannot be used for this test, move on + if screen.count() < 2 then return true end + + local s1, s2 = screen[1], screen[2] + + local old_count1, old_count2 = #s1.tags, #s2.tags + + local t = awful.tag.add("TEST", {screen=s1}) + + -- Check the current state + assert( t.index == old_count1+1 ) + assert( t.screen == s1 ) + assert( #s1.tags == old_count1+1 ) + assert( s1.tags[old_count1+1] == t ) + assert( #s2.tags == old_count2 ) + + -- Move to another index + local new_index = 3 + t.index = new_index + assert(t.index == new_index) + check_tag_indexes() + + -- Move the tag to screen 2 + t.screen = s2 + assert(t.screen == s2 ) + assert(#s1.tags == old_count1 ) + assert(#s2.tags == old_count2+1 ) + assert( s2.tags[old_count2+1] == t ) + assert( t.index == old_count2+1 ) + check_tag_indexes() + + -- Move to another index + t.index = new_index + assert(t.index == new_index) + check_tag_indexes() + + -- Delete it + t:delete() + check_tag_indexes() + + return true +end) + +require("_multi_screen")(steps, multi_screen_steps) + require("_runner").run_steps(steps)