Custom notification sounds for rooms

[Half-Shot]

Fixes element-hq/element-web#2105

This PR mainly exists so my alert noise for infrastructure breaking isn't the same noise as friendly PMs, which is probably heathy for my emotional state.

This is a very barebones feature for setting custom sounds for notifications within Riot. Riot reads for a room's "m.notification.sound" event and tries to load a mxcurl into a new audio element if ones doesn't already exist.

It does not support multiple sources for a sound.

Epic for this feature can be found at: element-hq/element-web#9687
Riot-web labs.md PR can be found at: element-hq/element-web#9688

[turt2live]

In general, settings should use the SettingsStore instead of going direct.

[Half-Shot]

@turt2live I forgot that existed. Updated.

[Half-Shot]

I've updated the description with a list of issues and corners cut when doing the PR for transparency. I suppose we want to fix some of them before merging but I believe some are quite minor.

[Half-Shot]

Looks like a not-my-problem fail:

[k80w]

Removing a sound from a room doesn't remove the tag.

A solution to this could be to give every room its own tag and change the sources whenever a rooms notification sound is changed. To me, this seems like a cleaner way to do things.

I don't think this is really an edge case; the odds of a user changing a notification sound for a room several times as they test different sounds and see which they'd prefer seems high.

[Half-Shot]

A solution to this could be to give every room its own tag and change the sources whenever a rooms notification sound is changed. To me, this seems like a cleaner way to do things.

Honestly we could just keep a counter of which rooms use which tags and reference count it, and just remove them on 0. The reason I've not done this is that I'm not convinced that it's going to cause a great impact to Riot if you do have lots of audio tags open. It may leave a few extra KB in memory, but it would be cleared on next restart anyway.

[Half-Shot]

is it possible to get a quick review on this?

[jryans]

I have added it to the team queue.

[turt2live]

Looks like a great first start, but there's more to do before we can safely merge this for labs. On top of the comments in the review, please also:

• Get a "this looks ok for now" review from @nadonomy (we can always iterate more finely later)
• Get a "this seems like something we want" checkmark from @lampholder to ensure it's in the product's vision
• Add a description to https://github.com/vector-im/riot-web/blob/develop/docs/labs.md as we're starting a trend of doing that.
• Create an epic issue to track what remaining bits would need to be done to get it out of labs. The list in the PR description seems like things we can avoid doing right this moment, but would be good out-of-labs candidates (minus those explicitly referenced in my prior points here).

[Half-Shot]

Thanks for the comprehensive feedback @turt2live, will get to it :)

[Half-Shot]

@turt2live I believe I have done everything I can my side now.

[turt2live]

lgtm although you'll probably have to be the one to ask Tom and Nad to take a look

[nadonomy]

@Half-Shot nice!

I know you're scratching a personal itch and this is in labs, but there's some 2 trivial UI changes that would be good to see:

1. We don't use black buttons in the UI. You're already decorating [Save] with mx_AccessibleButton_kind_primary and you'll want some visual distinction, with [Browse] carrying less weight, so I'd add a new CSS class for [Browse] with:

    color: $accent-color;
    background-color: transparent;
    border: 1px solid $accent-color;

We use similar styles in modals and elsewhere already.

2. I'd also decorate [Reset] with mx_AccessibleButton_kind_danger instead, and shorten 'Reset to default sound' to simply 'Reset'. You'll infer the function contextually, it commands less attention visually and the feature might benefit from internationalised strings.

[Half-Shot]

@nadonomy Thanks, these seem like good ideas. I'll go make the changes :)

[Half-Shot]

@nadonomy this is done

This is done again. I've moved the reset button next to information about the current sound in the room, because I realise the reset button is slightly misleading if it's moved near the upload box.

Default sound, no upload:

Pending upload:

Uploaded:

[nadonomy]

Great, thanks!

[Half-Shot]

Tagging @ara4n

[lampholder]

I've had a play and it LGTM :)

[Half-Shot]

Thanks :), does that mean I have met the criteria for labs now 😇 ?

[jryans]

The remaining steps as I understand it are:

• Remove the labs / feature flag, as @lampholder feels it's okay to go in without that
• Change to im.vector namespace since there's no flag now
• Request a fresh code review from riot-web team once that's done to double-check the code before landing

Almost there! 😁

[turt2live]

otherwise lgtm

[Half-Shot]

@turt2live all good to merge now? :3

[turt2live]