Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lnotify 0.3.4 is incompatible with procps-ng 3.3.12 #339

Open
larsks opened this issue May 22, 2019 · 4 comments
Open

lnotify 0.3.4 is incompatible with procps-ng 3.3.12 #339

larsks opened this issue May 22, 2019 · 4 comments
Labels
bug Unexpected problem or unintended behavior

Comments

@larsks
Copy link

larsks commented May 22, 2019

The lnotify script uses ps to determine the name of the currently running window, but it uses command line arguments that are not supported by the version of ps in many distributions. The generate command line looks like:

ps -ho comm -p xterm

And fails like this:

error: unsupported SysV option

Usage:
 ps [options]

 Try 'ps --help <simple|list|output|threads|misc|all>'
  or 'ps --help <s|l|o|t|m|a>'
 for additional help text.

For more details see ps(1).

Note this is a different failure than the one reported in #334.

@larsks
Copy link
Author

larsks commented May 22, 2019

@kevr I think this is your plugin. What is the ignore_windows_list code for? It currently has a somewhat arbitrary list of terminal names that is not user-configureable, and this particular chunk of code is responsible for multiple failures.

@larsks
Copy link
Author

larsks commented May 22, 2019

I wonder if we should:

(a) just remove it:

diff --git a/python/lnotify.py b/python/lnotify.py
index 5070ba3..f3a20ac 100644
--- a/python/lnotify.py
+++ b/python/lnotify.py
@@ -94,20 +94,8 @@ def handle_msg(data, pbuffer, date, tags, displayed, highlight, prefix, message)
     buffer_type = weechat.buffer_get_string(pbuffer, "localvar_type")
     away = weechat.buffer_get_string(pbuffer, "localvar_away")
     x_focus = False
-    window_name = ""
     my_nickname = "nick_" + weechat.buffer_get_string(pbuffer, "localvar_nick")

-    # Check if active window is in the ignore_windows_list and skip notification
-    if (environ.get('DISPLAY') != None) and path.isfile("/bin/xdotool"):
-        cmd_pid="xdotool getactivewindow getwindowpid".split()
-        window_pid = subprocess.check_output(cmd_pid)
-        cmd_name=("ps -ho comm -p %s"%(window_pid)).split()
-        window_name = subprocess.check_output(cmd_name)
-        ignore_windows_list = ["tilda", "gnome-terminal", "xterm"]
-        if window_name in ignore_windows_list:
-            x_focus = True
-            return weechat.WEECHAT_RC_OK
-
     if pbuffer == weechat.current_buffer() and x_focus:
         return weechat.WEECHAT_RC_OK

Or (b) make it user configurable? I'm not clear what the intent is here (e.g., as currently written, this code will suppress notifications whenever I'm using an gnome-terminal terminal, which seems odd).

@larsks larsks changed the title lnotify 0.3.3 is incompatible with procps-ng 3.3.12 lnotify 0.3.4 is incompatible with procps-ng 3.3.12 May 22, 2019
@weechatter weechatter added the bug Unexpected problem or unintended behavior label May 23, 2019
@sadsfae
Copy link

sadsfae commented May 29, 2019

The lnotify script uses ps to determine the name of the currently running window, but it uses command line arguments that are not supported by the version of ps in many distributions. The generate command line looks like:

ps -ho comm -p xterm

And fails like this:

error: unsupported SysV option

Usage:
 ps [options]

 Try 'ps --help <simple|list|output|threads|misc|all>'
  or 'ps --help <s|l|o|t|m|a>'
 for additional help text.

For more details see ps(1).

Note this is a different failure than the one reported in #334.

I have the same issue fwiw @larsks

larsks added a commit to larsks/scripts that referenced this issue May 31, 2019
The python "lnotify" plugin had some problematic code that would
suppress notifications if the active window was named "tilda",
"xterm", or "gnome-terminal". This list was not user-configurable, and
the code implementing this feature was broken due to weechat#339
(incompatible with common versions of ps) and weechat#334 (python3
bytes-vs-string incompatibility).

This commit removes the problematic code.

Closes: weechat#334
Closes: weechat#339
@cwegener
Copy link

I wonder if we should:

(a) just remove it:

Yes, I would say so. The choice of window titles seems completely
arbitrary.

I would be quite surprised if anyone is actually relying on this (now
completely broken) functionality at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

4 participants