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

Voice Message #3598

Merged
merged 69 commits into from
Jul 30, 2021
Merged

Voice Message #3598

merged 69 commits into from
Jul 30, 2021

Conversation

onurays
Copy link
Contributor

@onurays onurays commented Jul 1, 2021

Fixes #29

https://www.figma.com/file/uaWc62Ux2DkZC4OGtAGcNc/Voice-Messages?node-id=546%3A27109

Default state:

image

When text is not blank:

image

Voice Message initial state:

image

Sliding to cancel:

image

Locking:

image

Locked:

image

Playback before sending:

image

Timeline:

image

@bmarty
Copy link
Member

bmarty commented Jul 1, 2021

The CI is very unhappy, can you have a look please? Thanks!

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Just some remarks after a first quick review

Onuray Sahin added 4 commits July 6, 2021 13:17
* develop: (286 commits)
  Fix crash after video call
  Fix issue on button styles
  Clean after benoits review
  Fix warning about implicit type, introduced in Kotlin 1.5.20. "Returning type parameter has been inferred to Nothing implicitly. Please specify type arguments explicitly to hide this warning. Nothing can produce an exception at runtime."
  Bump kotlin_version from 1.5.10 to 1.5.20
  Bump libphonenumber from 8.12.25 to 8.12.26
  Fix call invite processed after call is ended because of fastlane mode.
  Jump to unread: removes unnecessary check which can cause scroll issue
  Jump to unread: avoid blink when jumping
  Clean after Benoits review
  Delete unused drawable to avoid conflict on develop
  Jump to unread: add towncrier file.
  Read marker: fix some issues with jump to unread visibility.
  Stop using ProgressDialog, there is a theme issue with it. It's not maintain by Google since it's deprecated. Force usage of MaterialAlertDialogBuilder to have the same UI effect. We sometimes need to block the UI :/
  Reordering
  Add text style for dialogs
  Colored dialog button is now handled by the theme
  Update theme for material dialog and create a destructive variant
  Reorder buttons
  Update doc
  ...

# Conflicts:
#	library/ui-styles/src/main/res/values/theme_dark.xml
#	library/ui-styles/src/main/res/values/theme_light.xml
#	vector/build.gradle
#	vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailFragment.kt
#	vector/src/main/java/im/vector/app/features/home/room/detail/RoomDetailViewModel.kt
#	vector/src/main/java/im/vector/app/features/home/room/detail/composer/TextComposerView.kt
#	vector/src/main/res/values/strings.xml
@onurays onurays requested a review from gaelledel July 7, 2021 07:51
@bmarty
Copy link
Member

bmarty commented Jul 7, 2021

Good work!

Some remarks, I will update the list for each new remark:

In the composer

  • When disabled from the labs, the "send" icon animation is not there anymore
  • I the user denies the microphone permission and click "do not ask again", the user is stuck if he clicks on the microphone again. We should at least display a dialog in this case
  • The animation "Send" -> "Mic" is OK, but the animation "Mic" -> "Send" is glitchy (empty the composer to see it)
  • If I click fast on the microphone, the Toast "Release to send" is displayed
  • The toast is not themed. From Figma:

image

  • The lock should be activated only if I stop touching the screen
  • When starting the recording, the timer displays "00:01". It should start from "00:00"
  • In landscape mode, the slide to cancel is too long, the length of the path should be limited.
  • If I wait for the 2 minutes limite, the app crash (did not repro...)
  • If I start the app again, I get an infinite item:

image

In the timeline

  • The pause icon is not centered:

image

  • Once the message has been played in the timeline, sometimes, I cannot play it again
  • I cannot swipe to reply (but I can reply with long press, "Reply")
  • Long press on the waves has no effect. It should open the bottom sheet
  • If I start a playback, the timer does not display immediately "00:00", there is a delay
  • Last item do not have the same width:

image

  • The trash icon is hard to click. Need some padding

Technical

  • In the Event content, the field content.info.duration is set to 0. The value is well set for content.org.matrix.msc1767.audio.duration
  • After recording, I can see error in loop in the logcat: E/VoiceMessageHelper$startRecordingAmplitudes: Cannot get max amplitude. Amplitude recording timer will be stopped. java.lang.IllegalStateException. Something is not released properly.
  • Add support for API 21. OGG support if 29 min
  • Support for RTL languages
  • Add FFMpeg entry in open source license file

Copy link

@gaelledel gaelledel left a comment

Choose a reason for hiding this comment

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

  • The recording should start after 1 sec upon user pressing on the mic button There is already a 1s limit to prevent accidental records.

  • The release to send action should only work after 1 sec of recording. If the user releases their finger before this time margin, the recording should not be sent and the composer should return to its default state

  • In the release to send toast, please add : " Hold to record, release to send"

  • Once the recording starts, add a vibration to indicate that the recording is started

  • Once the recording is started, and I start to slightly move my finger towards the padlock, the Slide to cancel and recording timer strings overlap

  • The recordings in the timeline should always have the same length no matter the duration of the recording. They should always be filled with a set length for the waveform

  • Cancelling action: Once users start dragging The Slide to cancel string, its opacity should start reducing (see the example given)

  • Cancelling action: Once the cancelling is performed, (upon finger release halfway through the UI) the Mic shouldn't animate in its default position but should simply return to its default state minus the animation

  • Locking mode: As I move my finger towards the padlock, it should become green and its shape should return to a circle that shrinks down in size. (see Figma or iOS implementation)

  • Locking mode: Once users enter the lock mode a toast should be displayed with the following string " Tap on the wavelength to stop and playback"

  • Recordings are currently set to 2 min. At 1:50, a toast should be displayed "10sec left" add vibration. Currently, a toast gets displayed at 11 sec left, then 10 sec. In locked mode, if the time limit is reached the recording should not be sent automatically, instead, it should go into playback mode. In unlocked mode, we should allow the recording to be sent automatically as soon as the time limit is reached.

  • Playback mode: Once in playback mode, there is a UI bug. The Delete icon overlaps with the expand menu button. This means that it causes the menu to open instead of allowing users to delete their recording

  • Playback mode: we can see the composer frame edge

  • In playback mode, the pause icon isn't centred properly

  • The emoji icon needs to be changed to https://www.figma.com/file/uaWc62Ux2DkZC4OGtAGcNc/Voice-Messages?node-id=562%3A1344

  • Change based on our discussion: if a user navigates back whilst a recording is ongoing and returns in the chatroom, the recording should be discarded and the composer should return to its default state.

  • Replace dialog strings upon first interaction with the MIC with: Title: Title: "Element would like to access the microphone on your device
    Body "Element needs to access your microphone to make and receive calls, take videos, and record voice messages."

  • Change based on our discussion: On reply mode replace random text string with Voice message

  • - [ ] For this PR, disable reply/edit mode if the user is recording a voice message (locked mode)

TEST BUILD 1.1.12-dev 40101132 / 26th Jul 21 by GD

28/07/21 Live testing

  • Increase the size of the MIC button when users interact with it
  • On default mode the MIC icon isn't centered. There is too much padding on the left and less padding on the right. Can we correct so we have equal distance padding?

@gaelledel
Copy link

gaelledel commented Jul 7, 2021

Reminder

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Please see my comments

@bmarty
Copy link
Member

bmarty commented Jul 7, 2021

Rendering in the timeline of the wave form:

EW

image

EA

image

  • There is something to check here (EDIT: OK, fixed)

@bmarty
Copy link
Member

bmarty commented Jul 7, 2021

  • When receiving a voice message, the notification could be improved. Now I see:

image

@Derad6709
Copy link

And one small remark: I think it will be possible to select headphones mic or device mic I want to record the audio.
Thanks :)

@fbruetting
Copy link

fbruetting commented Jul 16, 2021

  • While you're at it: In WhatsApp if you play and pause a voice message, the position gets moved back by one second - so when you continue, you notice where you paused. That's very nice, would be cool if you could add this, too! 😎

Now use limit for distances
Still a pb when Mic is on, margins are not correct
@bmarty
Copy link
Member

bmarty commented Jul 20, 2021

To be converted to an issue if not done for the release

  • The overhead added by the FFMpeg lib seems to be 10Mb. We could probably try to build the library again by removing what is not used to reduce this overhead. iOS team is looking at this point too.

@XayOn
Copy link

XayOn commented Jul 25, 2021

I've been testing this branch, and I found a usability bug.
Whenever you go to home screen and back to the app, with text already written, it loads the "mic" icon... and as it has text, it loads the normal send button too, leaving the app as:
sZMcnPxmKMPBaCEjazLaCkmy

@nadonomy
Copy link

nadonomy commented Jul 26, 2021

Some issues from testing, can share screenshots if useful:

  • Red dot next to timer is misaligned, its a couple of px too high.
  • Sliding left to cancel doesn't succeed gracefully. The UI just kind of transitions underneath you abruptly with no motion.
  • Sliding up to lock also doesn't succeed gracefully. We need some kind of better feedback, and nicer transitions between states, when completing these two actions.
  • Sliding up to lock UI is misaligned. The background/sliding elements should be horizontally aligned to the button being slided.
  • It's way too easy for a slide to cancel to not succeed. If you record a message, slide/flick the button and release you'd expect it to cancel, but it instead sends. Considering the failure mode is sending a voice message you don't want to, we should make this way more generous.
  • Sliding to cancel doesn't clear whatever floating tooltip is being shown (e.g. 'Hold to record, release to send') so it hangs around contextless until it times out.

@DoM1niC
Copy link

DoM1niC commented Jul 27, 2021

VoiceRecordBtn overlap after someone is typing.... with the SendBtn :( @onurays

Sry @XayOn Posted already this issue 😅

roomDetailViewModel.handle(RoomDetailAction.EnterEditMode(action.eventId, views.composerLayout.text.toString()))
} else {
requireActivity().toast(R.string.error_voice_message_cannot_reply_or_edit)
}
}
is EventSharedAction.Quote -> {
roomDetailViewModel.handle(RoomDetailAction.EnterQuoteMode(action.eventId, views.composerLayout.text.toString()))
Copy link
Member

Choose a reason for hiding this comment

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

You should disable this as well

waveform = if (amplitudeList.size < 50) {
amplitudeList
} else {
amplitudeList.chunked(amplitudeList.size / 50) { items -> items.maxOrNull() ?: 0 }
Copy link
Member

Choose a reason for hiding this comment

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

So the limit of waveform list size on the SDK can be removed / commented out?

@pvagner
Copy link
Contributor

pvagner commented Jul 29, 2021

Hello,
I have got a chance to test this in action and I am afraid about accessibility support:

  • In the timeline play button is not labelled. Play icon is swapped with the pause icon depending on the player state but accessibility description is not reflecting the playback status.
  • Record button aka microphone icon is hidden from the view when recording or playing back the audio, however accessibility services can still navigate to that icon.
  • When in locked mode, the waveform visualization is useless to visually disabled people, perhaps it should have content description saying stop recording.
  • When holding down the microphone icon to start recording I can feel the vibration signalling the recording has been started. Still holding down that icon and swiping to the left to cancel the recording I can't feel the vibration.
    Edit: Both hold and send mode as well as lock mode are useable with the screen reader once content description related issues are resolved.
    Thanks to everyone working on this, this sounds amazing!

@fbruetting
Copy link

I never programmed Android, but a general question: A lot of the above mentioned issues sound like you don't use a state machine - is there a reason for this? I'm pretty shure that would make it much easier and less error prone. 🤔

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, remaining issues will be handled in the coming days. Good work!

@onurays onurays merged commit c6bd6e4 into develop Jul 30, 2021
@onurays onurays deleted the feature/ons/voice_message branch July 30, 2021 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Send and play voice messages
9 participants