-
Couldn't load subscription status.
- Fork 363
Feature: remove_current in notification queues #1491
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
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1491 +/- ##
==========================================
- Coverage 64.90% 64.80% -0.10%
==========================================
Files 51 51
Lines 9024 9039 +15
Branches 1048 1057 +9
==========================================
+ Hits 5857 5858 +1
- Misses 3167 3181 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
thanks for your contribution. I think that it's best to not change the default values of the config.
Also for the code, instead of making a separate queues_remove... function, why not reuse the existing functions to close and remove history?
|
let me know what do you think |
|
Hello, @bynect, Thanks for helping me with this contribution! 🎉 I just read the comments, nice catches! I'll just point some questions here for we to finish this feature as soon as possible! So, the idea of keeping the dunstrc file as is, is great! I should not have changed the default behavior haha Just thought about it after opening the Pull Request, but really doesn't seems to be a good idea! 😅 Also, you pointed that, we already have functions to handle close and remove signals. Yes, you're right, the remove_current does a simple close & remove from history. But seems cleaner to keep separated responsibilities (close functions only for close action, remove functions only for removing actions, also, I thought the history removing should be used with the close_all or something). Please, tell me what you think about this logic I used, and if it's incorrect! 🙏 Thanks again for your messages! If you believe it's strictly necessary or better to keep the code using shared functions to achieve this new action (remove_current), just tell me and I will change it right away! ❤️ |
|
you are right that the operation in the future could differ from a simple combination of those function. as a compromise what about declaring functions remove_current but using internally those two functions? |
|
Hello, @bynect! The changes are done! Let me know if you have any update. 🙏 |
|
code looks good. the ci problem is unrelated. I will try it and merge shortly 👍🏻 |
|
thanks |
Pull Request Purpose 🤔
This pull request introduces a new "remove_current" mouse action to allow users to remove notifications from history; implementing its functionality in the notification handling logic, and updating tests and documentation. 🚀
I really love the dunst project, and I think a very basic and useful feature that it's currently missing is to click and remove the notification from history.
So I decided to implement it! 🎉
Here's my quick contribution to the project ❤️
The main ideia, is to have "do_action" and "remove_notification" to work together nicelly, and avoid stuff like this:
I hope the Pull Request goes well and the code is not that mess hahaha
This PR was highly inspired by #1463 🚀
I hope it works and get merged soon for other users to also use this amazing feature! 🙏
PS: I also updated the docs to include this new option/functionality, and added a tiny test suit in the valid_options, nothing big. 👍
Thanks!