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

Resumes playback of VoiceMessages on screen rotation and continue playing a voice message if chatActivity in background but still open #3704

Merged
merged 6 commits into from
Mar 15, 2024

Conversation

Ruggero1912
Copy link
Contributor

πŸ–ΌοΈ Screenshots

🏚️ Before 🏑 After
B A
youtube demo video before modifications youtube demo video after modifications

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not needed
  • πŸ”– Capability is checked or not needed
  • πŸ”™ Backport requests are created or not needed: /backport to stable-xx.x
  • πŸ“… Milestone is set
  • 🌸 PR title is meaningful (if it should be in the changelog: is it meaningful to users?)

I modified ChatActivity in these terms:

  • added the method onSaveInstanceState() which is called according to this logic and enables to store the voiceMessage ID and audio position for future resuming
  • new method resumeAudioPlaybackIfNeeded() which is in charge of effectively scrolling back to the audio to be resumed and to resume playing it
    • this method is called once the adapter has finished to load chat messages
  • if the user clicks back button, the stopMediaPlayer method is called inside handleOnBackPressed
  • inside the onStop() method the method stopMediaPlayer() is not called anymore, because:
    • if the activity is destroyed, the method stopMediaPlayer is called inside onDestroy()
    • if the user clicks back button, the stopMediaPlayer method is called inside handleOnBackPressed

@mahibi
Copy link
Collaborator

mahibi commented Mar 12, 2024

Thank you @Ruggero1912
Tested and works fine πŸ‘

Will leave a few minor comments in the code.
From my side, comments don't need to be that verbose. Other than that, could you

  • autoformat the sourcecode (STRG+ALT+L)
  • rebase on the latest master

Also might make sense that @rapterjet2004 has a look as you worked on voice messages last.
After this, it should be ready to merge.

@Ruggero1912
Copy link
Contributor Author

Ruggero1912 commented Mar 12, 2024

Hi, I made the modifications required, let me know if everything is ok πŸ‘

Not sure if I should also sign the first two commits, I created a gpg key to sign the commit that I made today.

In the case it is needed, could you please address me on how to sign two "old" commits?

Thanks

@mahibi
Copy link
Collaborator

mahibi commented Mar 13, 2024

Hi, I made the modifications required, let me know if everything is ok πŸ‘

Not sure if I should also sign the first two commits, I created a gpg key to sign the commit that I made today.

In the case it is needed, could you please address me on how to sign two "old" commits?

Thanks

I think squash your commits into one commit would be the easiest option.

Other than that, you could try:
https://superuser.com/questions/397149/can-you-gpg-sign-old-commits

so that audio continues playing when activity in background.
if backpressed, stops mediaplayer

Signed-off-by: Giacomo Pacini <giacomopacini98@gmail.com>
also restores view position to that message and then resumes audio playback if was playing.
it allows to continue playing audio on screen rotation.

Signed-off-by: Giacomo Pacini <giacomopacini98@gmail.com>
…mpanion object

Signed-off-by: Giacomo Pacini <giacomopacini98@gmail.com>
@Ruggero1912
Copy link
Contributor Author

I think squash your commits into one commit would be the easiest option.

Other than that, you could try: https://superuser.com/questions/397149/can-you-gpg-sign-old-commits

Done, thanks.

@mahibi mahibi enabled auto-merge March 15, 2024 13:32
@mahibi mahibi disabled auto-merge March 15, 2024 15:00
@mahibi mahibi merged commit 007c407 into nextcloud:master Mar 15, 2024
12 of 15 checks passed
@mahibi
Copy link
Collaborator

mahibi commented Mar 15, 2024

Thank you @Ruggero1912 πŸ‘

@mahibi
Copy link
Collaborator

mahibi commented Mar 15, 2024

For the long term, unrelated to this PR:
Playing a voice message could be done as a service with Media controls in System UI

@rapterjet2004
Copy link
Contributor

rapterjet2004 commented Mar 15, 2024

@Ruggero1912 Great PR! πŸš€ 🎸
Unrelated to your PR, I didn't realize how annoying it was for the chat and waveform to reload on orientation change until I saw your video πŸ˜† I'll try to fix that in a separate PR.

@mahibi

Playing a voice message could be done as a service with Media controls in System UI

WhatsApp also has a UI component for out-of-chat playback while still in the app. These features along with a playback speed button would definitely flesh out the voice messaging implementation.
image
Also, given the growing complexity of the message input box, It would be wise to refactor it into it's own fragment. I think you brought this up some time ago. Definitely something we should handle eventually before things get too complicated. Maybe management might want Nextcloud Assistant embedded into messaging, which could complicate things even more.

@Ruggero1912
Copy link
Contributor Author

@Ruggero1912 Great PR! πŸš€ 🎸
Unrelated to your PR, I didn't realize how annoying it was for the chat and waveform to reload on orientation change until I saw your video πŸ˜† I'll try to fix that in a separate PR.

@mahibi

Playing a voice message could be done as a service with Media controls in System UI

WhatsApp also has a UI component for out-of-chat playback while still in the app. These features along with a playback speed button would definitely flesh out the voice messaging implementation.
image
Also, given the growing complexity of the message input box, It would be wise to refactor it into it's own fragment. I think you brought this up some time ago. Definitely something we should handle eventually before things get too complicated. Maybe management might want Nextcloud Assistant embedded into messaging, which could complicate things even more.

I agree with you.
I am a user of Talk for android and those are the things that I find more annoying.

I was thinking about developing the possibility to change voice messages reproduction speed as my next contribution

I'm happy to see that you are all very interested in the development and careful to the feedbacks πŸ”

Copy link

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants