Merge pull request #3436 from Elv13/fix_3428

Fix setting the screen in a notification preset.

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

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
@@ -128,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

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`",
@@ -861,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
 

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

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 */

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",

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)

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
@@ -71,90 +94,106 @@ 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()
+
+        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
+            if #objs[pos][s2] > 0 then
+                collectgarbage("collect")
+                return
+            end
+        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)

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)