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

[IMPROVE] Voice messages #3373

Closed
wants to merge 1 commit into from
Closed

[IMPROVE] Voice messages #3373

wants to merge 1 commit into from

Conversation

jacotec
Copy link
Contributor

@jacotec jacotec commented Sep 10, 2021

Proposed changes

Change voice message container to m4a which gives correct length in Android, IOS, Chrome, Edge and Firefox
Enable background mode for voice playback, so listening to voice messages is possible when the app is put into background

Issue(s)

Fixes #2586 and #2578

How to test or reproduce

  • Record a voice message with the app at IOS or Android. The message length is correctly displayed on IOS, Android and the web client
  • Playback a voice message and put the app into background (i.e. by changing to another app) - the playback continues

Screenshots

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves a current function)
  • New feature (non-breaking change which adds functionality)
  • Documentation update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works (if applicable)
  • I have added necessary documentation (if applicable)
  • Any dependent changes have been merged and published in downstream modules

Further comments

  • Fixes wrong length display of voice messages (especially recorded on IOS)
  • Continue playback of voice messages when the app is put into background

- Fixes wrong length display of voice messages (especially recorded on IOS)
- Continue playback of voice messages when the app is put into background
Copy link
Contributor

@gerzonc gerzonc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gerzonc gerzonc changed the title [FIX] Voice messages improvements [IMPROVE] Voice messages Sep 16, 2021
@diegolmello
Copy link
Member

@jacotec I can't checkout your code to resolve conflicts.
image

Do you mind giving us permission? https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Thanks!

@jacotec
Copy link
Contributor Author

jacotec commented Sep 16, 2021

@jacotec I can't checkout your code to resolve conflicts.
image

Do you mind giving us permission? https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Thanks!

Hi @diegolmello
"Allow edits by maintainers" is checked at my PR ... what can I do else?

@diegolmello
Copy link
Member

@jacotec Can you update your branch then?
I'll reviewing/merging stuff, but I wait for you if you can do it right away.

@jacotec
Copy link
Contributor Author

jacotec commented Sep 16, 2021

@jacotec Can you update your branch then?
I'll reviewing/merging stuff, but I wait for you if you can do it right away.

@diegolmello Can you help a mid-50's guy who is working 15 years with SVN and who's getting his head into JavaScript and GitHub to fix this voice message stuff how I should do this? ;-)

I've forked the project, updated my fork with my changes, created a PR to the upstream project, enabled "edit by maintainers" ... what can I do to to help you here? What means "update my branch"?

BTW: I'm currently working on #3002 and I just need to iron out a last issue ... what's the deadline for the next version? Would be fine to have the voice message fixes all in one package :-)

@diegolmello
Copy link
Member

diegolmello commented Sep 16, 2021

Easier to fix this: I close this PR, get your code and then I open a separate PR.

If you send me your email, I can add you as co-author so you get credits on the commit.
Can we do it that way?
I can make it in a few, but for sure it'll land next release (beta coming next Tuesday~Wed).

@jacotec
Copy link
Contributor Author

jacotec commented Sep 16, 2021

Easier to fix this: I close this PR, get your code and then I open a separate PR.

If you send me your email, I can add you as co-author so you get credits on the commit.
Can we do it that way?
I can make it in a few, but for sure it'll land next release (beta coming next Tuesday~Wed).

Hi @diegolmello

Sure ... mj -at- jacotec -dot- de ;-)

I'm a bit stuck @ #3002 with a last GUI issue ... if I can't solve this tomorrow, can I send it to you for your opinion? Just thinking of the full package for next week.

@diegolmello
Copy link
Member

@jacotec Yep. Thanks.

@jacotec
Copy link
Contributor Author

jacotec commented Sep 16, 2021

@diegolmello How can I contact you in case I'm still stuck tomorrow? Can you send me your email by email? ;-)

@diegolmello
Copy link
Member

@diegolmello
Copy link
Member

Closed in favor of #3385

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