Background audio messages info (PSB-180)

[alfogrillo]

Ticket -> https://element-io.atlassian.net/browse/PSB-180?atlOrigin=eyJpIjoiM2QxY2RmZWM1MDcxNDAwN2E1YWJiYjU3OWM2MmZkMzUiLCJwIjoiaiJ9

This PR

• Hides the information about the message/room in the background audio player
• Introduces the new BuildSetting.allowBackgroundAudioMessagePlayback to disable the background audio feature completely

Before	After

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[github-actions]

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/miQ3Ad

[codecov]

Codecov Report

Base: 11.63% // Head: 11.63% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (ac655c4) compared to base (380ad94).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #6870      +/-   ##
===========================================
- Coverage    11.63%   11.63%   -0.01%     
===========================================
  Files         1559     1559              
  Lines       155250   155255       +5     
  Branches     62667    62668       +1     
===========================================
- Hits         18060    18057       -3     
- Misses      136578   136586       +8     
  Partials       612      612              

Impacted Files	Coverage Δ
Config/BuildSettings.swift	43.07% <ø> (ø)
...s/Room/VoiceMessages/VoiceMessageAudioPlayer.swift	0.00% <0.00%> (ø)
...iceMessages/VoiceMessageMediaServiceProvider.swift	0.00% <0.00%> (ø)
...ojiPicker/Data/EmojiMart/EmojiItem+EmojiMart.swift	95.34% <0.00%> (-0.31%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pixlwave]

The code looks fine to me but I'm going to question the decision to remove useful info from the now playing screen on Element. We may have users that are used to having this and I can't really see any justification for removing it for regular users.

Could that change perhaps also be behind a build flag?

[pixlwave]

Would it be worth adding a check for isPlaying in here too?

[alfogrillo]

I don't think so. Pausing a paused stream looks like a nop.

[pixlwave]

Even for any of the delegates that get called?

[alfogrillo]

Tried it, I'm not seeing anything bad in the app.

[alfogrillo]

The code looks fine to me but I'm going to question the decision to remove useful info from the now playing screen on Element. We may have users that are used to having this and I can't really see any justification for removing it for regular users.

Could that change perhaps also be behind a build flag?

That sounds reasonable to me.
But how are we going to do it? Adding a second flag?

• Flag 1: background audio on/off
• Flag 2: less info in the player on/off

What do you think @Johennes?

[Johennes]

We had discussed removing the possibly sensitive info on the lock screen with product before and reached this decision due to privacy concerns. This does seem like a relatively granular setting so my personal tendency is to pick a sensible default rather than adding another configuration option. @jakewb-b maybe you could expand on this?

[alfogrillo]

Another option I was thinking is to have 3-states build setting like this:

extension BuildSetting {
    enum BackgroundAudioPlayerBehaviour {
        case off
        case `default`
        case anonymous
    }
    
    static let backgroundAudioPlayerBehaviour: BackgroundAudioPlayerBehaviour = .default
}

But probably it's better to understand with @jakewb-b if this should be or not a runtime setting.

[jakewb-b]

As a rule I’d much prefer to avoid additional settings and choose a sensible default, as Johannes suggests. In this case, if we’re going with a default we should bias for privacy and I’m content to remove the information from the lock screen as I think it’s of relatively low utility and, while any change may be a surprise to users, it’s not something that seems likely to cause huge issues in using the feature.

I think there could be a case in the future for a runtime setting that bundles a few other items into one privacy choice to, for example, ‘minimise display of room and user information on lock screen’ but we’d need to work out if there are, in practice, any other items that could usefully be bundled with this.

In the short term, this mustn’t block us progressing so let’s go with hiding the information for now and then review.

[alfogrillo]

As a rule I’d much prefer to avoid additional settings and choose a sensible default, as Johannes suggests. In this case, if we’re going with a default we should bias for privacy and I’m content to remove the information from the lock screen as I think it’s of relatively low utility and, while any change may be a surprise to users, it’s not something that seems likely to cause huge issues in using the feature.

I think there could be a case in the future for a runtime setting that bundles a few other items into one privacy choice to, for example, ‘minimise display of room and user information on lock screen’ but we’d need to work out if there are, in practice, any other items that could usefully be bundled with this.

In the short term, this mustn’t block us progressing so let’s go with hiding the information for now and then review.

Thanks Jake,
@pixlwave, still needing your approval if we are happy with the code. ;-)