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

Improved Voice Messages UI #365

Closed
wants to merge 24 commits into from

Conversation

metaphore
Copy link

New visual design for message audio attachments.

@metaphore
Copy link
Author

metaphore commented Oct 12, 2020

That should be fine for testing now.
@nielsandriesse could you please check on the styling? I followed iOS "foreground" color, but for the "background" counterpart, I use a bit muted version of the same color. I reckon it looks a bit more balanced, please let me know what you think.
image

@Brice-W
Copy link

Brice-W commented Oct 19, 2020

@metaphore I tested this PR, I found a few issues:

  1. Bug when replying to your own voice message:
    Send a voice message, then select it and write a reply, send it, and make another reply and send it: the app goes back to the conversations list UI automatically

  2. API 23:
    Send a message from an API > 23 to API 23: the voice message UI looks different.
    Still on API 23: after receiving and sending a voice message, go back to the main UI, then reopen the conversation with the voice messages: the voice messages UI keep changing.

  3. API 21 & 22: voice messages aren’t working (the issue is already present in production, so unrelated to this PR, but if it’s easy and quick to fix that’d be good to include that fix)

@nielsandriesse
Copy link

A few things:

• If I tap a voice message while it's still loading, the app crashes.
• Style-wise things are looking nice (the adjusted background color you used looks good to me), but there are still a few differences with respect to iOS:

https://jmp.sh/K36ywLW
https://jmp.sh/SjHFdwB

• Let's cache the volume samples in the database. I do this on iOS because parsing them out is a relatively expensive operations (especially for long voice messages).
• Also, if you could take a look at @Brice-W's comments above that'd be great :)

@metaphore
Copy link
Author

metaphore commented Oct 26, 2020

@Brice-W Should be good to test now.

Bug when replying to your own voice message

I couldn't reproduce that. Maybe it was fixed in the process.

API 21 & 22: voice messages aren’t working

It works for me on 21, but not on 22. There's an issue with the media player we are using, it might be incompatible with some system configurations, but it doesn't look like an easy fix could solve that. I can look into that but in a separate task.

the voice message UI looks different

For the APIs < 23 there is no support to extract the required data from audio files, thus fake waveform data is displayed.
Also on my API 23 emulator, there's a missing data field during the audio parsing process (which should be present according to the docs), so it fails and generates fake data again.
For other APIs, it might have small variations but in general, should look the same. Please let me know if you find any other inconsistencies in that regard.

@metaphore
Copy link
Author

@Brice-W I found a way to patch the issue related to API 23, now it should work as expected.

@Brice-W
Copy link

Brice-W commented Oct 26, 2020

@metaphore I tested the 3 cases again

  1. I can't reproduce it as well, so it was fixed somehow 👍
  2. API 23 works well :)
  3. voice messages now work on both API 21 and 22. The UI looks different than other UI (like it did earlier in 23), but as long as it is working that's fine.

I just saw something else on APIs 21 -> 23: the voice message duration isn't displayed on the right of the message

@metaphore
Copy link
Author

Thanks @Brice-W

on APIs 21 -> 23: the voice message duration isn't displayed on the right of the message

Oh yeah, sorry, forgot to mention that. It's the same as

or the APIs < 23 there is no support to extract the required data from audio files

So I cannot read it along with the raw audio data and thus the duration is just left empty and not displayed.

…into audio-view-design-update

# Conflicts:
#	src/org/thoughtcrime/securesms/database/helpers/SQLCipherOpenHelper.java
@nielsandriesse nielsandriesse changed the title Audio view design update Improved Voice Messages UI Oct 28, 2020
@nielsandriesse
Copy link

Merged through #370 (except for the last dev merge)

View.inflate(context, R.layout.message_audio_view, this)
container = findViewById(R.id.audio_widget_container)
controlToggle = findViewById(R.id.control_toggle)
playButton = findViewById(R.id.play)

Choose a reason for hiding this comment

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

Just for future reference, you don't need to do this in Kotlin :) . You can just reference the view directly without first doing findViewById(...), assigning it, etc.

Choose a reason for hiding this comment

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

This is also why we have the convention of using lower camel case for view IDs

Copy link
Author

@metaphore metaphore Oct 28, 2020

Choose a reason for hiding this comment

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

That was done intentionally. Kotlin synthetics are no longer recommended practice by Google https://www.reddit.com/r/androiddev/comments/ala9p2/why_kotlinx_synthetic_is_no_longer_a_recommended/?utm_source=amp&utm_medium=&utm_content=post_num_comments

They slow down things a bit and View Binding seems like a good alternative https://developer.android.com/topic/libraries/view-binding

Choose a reason for hiding this comment

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

Oh okay then, what a Google move haha

Copy link
Author

Choose a reason for hiding this comment

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

New day, new library at Google :) once this thing is out of alpha it might become the default xml view markup replacement https://developer.android.com/jetpack/compose

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

Successfully merging this pull request may close these issues.

None yet

3 participants