feat(check-pronunciation): Streamline UX

david-allison commented 2 weeks ago

Purpose / Description

A few issues/suggestions were raised in #16185 about the audio recorder changes introduced for #13043

takes up too much space vertically
increased number of touches required
❓ stops working after a while
❌ move the recorder to the bottom of the screen
✅ A long-press should start, releasing the long-press should stop

Fixes

Fixes #16185

Approach

1) Major refactor to improve the state handling logic 2) Simplify the Reviewer state to the following, add a new layout and changes to the Waveform control:

stateDiagram-v2
    direction LR
    [*] --> Empty

    Empty --> Recording : ⏺️
    Recording --> Empty : ❎
    Recording --> Playback : ⏹️
    Playback --> Empty : ❎

    state Playback {
        [*]  --> p1
        p1: Paused
        p2: Playing

        p1 --> p2 : ▶️
        p2 --> p1 : ↩️
    }

3) Add 'on hold' logic for long presses

How Has This Been Tested?

https://github.com/ankidroid/Anki-Android/assets/62114487/6fb5c329-152e-4c89-b231-f2ac2ac4395c

Learning (optional, can help others)

onStop
Adding custom attributes to Views
MaterialButton is hard when it comes to centering icons

Checklist

[x] You have a descriptive commit message with a short title (first line, max 50 chars).
[x] You have commented your code, particularly in hard-to-understand areas
[x] You have performed a self-review of your own code
[x] UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
[ ] UI Changes: You have tested your change using the Google Accessibility Scanner
- [ ] ⚠️ no issues with touch targets. Historical issues with 'item label' - proposing to extract to another issue

david-allison commented 2 weeks ago

@criticalAY Can't reproduce

Could you check 0a629925b427cc21a8c7877d19d31fe3522a8343 and 7879edfa469178e5d974cdadbd7a13846e44845e to find the bad commit

You'll need gh pr checkout --force 16248

https://github.com/ankidroid/Anki-Android/assets/62114487/f7d82d2f-7610-4770-80f8-a460d658370b

criticalAY commented 2 weeks ago

I tested it on my physical device and to no amaze it's an emulator issue, you can ignore it, looks good to me

david-allison commented 2 weeks ago

Is the emulator issue introduced in this PR, or reproducible on main?

criticalAY commented 2 weeks ago

This PR too

david-allison commented 2 weeks ago

For clarity: And also main?

criticalAY commented 2 weeks ago

Yes, both main and this PR have issue on emulator

BrayanDSO commented 2 weeks ago

My concerns:

The Replay icon stops the audio instead of replaying it, which is unintuitive to me, so I suggest using another icon.
There shouldn't be a delay between long pressing the record button and the recording start.

david-allison commented 2 weeks ago

My concerns:

The Replay icon stops the audio instead of replaying it, which is unintuitive to me, so I suggest using another icon.

If I can find an icon similar to this, would it be acceptable?

There shouldn't be a delay between long pressing the record button and the recording start.

So this would be moving 'record' to ACTION_DOWN?

david-allison commented 2 weeks ago

Crash on closing the screen while recording is playing back

 Process: com.ichi2.anki.debug, PID: 9262
 java.lang.IllegalStateException
    at android.media.MediaPlayer._pause(Native Method)
    at android.media.MediaPlayer.pause(MediaPlayer.java:1573)
    at com.ichi2.audio.AudioRecordingController.controlAudioRecorder(AudioRecordingController.kt:483)
    at com.ichi2.audio.AudioRecordingController.createUI$lambda$4(AudioRecordingController.kt:190)
    at com.ichi2.audio.AudioRecordingController.$r8$lambda$Kt6IrF7armXMsCjnga1mPK4zvx0(Unknown Source:0)
    at com.ichi2.audio.AudioRecordingController$$ExternalSyntheticLambda1.onClick(Unknown Source:2)
    at android.view.View.performClick(View.java:8043)
    at android.widget.TextView.performClick(TextView.java:17816)
    at com.google.android.material.button.MaterialButton.performClick(MaterialButton.java:1218)
    at android.view.View.performClickInternal(View.java:8020)
    at android.view.View.-$$Nest$mperformClickInternal(Unknown Source:0)
    at android.view.View$PerformClick.run(View.java:31850)
    at android.os.Handler.handleCallback(Handler.java:958)
    at android.os.Handler.dispatchMessage(Handler.java:99)
    at android.os.Looper.loopOnce(Looper.java:230)
    at android.os.Looper.loop(Looper.java:319)
    at android.app.ActivityThread.main(ActivityThread.java:8893)
    at java.lang.reflect.Method.invoke(Native Method)
    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:608)
    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1103)

BrayanDSO commented 2 weeks ago

If I can find an icon similar to this, would it be acceptable?

I won't block because of the icon, so choose what you like more.

So this would be moving 'record' to ACTION_DOWN?

Yes. Either record while pressing, or press once then stop when pressed again

david-allison commented 2 weeks ago

I'll propose to drop the final commit so this can go in, would that be acceptable?

BrayanDSO commented 2 weeks ago

I'll propose to drop the final commit so this can go in, would that be acceptable?

Yeah. Add an issue for the holding feature then

david-allison commented 2 weeks ago

Blocked on https://github.com/ankidroid/Anki-Android/pull/16248#issuecomment-2080812264

ankidroid / Anki-Android