awesomeWM / awesome

awesome window manager
https://awesomewm.org/
GNU General Public License v2.0
6.37k stars 598 forks source link

notify-send does not notify and hangs on master #2829

Open vn-ki opened 5 years ago

vn-ki commented 5 years ago

Output of awesome --version:

awesome v4.3-314-g1116bc47 (Too long)
 • Compiled against Lua 5.3.5 (running with Lua 5.3)
 • D-Bus support: ✔
 • xcb-errors support: ✘
 • execinfo support: ✔
 • xcb-randr version: 1.6
 • LGI version: 0.9.2

How to reproduce the issue:

Actual result:

Expected result:

Ping: @Elv13

vn-ki commented 5 years ago

Note: naughty.notify and naughty.notification produces notifications

Elv13 commented 5 years ago

Can you bisect or tell me the last version that worked so I can revert the commit. The tests passed... If https://github.com/awesomeWM/awesome/pull/2825 was involved I would not be that surprised, but master...

Elv13 commented 5 years ago

I confirm I cannot reproduce, it works for me

DISPLAY=:2 /home/lepagee/dev/awesome/build/awesome -c /home/lepagee/dev/awesome/build/awesomerc.lua --search /home/lepagee/dev/awesome/lib -v
awesome v4.3-314-g1116bc47d (Too long)
 • Compiled against Lua 5.1.5 (running with Lua 5.1)
 • D-Bus support: ✔
 • xcb-errors support: ✘
 • execinfo support: ✔
 • xcb-randr version: 1.6
 • LGI version: 0.9.0
>
elenapan commented 5 years ago

Having the same issue on v4.3-314-g1116bc47 with the new notification API enabled.

notify-send hangs with one argument: notify-send "something" However, with 2 arguments it works fine: notify-send "something" "else"

I think notify-send automatically takes the first argument as the title of the notification and the second one as the message. It might have something to do with that.

Elv13 commented 5 years ago

@elenapan can you bisect or otherwise investigate a bit more? Which distribution do you use? Is it also Lua 5.3?

It works for me in Xephyr and in a real session both with the legacy and new API. The CI also seems to work. Is there errors in ~/.xsession_errors?

elenapan commented 5 years ago

My apologies, looking at ~/.xsession-errors made me realize it was my fault.

Inside naughty.connect_signal("request::display", function(n) ... end) I was trying to customize the notification's widget_template by checking if n.title matches some string.

However when I send a notification with no title (so, notify-send with just one argument), n.title becomes nil and this error appears on the line where I was trying to access it:

2019-07-21 12:46:30 E: awesome: Error during a protected call: /home/elena/.config/awesome/rc.lua:275: attempt to index a nil value (field 'title')
stack traceback:
    /home/elena/.config/awesome/rc.lua:275: in local 'func'
    /usr/local/share/awesome/lib/naughty/core.lua:255: in function 'naughty.emit_signal'
    /usr/local/share/awesome/lib/naughty/notification.lua:810: in function </usr/local/share/awesome/lib/naughty/notification.lua:727>
    (...tail calls...)
    /usr/local/share/awesome/lib/naughty/dbus.lua:243: in function </usr/local/share/awesome/lib/naughty/dbus.lua:125>
    [C]: in function 'xpcall'
    /usr/local/share/awesome/lib/gears/protected_call.lua:36: in function </usr/local/share/awesome/lib/gears/protected_call.lua:35>
    (...tail calls...)
    /usr/local/share/awesome/lib/naughty/dbus.lua:279: in upvalue 'target'
    /usr/share/lua/5.3/lgi/override/GObject-Closure.lua:286: in function </usr/share/lua/5.3/lgi/override/GObject-Closure.lua:283>

Now I just check if it exists before accessing it.

The default widget_template works without problems.

@vn-ki ~/.xsession-errors will probably give you a hint.

Elv13 commented 5 years ago

Ok, "cool". It is still a bug, but it is a lot eaiser to fix than unknown "it doesn't work for me and it works for you" type of issue.

Elv13 commented 5 years ago

@elenapan Can you test and review https://github.com/awesomeWM/awesome/pull/2825 ? Many of the changes are likely to impact your config from what I can see from your various messages. I added a chain of fallbacks in case the template has issue and I also added a default value for the message and title. That pull request (along with the gears.matcher one) are the last groundwork bits before adding the notification rules. Those rules will make it easy to ei. use a different template for music player notification or chat apps.

elenapan commented 5 years ago

2825 gave me this error when setting a widget_template (any widget_template, even just a lone textbox):

2019-07-23 02:32:52 E: awesome: Error during a protected call: /usr/local/share/awesome/lib/naughty/layout/box.lua:165: attempt to call a nil value (method 'set_width')
stack traceback:
    /usr/local/share/awesome/lib/naughty/layout/box.lua:165: in upvalue 'generate_widget'
    /usr/local/share/awesome/lib/naughty/layout/box.lua:252: in function </usr/local/share/awesome/lib/naughty/layout/box.lua:237>
    (...tail calls...)
    /home/elena/.config/awesome/notifications.lua:108: in local 'func'
    /usr/local/share/awesome/lib/gears/object.lua:177: in function 'naughty.init.emit_signal'
    /usr/local/share/awesome/lib/naughty/notification.lua:1097: in function </usr/local/share/awesome/lib/naughty/notification.lua:1006>
    (...tail calls...)
    /home/elena/.config/awesome/helpers.lua:344: in function </home/elena/.config/awesome/helpers.lua:333>
    (...tail calls...)
    /usr/local/share/awesome/lib/awful/spawn.lua:484: in function </usr/local/share/awesome/lib/awful/spawn.lua:478>
    [C]: in function 'xpcall'
    /usr/local/share/awesome/lib/gears/protected_call.lua:36: in function </usr/local/share/awesome/lib/gears/protected_call.lua:35>
    (...tail calls...)
    /usr/local/share/awesome/lib/awful/spawn.lua:583: in upvalue 'done'
    /usr/local/share/awesome/lib/awful/spawn.lua:598: in function </usr/local/share/awesome/lib/awful/spawn.lua:590>

I replaced w:set_width(...) with w.width = ... in /usr/local/share/awesome/lib/naughty/layout/box.lua and it seems to work just fine.

notify-send does not hang anymore when accessing empty fields of the notification. No changes to my config were needed.

Elv13 commented 5 years ago

Whoops, the "fix" for the max_width assumed the template would use a constraint container. While doing so is a good idea, it should not be necessary. I pushed a fix.

Elv13 commented 5 years ago

@elenapan Can you try again the changes I made last week? I would also appreciate a review on that pull request. I will merge is "soon" so it gets some testing before I submit the last part (using rules to filter notifications and replace the presets). That pull request is ready, but held back by 2 open and unreviewed pull requests.

elenapan commented 5 years ago

Just tried the changes from last week, and they work fine for me. Clear stderr and no notify-send hanging. Let me know if you need more info.

Also, sorry for asking, but how would I go about reviewing a pull request? It will be my first time doing it. Should I just test the various features it adds using my own config?

Elv13 commented 5 years ago

Also, sorry for asking, but how would I go about reviewing a pull request? It will be my first time doing it. Should I just test the various features it adds using my own config?

It is up to you. Usually you look at the code in the "file changed" tab of the pull request, then press on the line number to leave comments. You give your opinion on the API changes, find typos or report potential issues (which you tested or just think could happen). If you tested the code with your usual notifications and it didn't explode, then it's already a sign I didn't introduce [additional] major regressions by accident. Once you are done, you press the "review changes" at the top right of the pull request and leave a general comment. Make sure not to use "start a review" and not the "add single comments" button, the later sends a lot of email and makes it very hard to track the fixes once I make them.