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

Add command to set per-room notification levels #305

Merged
merged 8 commits into from
Aug 17, 2024

Conversation

rmsthebest
Copy link
Contributor

As mentioned in the issue #304, I couldn't find a way to set notification options.

Here is an attempt to implement it. It isn't working, I'm not sure why so help is appreciated.

I apologize for breaking formatting, maybe my editor isnt picking up the rustfmt.toml from the repo?
If we get this working we can fix that later

Copy link
Owner

@ulyssa ulyssa left a comment

Choose a reason for hiding this comment

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

This looks good! Thank you for implementing this!

I apologize for breaking formatting, maybe my editor isnt picking up the rustfmt.toml from the repo?

The formatting thing is probably because your editor isn't using cargo +nightly fmt to enable the nightly-only options that are in .rustfmt.toml. I've run it by hand and pushed to the branch to make it easier to read.

If we get this working we can fix that later

What part wasn't working for you? I tried playing around with this locally and it seemed to be working at a first glance to me. I could mute and unmute rooms and messages sent to that room by other accounts disappeared/appeared based on the settings. The settings changes are also visible in Element.

I've added an unset command that uses delete_user_defined_room_rules() to restore settings to the account default, and I've removed some of the level name variants to keep it a small easy-to-type list for now.

@ulyssa
Copy link
Owner

ulyssa commented Aug 17, 2024

I've fixed the merge conflicts and added support for using with the RoomAction::Show action that was added in #279.

@ulyssa ulyssa marked this pull request as ready for review August 17, 2024 21:41
@ulyssa
Copy link
Owner

ulyssa commented Aug 17, 2024

From what I can tell playing around with this, it seems good to me! If there are any other issues we can fix those in a follow-up PR.

@ulyssa ulyssa changed the title Configure Notification Levels per room (not working atm) Add command to set per-room notification levels Aug 17, 2024
@ulyssa ulyssa merged commit 2a66496 into ulyssa:main Aug 17, 2024
3 checks passed
@ulyssa ulyssa added this to the v0.0.10 milestone Aug 17, 2024
@rmsthebest
Copy link
Contributor Author

This looks good! Thank you for implementing this!

I apologize for breaking formatting, maybe my editor isnt picking up the rustfmt.toml from the repo?

The formatting thing is probably because your editor isn't using cargo +nightly fmt to enable the nightly-only options that are in .rustfmt.toml. I've run it by hand and pushed to the branch to make it easier to read.

If we get this working we can fix that later

What part wasn't working for you? I tried playing around with this locally and it seemed to be working at a first glance to me. I could mute and unmute rooms and messages sent to that room by other accounts disappeared/appeared based on the settings. The settings changes are also visible in Element.

I've added an unset command that uses delete_user_defined_room_rules() to restore settings to the account default, and I've removed some of the level name variants to keep it a small easy-to-type list for now.

So a few days after I wrote this I noticed that the notification settings seems to had changed, because it changed them on my phone, but in iamb on my desktop it didnt seem to care about the per room settings at all.

I'm glad it works for you though.
Thanks for fixing it up and merging!

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.

2 participants