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

Use libnotify bindings, close notification after touch #13

Merged
merged 7 commits into from
Jul 28, 2020

Conversation

n-st
Copy link
Contributor

@n-st n-st commented Jul 28, 2020

Fixes #12.

Copy link
Owner

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR!

My first time writing Go code, so it will probably need some polishing. ;)

Tried to do my best to explain some comments so that you benefit by learning some Go today 👍

The Makefile automatically added the new library to two dependency files — I assume that's okay/intentional?

Yes, it pins dependencies to the exact versions, so if it works for you, it is guaranteed that it will work for everyone else, even if dependencies change over time and the code will not work with newer dependencies.

I only generate a single notification object and show/close it as appropriate. Works fine with the dunst OSD service, but maybe other services don't re-show notifications that existed previously?

I tested on mako on Wayland and it works exactly the same, so I think it's safe to assume that your logic is good - if not, someone will eventually report a bug and we will deal with it then 😉

notifier/libnotify.go Outdated Show resolved Hide resolved
notifier/libnotify.go Show resolved Hide resolved
notifier/libnotify.go Show resolved Hide resolved
notifier/libnotify.go Show resolved Hide resolved
@n-st
Copy link
Contributor Author

n-st commented Jul 28, 2020

Thank you for the helpful feedback! :)
I've added your suggestions and marked the comment threads "resolved" (my screen was getting a bit crowded). Github should allow you to reopen them, right?

Copy link
Owner

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's awesome, thank you very much for your contribution!

@maximbaz maximbaz merged commit 021d7cb into maximbaz:master Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: Close notification when touch is detected
2 participants