Add playback speed control to voice messages #4482
Conversation
arkascha
force-pushed
the
issues-334-allow-voice-messages-to-be-played-at-different-speeds
branch
from
f45dbe4 to
c5c716b
Compare
AndyScherzinger
added
enhancement
arkascha
force-pushed
the
issues-334-allow-voice-messages-to-be-played-at-different-speeds
branch
from
0e27404 to
898cd0e
Compare
|
@rapterjet2004 It seems we had a mid air collision ... I received an email message pointing to a commit you added, but I cannot find that commit. I am afraid I suppressed it by a force push I made inside my repository from where I created this PR. Sorry for that, I did not expect it is possible to add a commit to a PR, that is a new concept for me. Could you please cherry-pick that commit again? |
arkascha
force-pushed
the
issues-334-allow-voice-messages-to-be-played-at-different-speeds
branch
from
898cd0e to
6410dae
Compare
rapterjet2004
force-pushed
the
issues-334-allow-voice-messages-to-be-played-at-different-speeds
branch
from
6410dae to
c97933b
Compare
|
Is there anything that needs to be done on my side? |
|
I guess no, just waiting for @rapterjet2004 or @mahibi The failing check will be fixed once #4502 gets merged |
AndyScherzinger
force-pushed
the
issues-334-allow-voice-messages-to-be-played-at-different-speeds
branch
from
c97933b to
c2ac90d
Compare
|
As I understand it you are accepting this change without any implemented tests. A bit surprising, that is why I asked about your test strategy when initially opening the PR. |
|
We do implement test, generally speaking i.e. in https://github.com/nextcloud/talk-android/tree/master/app/src/androidTest/java/com/nextcloud/talk and https://github.com/nextcloud/talk-android/tree/master/app/src/test and at some point might also add screenshot tests like we have in the Files client repository. Looking at the size of the change andf the changes themselves I'd persoanlly be fine with no additional, automated test. However this is just my approval and @mahibi or @rapterjet2004 (who build the voice message visualization feature) might have a different feedback. |
|
works great. Fine for me to merge! Thank you @arkascha ! |
Signed-off-by: Christian Reiner <[email protected]> Themed the PlaybackSpeedControl + Work around onBind bug Signed-off-by: rapterjet2004 <[email protected]>
Signed-off-by: Andy Scherzinger <[email protected]>
…eation Signed-off-by: Andy Scherzinger <[email protected]>
AndyScherzinger
force-pushed
the
issues-334-allow-voice-messages-to-be-played-at-different-speeds
branch
from
b8aef7d to
5011649
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
This is an implementation of the feature request in issue #3786 :
"Allow voice messages to be played at different speeds"
This PR should not be considered final. There are a few questions to be answered first, I cannot do that myself, however. I need someone from inside the project for that. So this is more a "request for comment" than a PR ;-)
The current implementation does not at all offer a fancy design. The UI is as simplistic as possible.
I could very well imagine a button that extends when clicked and offers the options in parallel instead of cycling through the offered options as it is currently implemented. But that certainly is something a design expert has to decide.
The current implementation has 4 speeds it offers (opposed to WhatsApp offering 3): 0.8x, 1.0x, 1.5x, 2.0x. I added the slow variant since that could sometimes help to understand messages with bad acoustics.
The chosen speed is persisted on a per-user base. This appeared intuitive to me, since usually individual users keep their typical talking speed. Which means that if there is a need to change the playback speed that change probably makes sense for all voice messages of that user.
I would need some advice about the testing strategy in this project ... When trying to implement tests I found that next to no tests and test structure exist. I assume I miss something here ...
Seems I found a bug in how the message view holders are handled: their onBind() method is called over and over again when a voice message is played. That might be triggered by updating the progress slider. This behaviour repaints the view holder millions of times which actually creates quite a load on the system ...
I am thankful for any feedback on this!
🖼️ Screenshots
🚧 TODO
🏁 Checklist
/backport to stable-xx.x