element-ios icon indicating copy to clipboard operation
element-ios copied to clipboard

Background audio messages info (PSB-180)

Open alfogrillo opened this issue 3 years ago • 6 comments

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
before now

alfogrillo avatar Oct 13 '22 12:10 alfogrillo

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

sonarqubecloud[bot] avatar Oct 13 '22 12:10 sonarqubecloud[bot]

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

QR code

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

github-actions[bot] avatar Oct 13 '22 13:10 github-actions[bot]

Codecov Report

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

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%) :arrow_down:

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

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

codecov[bot] avatar Oct 13 '22 13:10 codecov[bot]

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?

alfogrillo avatar Oct 14 '22 09:10 alfogrillo

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?

Johennes avatar Oct 14 '22 09:10 Johennes

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.

alfogrillo avatar Oct 17 '22 10:10 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.

jakewb-b avatar Oct 18 '22 15:10 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.

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

alfogrillo avatar Oct 19 '22 09:10 alfogrillo