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

feat(check-pronunciation): Streamline UX #16248

Merged
merged 5 commits into from Apr 28, 2024

Conversation

david-allison
Copy link
Member

@david-allison david-allison commented Apr 22, 2024

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

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 : ↩️
    }
  1. Add 'on hold' logic for long presses

How Has This Been Tested?

Screen_Recording_20240422_184951_AnkiDroid.mp4

Learning (optional, can help others)

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

Checklist

  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • 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

Copy link
Contributor

@criticalAY criticalAY left a comment

Choose a reason for hiding this comment

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

Nice!! Especially the AudioRecorderController, using enum 👍 , Looks good to me!!

criticalAY

This comment was marked as resolved.

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

always daunting to see "large diffs hidden by default" type message in the commit viewer but ... all looks good, much more readable style with the enum state handling

@mikehardy mikehardy added the Needs Second Approval Has one approval, one more approval to merge label Apr 23, 2024
criticalAY

This comment was marked as resolved.

@david-allison

This comment has been minimized.

@david-allison david-allison added Needs reviewer reply Waiting for a reply from another reviewer and removed Needs Review labels Apr 23, 2024
@criticalAY

This comment has been minimized.

@david-allison

This comment has been minimized.

@david-allison david-allison removed the Needs reviewer reply Waiting for a reply from another reviewer label Apr 23, 2024
@criticalAY

This comment has been minimized.

@david-allison

This comment has been minimized.

@criticalAY

This comment was marked as resolved.

@BrayanDSO

This comment was marked as resolved.

@BrayanDSO BrayanDSO added the Needs Author Reply Waiting for a reply from the original author label Apr 26, 2024
@david-allison

This comment was marked as resolved.

@david-allison

This comment was marked as resolved.

@BrayanDSO

This comment was marked as resolved.

@david-allison

This comment was marked as resolved.

@BrayanDSO

This comment was marked as resolved.

@BrayanDSO BrayanDSO added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Author Reply Waiting for a reply from the original author Needs Second Approval Has one approval, one more approval to merge labels Apr 27, 2024
@david-allison david-allison force-pushed the check-pronunciation branch 2 times, most recently from 4f3a4d7 to 8a72fe5 Compare April 27, 2024 15:42
@david-allison david-allison added the Needs Author Reply Waiting for a reply from the original author label Apr 27, 2024
@david-allison
Copy link
Member Author

Blocked on #16248 (comment)

* Reviewer: make it greppable
* Controller: add logs

Issue 16185
`isPlaying` has been changed: no longer returns `true` if `ended`

refactor: `isRecordingPaused`
If a user recorded audio and did not play it
then the audio was retained when the next card
was shown
* cleanup: `.apply` and moving changes outside `draw`
* AudioWaveform_display_vertical_line
* AudioWaveform_android_background
When the audio recorder is on the reviewer
we want a single mode which includes playback
without pausing

This removes the 'save' button and associated state

record -> stop -> playback -> cancel

====

icons were added for:
* play
* stop

The record button was made slightly smaller to be consistent with this icon set

Issue 16185
@david-allison david-allison removed the Needs Author Reply Waiting for a reply from the original author label Apr 28, 2024
@david-allison david-allison added this pull request to the merge queue Apr 28, 2024
Merged via the queue into ankidroid:main with commit 9c5ce7c Apr 28, 2024
6 checks passed
@github-actions github-actions bot removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Apr 28, 2024
@github-actions github-actions bot added this to the 2.18 release milestone Apr 28, 2024
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.

Simplify "Pronunciation Check" feature for Reviewer
4 participants