From 89c84caee4fcadc80ff34f3ad319339dc1c89d05 Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Mon, 20 Sep 2021 15:53:51 -0700 Subject: [PATCH 01/10] naughty.legacy: Explicitly remove the screen. Do not rely on the GC. It isn't reliable with Luajit. --- lib/naughty/layout/legacy.lua | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/naughty/layout/legacy.lua b/lib/naughty/layout/legacy.lua index ab8d3842..a1999caa 100644 --- a/lib/naughty/layout/legacy.lua +++ b/lib/naughty/layout/legacy.lua @@ -62,6 +62,12 @@ screen.connect_for_each_screen(function(s) } end) +capi.screen.connect_signal("removed", function(s) + timer.delayed_call(function() + current_notifications[s] = nil + end) +end) + --- Sum heights of notifications at position -- -- @param s Screen to use From 21a111d154c3595c8d7e9d74b43fd6321aaa0cd9 Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Wed, 22 Sep 2021 19:15:33 -0700 Subject: [PATCH 02/10] naughty: Allow to set the message from the presets again. This was never really supported and is really not what the presets were event meant to be used for. But it worked and `lain` used it. --- lib/naughty/notification.lua | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/lib/naughty/notification.lua b/lib/naughty/notification.lua index 4e602a52..860eec08 100644 --- a/lib/naughty/notification.lua +++ b/lib/naughty/notification.lua @@ -570,6 +570,27 @@ function notification:set_timeout(timeout) self:emit_signal("property::timeout", timeout) end +function notification:get_message() + -- This property was called "text" in older versions. + -- Some modules like `lain` abused of the presets (they + -- had little choice at the time) to set the message on + -- an existing popup. + local p = rawget(self, "preset") or {} + local message = self._private.message or p.message or "" + + if message == "" and p.text and p.text ~= "" then + gdebug.deprecate( + "Using the preset configuration to set the notification ".. + "message is not supported. Please use `n.message = 'text'`.", + {deprecated_in=5} + ) + + return p.text + end + + return message +end + function notification:set_text(txt) gdebug.deprecate( "The `text` attribute is deprecated, use `message`", From e752943b985b0bf30942a4c891e73c6eebf1c475 Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Thu, 16 Sep 2021 16:26:07 -0700 Subject: [PATCH 03/10] notification: Correctly handle `screen` set in the presets. It was setting directly to `_private`, which both didn't work and caused a memory leak. Fix #3428 --- lib/naughty/notification.lua | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/naughty/notification.lua b/lib/naughty/notification.lua index 860eec08..5ef46c31 100644 --- a/lib/naughty/notification.lua +++ b/lib/naughty/notification.lua @@ -882,7 +882,15 @@ local function select_legacy_preset(n, args) )) for k, v in pairs(n.preset) do - n._private[k] = v + -- Don't keep a strong reference to the screen, Lua 5.1 GC wont be + -- smart enough to unwind the mess of circular weak references. + if k ~= "screen" then + n._private[k] = v + end + end + + if n.preset.screen then + n._private.weak_screen[1] = capi.screen[n.preset.screen] end end From 9336b62f80cbba466f6e1fa482042c5aa2cb0bba Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Thu, 16 Sep 2021 16:37:20 -0700 Subject: [PATCH 04/10] ruled.notification: Handle legacy presets. This is deprecated, but some modules like `lain` use them, so it must still minimally work for backward compatibility. --- lib/ruled/notification.lua | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/lib/ruled/notification.lua b/lib/ruled/notification.lua index f76cbc4f..de6b306e 100644 --- a/lib/ruled/notification.lua +++ b/lib/ruled/notification.lua @@ -160,6 +160,15 @@ end -- @staticfct ruled.notification.apply function module.apply(n) local callbacks, props = {}, {} + + if n.preset then + for k, v in pairs(n.preset) do + if not n._private[v] then + props[k] = v + end + end + end + for _, v in ipairs(nrules._matching_source) do v.callback(nrules, n, props, callbacks) end From 552b2a22d14f1cdfe1c4b5d6b21c13f65917d526 Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Thu, 16 Sep 2021 16:40:15 -0700 Subject: [PATCH 05/10] tests: Test the notification presets. Make sure they behave the same in legacy and ruled modes. --- tests/test-naughty-preset.lua | 85 +++++++++++++++++++ tests/test-naughty-screen.lua | 155 ++++++++++++++++++---------------- 2 files changed, 166 insertions(+), 74 deletions(-) create mode 100644 tests/test-naughty-preset.lua diff --git a/tests/test-naughty-preset.lua b/tests/test-naughty-preset.lua new file mode 100644 index 00000000..df63dd2e --- /dev/null +++ b/tests/test-naughty-preset.lua @@ -0,0 +1,85 @@ +local naughty = require("naughty") +local notification = require("naughty.notification") +require("ruled.notification"):_clear() + +local steps = {} + +local BAD_IDEA = "never do this in practice, compat only" + +local notif = setmetatable({}, {__mode="v"}) + +screen[1]:split() + +for _, legacy_preset in ipairs {true, false} do + table.insert(steps, function() + -- This will either take the legacy preset path + -- or the ruled preset path. + function naughty.get__has_preset_handler() + return legacy_preset + end + + local custom_preset = { + bg = "#00ff00", + fg = "#ff0000", + text = BAD_IDEA + } + + notif[1] = notification { + preset = custom_preset, + } + + assert(notif[1].bg == "#00ff00") + assert(notif[1].message == BAD_IDEA) + + return true + end) + + table.insert(steps, function() + notif[1]:destroy() + + return true + end) + + for s in screen do + -- Make sure the screen doesn't cause a memory leak. + table.insert(steps, function() + collectgarbage("collect") + + if notif[1] then return end + + local custom_preset = { + bg = "#0000ff", + fg = "#ff0000", + screen = s + } + + notif[1] = notification { + preset = custom_preset, + title = "test", + } + + assert(notif[1].bg == "#0000ff") + + assert(notif[1].screen == s) + + return true + end) + + table.insert(steps, function() + assert(notif[1].screen == s) + notif[1]:destroy() + + return true + end) + + table.insert(steps, function() + collectgarbage("collect") + + if notif[1] then return end + + return true + end) + end +end + +require("_runner").run_steps(steps) diff --git a/tests/test-naughty-screen.lua b/tests/test-naughty-screen.lua index 4ada5b7b..7428a462 100644 --- a/tests/test-naughty-screen.lua +++ b/tests/test-naughty-screen.lua @@ -71,90 +71,97 @@ local function check_screen(s) end end --- Create notifications in each position. -table.insert(steps, function() - rnotif._clear() - add_many(s1) +for _, legacy_preset in ipairs {true, false} do - return true -end) - --- Make sure removing notification works. -table.insert(steps, function() - - remove_at(s1, 2) - - -- Split the screen - s1:split() - - s2 = screen[2] - assert(s1 ~= s2) - - return true -end) - --- Make sure the notification moved as the screen shrunk. -table.insert(steps, function() - check_screen(s1) - - -- Make sure we can still remove them without errors. - remove_at(s1, 2) - - -- Add more! - add_many(s2) - - -- Make sure none got moved to the wrong position due to a fallback code - -- path triggered by accident. The first few iteration were prone to this. - check_screen(s1) - check_screen(s2) - - return true -end) - --- Remove everything and see what happens. -table.insert(steps, function() - - for _=1, 3 do - for _, s in ipairs {s1,s2} do - remove_at(s, 1) + -- Create notifications in each position. + table.insert(steps, function() + function naughty.get__has_preset_handler() + return legacy_preset end - end - for _=1, 2 do - remove_at(s2, 1) - end + rnotif._clear() + add_many(s1) - -- And add them again. - add_many(s1) - add_many(s2) + return true + end) - return true -end) + -- Make sure removing notification works. + table.insert(steps, function() ---local weak = nil --FIXME + remove_at(s1, 2) --- Delete a screen and make sure it gets GCed. -table.insert(steps, function() - s2:fake_remove() + -- Split the screen + s1:split() - -- Help the weak tables a little. - for _, pos in ipairs(positions) do - objs[pos][s1] = nil - end + s2 = screen[2] + assert(s1 ~= s2) - -- Drop our string reference to s2. - --weak, s2 = setmetatable({s2}, {__mode="v"}), nil --FIXME + return true + end) - return true -end) + -- Make sure the notification moved as the screen shrunk. + table.insert(steps, function() + check_screen(s1) ---FIXME ---table.insert(steps, function() --- if weak[1] == nil then return true end --- --- for _=1, 10 do --- collectgarbage("collect") --- end ---end) + -- Make sure we can still remove them without errors. + remove_at(s1, 2) + + -- Add more! + add_many(s2) + + -- Make sure none got moved to the wrong position due to a fallback code + -- path triggered by accident. The first few iteration were prone to this. + check_screen(s1) + check_screen(s2) + + return true + end) + + -- Remove everything and see what happens. + table.insert(steps, function() + + for _=1, 3 do + for _, s in ipairs {s1,s2} do + remove_at(s, 1) + end + end + + for _=1, 2 do + remove_at(s2, 1) + end + + -- And add them again. + add_many(s1) + add_many(s2) + + return true + end) + + local weak = nil + + -- Delete a screen and make sure it gets GCed. + table.insert(steps, function() + s2:fake_remove() + + -- Help the weak tables a little. + for _, pos in ipairs(positions) do + objs[pos][s1] = nil + end + + -- Drop our string reference to s2. + weak, s2 = setmetatable({s2}, {__mode="v"}), nil + + return true + end) + + table.insert(steps, function() + if weak[1] == nil then return true end + + for _=1, 10 do + collectgarbage("collect") + end + end) + +end require("_runner").run_steps(steps) From bd8f2f936b53d30e61a6fe04a90cc933cc961b93 Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Sun, 19 Sep 2021 22:21:38 -0700 Subject: [PATCH 06/10] naughty: Fix a memory leak related to suspended notifications. --- lib/naughty/core.lua | 15 ++++++++++---- tests/test-naughty-screen.lua | 38 ++++++++++++++++++++++++++++++++--- 2 files changed, 46 insertions(+), 7 deletions(-) diff --git a/lib/naughty/core.lua b/lib/naughty/core.lua index 0afd5508..757bbe17 100644 --- a/lib/naughty/core.lua +++ b/lib/naughty/core.lua @@ -191,6 +191,10 @@ naughty.notifications = { suspended = { }, _expired = {{}} } naughty._active = {} +local function get_screen(s) + return s and capi.screen[s] +end + local function init_screen(s) if naughty.notifications[s] then return end @@ -218,15 +222,18 @@ capi.screen.connect_signal("removed", function(scr) naughty.emit_signal("request::screen", n, "removed", {}) end end + + for _, n in ipairs(naughty._active) do + if n._private.args and get_screen(n._private.args.screen) == scr then + n._private.args.screen = nil + end + end + -- Destroy all notifications on this screen naughty.destroy_all_notifications({scr}) naughty.notifications[scr] = nil end) -local function get_screen(s) - return s and capi.screen[s] -end - local function remove_from_index(n) for _, positions in pairs(naughty.notifications) do for _, ns in pairs(positions) do diff --git a/tests/test-naughty-screen.lua b/tests/test-naughty-screen.lua index 7428a462..ef95acc4 100644 --- a/tests/test-naughty-screen.lua +++ b/tests/test-naughty-screen.lua @@ -30,11 +30,34 @@ local s1, s2 = mouse.screen, nil for _, p in ipairs(positions) do objs[p] = setmetatable({},{ - __index=function(t,k) t[k]={};return t[k] end, + __index = function(t,k) + t[k] = setmetatable({}, {__mode = "kv"}) + return t[k] + end, __mode = "k" }) end +local function cleanup(n) + -- Wait until there is no notifications left. + for _, pos in ipairs(positions) do + for s, notifs in pairs(objs[pos]) do + for k, n2 in ipairs(notifs) do + if n == n2 then + table.remove(notifs, k) + if #notifs == 0 then + objs[pos][s] = nil + end + return + end + end + end + end +end + +naughty.connect_signal("property::screen", cleanup) +naughty.connect_signal("destroyed", cleanup) + local function add_many(s) for _, pos in ipairs(positions) do for i=1, 5 do @@ -143,9 +166,17 @@ for _, legacy_preset in ipairs {true, false} do table.insert(steps, function() s2:fake_remove() - -- Help the weak tables a little. + return true + end) + + -- Check if notifications are moved. + table.insert(steps, function() + -- Wait until there is no notifications left. for _, pos in ipairs(positions) do - objs[pos][s1] = nil + if #objs[pos][s2] > 0 then + collectgarbage("collect") + return + end end -- Drop our string reference to s2. @@ -154,6 +185,7 @@ for _, legacy_preset in ipairs {true, false} do return true end) + table.insert(steps, function() if weak[1] == nil then return true end From ccbe11687782e96dd96972fcd29e8d92fc0a7be4 Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Mon, 20 Sep 2021 15:34:04 -0700 Subject: [PATCH 07/10] tests: Try to mitigate a flacky test with luajit 5.1. Give more attempt for the wibars to be GCed. This test fails ~5% of the time since moving from Travis CI to GitHub actions. --- tests/test-struts.lua | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/tests/test-struts.lua b/tests/test-struts.lua index 2cfee925..90cd9053 100644 --- a/tests/test-struts.lua +++ b/tests/test-struts.lua @@ -475,15 +475,29 @@ table.insert(steps, function() rwibar:remove() twibar:remove() + for _, w in ipairs(wibars) do + assert(not w.visible) + end + -- Make sure the placement doesn't hold a reference. bwibar, lwibar, rwibar, twibar = nil, nil, nil, nil screen.primary.mywibox = nil + return true +end) + +table.insert(steps, function() for _=1, 3 do collectgarbage("collect") end - assert(not next(wibars)) + if next(wibars) then + for _, w in ipairs(wibars) do + assert(not w.visible) + end + + return + end return true end) From 24fc1043eee105f2de320604677869c9ff908a65 Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Mon, 20 Sep 2021 16:04:41 -0700 Subject: [PATCH 08/10] tests: Remove a flackyness in test-input-binding --- tests/test-input-binding.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test-input-binding.lua b/tests/test-input-binding.lua index 25e2c007..99fdce49 100644 --- a/tests/test-input-binding.lua +++ b/tests/test-input-binding.lua @@ -256,7 +256,7 @@ local new1, new2 = nil, nil -- Check that you can add new default key/mousebindings at any time. table.insert(steps, function() - assert(#mouse.screen.clients == 0) + if #mouse.screen.clients > 0 then return end new1 = module.key { key = "a", From b038463e22c2661b54b0ea5e81324b8e2772b013 Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Wed, 22 Sep 2021 13:36:49 -0700 Subject: [PATCH 09/10] naughty.legacy: Handle broken configs betters. If a config with naughty.layouts.box crashes at startup and the fallback config uses naughty.layouts.legaxy, it is possible their will be some lookup for an object which isn't tracked by the legacy module. --- lib/naughty/layout/legacy.lua | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/naughty/layout/legacy.lua b/lib/naughty/layout/legacy.lua index a1999caa..c5d6c887 100644 --- a/lib/naughty/layout/legacy.lua +++ b/lib/naughty/layout/legacy.lua @@ -134,7 +134,7 @@ local function get_offset(s, position, idx, width, height) local find_old_to_replace = function() for i = 1, idx-1 do local n = current_notifications[s][position][i] - if n.timeout > 0 then + if n and n.timeout > 0 then return n end end From 64d190546d759138ebe31c8768021ea5a13e07f0 Mon Sep 17 00:00:00 2001 From: Emmanuel Lepage Vallee Date: Wed, 22 Sep 2021 13:38:20 -0700 Subject: [PATCH 10/10] core: Always add a search path when `-c` is passed to the cli. This way modular config load with just `-c` and don't require the extra `-s`. --- options.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/options.c b/options.c index df09ca8d..88de7b85 100644 --- a/options.c +++ b/options.c @@ -444,6 +444,9 @@ options_check_args(int argc, char **argv, int *init_flags, string_array_t *paths if (confpath != NULL) fatal("--config may only be specified once"); confpath = a_strdup(optarg); + + /* Make sure multi-file config works */ + string_array_append(paths, g_path_get_dirname(optarg)); break; case 'm': /* Validation */