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

Support enabling multiple notification sinks #344

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

Nemo157
Copy link
Contributor

@Nemo157 Nemo157 commented Aug 30, 2024

Fixes #340, as I mention there I have a usecase for using both kinds of notifications at the same time.

src/config.rs Outdated
Comment on lines 410 to 411
#[serde(untagged)]
Multiple(Vec<Self>),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This technically allows [["bell", "bell"], "desktop"] and other silly settings, but for code simplicity that feels fine to me. The other approach would be to use something like bitflags but I'm not sure how complicated deserializing that would be.

Copy link
Owner

Choose a reason for hiding this comment

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

I think that making this look like a bitwise-or would be good so that folks can write via: "bell|desktop". There's an example of what that looks like here for VimModes which you could copy for making a NotifyViaSet(Vec<NotifyVia>) type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally got around to changing it to look like a bitwise-or, and added parser tests for it.

@ulyssa ulyssa added this to the v0.0.11 milestone Sep 17, 2024
@ulyssa
Copy link
Owner

ulyssa commented Sep 17, 2024

This looks great! Thank you for adding tests for it, too!

@ulyssa ulyssa changed the title feat: allow enabling multiple notification sinks Support enabling multiple notification sinks Sep 17, 2024
@ulyssa ulyssa merged commit 9a9bdb4 into ulyssa:main Sep 17, 2024
3 checks passed
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.

Allow enabling both desktop and bell notifications
2 participants