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.

Sign up for GitHub

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

Jump to bottom

feat(new reviewer): type in the answer (native) #17647

Merged
merged 11 commits into from
Dec 23, 2024
Merged

feat(new reviewer): type in the answer (native) #17647

merged 11 commits into from
Dec 23, 2024

Conversation

BrayanDSO
Copy link
Member

@BrayanDSO BrayanDSO commented Dec 22, 2024 •
edited
Loading

Purpose / Description

Implements type in the answer cards in the new reviewer natively. I don't know when/if I will implement the HTML version

Fixes

Approach

In the commits

How Has This Been Tested?

Emulator 34

ok.mp4
aaa.mp4

Learning (optional, can help others)

  • InputMethodManager.restartInput can be used to restart the IME language hint immediately (it's on the javadoc of imeHintLocales)
  • WindowInsetsControllerCompat can be used to reliably show the IME (source)

I should learn more about Android MVVM and flow, specially to better handle configuration changes. Also, ReviewerFragment is getting big. Not sure if I can split some of it into other files, or if I should. The reviewer does a lot of things anyway

Checklist

Please, go through these checks before submitting the PR.

  • 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

@BrayanDSO BrayanDSO force-pushed the type-answer branch 3 times, most recently from 0ae2f0f to 42188e4 Compare December 22, 2024 10:26
@BrayanDSO BrayanDSO force-pushed the type-answer branch 2 times, most recently from 2ad64d1 to 2776678 Compare December 22, 2024 12:07
mikehardy
mikehardy approved these changes Dec 22, 2024
View reviewed changes
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.

Wow - just casually dropping this in the PR queue 🔥
I'll re-read the densest commits at least one more time but nothing stood out on the first tread and I'd prefer to move faster rather than slower as it's still gated behind a dev preference and we're early in an alpha cycle anyway, so +1 in general for "let's go, faster rather than slower" as a personal opinion on review+merge

Thanks @BrayanDSO !

david-allison
david-allison reviewed Dec 23, 2024
View reviewed changes
Copy link
Member

@david-allison david-allison left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Note: code review only, not tested physically. Treat this as an approve

This is excellent, thank you!

Nothing blocking. I feel Field would simplify this even further, but fully 'implementer's choice'.

AnkiDroid/src/main/java/com/ichi2/anki/servicelayer/LanguageHintService.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/previewer/TypeAnswer.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/previewer/TypeAnswer.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/previewer/TypeAnswer.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/previewer/TypeAnswer.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/utils/ext/Fragment.kt Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/ui/windows/reviewer/ReviewerFragment.kt Outdated Show resolved Hide resolved
AnkiDroid/src/main/res/layout/reviewer2.xml Show resolved Hide resolved
@david-allison david-allison added Needs Author Reply Waiting for a reply from the original author Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) and removed Needs Review labels Dec 23, 2024
@david-allison david-allison mentioned this pull request Dec 23, 2024
Open
5 tasks
@BrayanDSO
feat(previewers): non combining typed answers
66820fc 
Original PR and motivation: ankitects/anki#3422 (comment)

Diff: https://github.com/ankitects/anki/pull/3422/files#diff-7c3df145bba194e3cdb8114cb4bd77c03e431c29c82a7620b4f4df599cd4b450

The implementation is also being refactored into a class to better handle all of the different details around type in answer, e.g. combining, expected answer and language hint
@BrayanDSO
docs+rename: showAnswerInternal and showAnswer
3f86dca
@BrayanDSO
rename: CardView.PreviewerStyle
9f2042d 
to CardView.ViewerStyle

made by the IDE
@BrayanDSO
refactor: add Fragment.window util
7819877
@BrayanDSO
refactor: add typedAnswer param to typeAnsFilter
effba4f 
separated from the next commit to ease the reviewing process
@BrayanDSO BrayanDSO removed the Needs Author Reply Waiting for a reply from the original author label Dec 23, 2024
@BrayanDSO
feat(new reviewer): type answer (native)
e1e7ce9
@BrayanDSO
refactor: replace ConstraintLayouts in reviewer2.xml
c85978e 
They were initially used because of the WIP nature of the reviewer, so they would be able to handle any design decisions

Now that most of the reviewer is implemented, I don't see a need for using ConstraintLayouts anymore, so I am replacing them with LinearLayouts

LinearLayouts are simpler to deal with, have less boilerplate, and make easier to preview changes. They also render faster
@BrayanDSO
refactor: replace ConstraintLayouts of template previewer
c7aa834 
same rationale of the previous commit
@mikehardy
Copy link
Member

Let's go! 🚂

@mikehardy mikehardy added this pull request to the merge queue Dec 23, 2024
Merged via the queue into ankidroid:main with commit 16f88cf Dec 23, 2024
9 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 Dec 23, 2024
@github-actions github-actions bot added this to the 2.21 release milestone Dec 23, 2024
@BrayanDSO BrayanDSO deleted the type-answer branch December 23, 2024 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-allison david-allison david-allison left review comments

@mikehardy mikehardy mikehardy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.21 release
Development

Successfully merging this pull request may close these issues.
3 participants
@BrayanDSO @mikehardy @david-allison