-
Notifications
You must be signed in to change notification settings - Fork 42
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
check if window is not the same as the one where the command started … #35
base: master
Are you sure you want to change the base?
Conversation
…executing on expiration to show notifications in a smarter way
@@ -53,6 +53,14 @@ function _auto_notify_message() { | |||
fi | |||
} | |||
|
|||
function _auto_notify_active_window_id () { | |||
if [[ -n $DISPLAY ]] ; then | |||
xprop -root _NET_ACTIVE_WINDOW | awk '{print $5}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work on Mac?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. Only tested on linux.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need this to work on Mac or at the very least gracefully ignore this feature in order for this to get merged. Wouldnt want this to not work for non-linux users.
The same question also applies to those using wayland, does xprop work there?
For example on my machine I get:
$ xprop -root _NET_ACTIVE_WINDOW | awk '{print $5}'
0x0
which doesnt look right.
@@ -154,6 +154,15 @@ then all the values in ``AUTO_NOTIFY_IGNORE`` are not used. | |||
|
|||
export AUTO_NOTIFY_WHITELIST=("apt-get" "docker") | |||
|
|||
**Ignoring Window Check** | |||
|
|||
By default the notification only shows if the active window is not the same as the the one that the command was run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably make this the non default for backwards compatibility purposes. I'll give it some thought
@@ -119,6 +131,7 @@ function _auto_notify_track() { | |||
AUTO_COMMAND="${1:-$2}" | |||
AUTO_COMMAND_FULL="$3" | |||
AUTO_COMMAND_START="$(date +"%s")" | |||
AUTO_NOTIFY_ACTIVE_WINDOW_ID="$(_auto_notify_active_window_id)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it won't work if multiple commands are running at once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle I really like the idea, but I think it needs good amount of testing before being merged
Thanks for the PR @mikob! 🎉
…executing on expiration to show notifications in a smarter way