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

Refactor(MediaCheckDialog): Media check to use ViewModel #17943

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Refactor(MediaCheckDialog): Media check to use ViewModel #17943

wants to merge 2 commits into from

Conversation

criticalAY
Copy link
Contributor

@criticalAY criticalAY commented Feb 8, 2025 •
edited
Loading

Purpose / Description

Migrates the logic of MediaCheck to ViewModel
Split the dialog logic, use different dialog for result and conformation

Fixes

Approach

Use ViewModel to handle the media check dialog and its result

How Has This Been Tested?

Tested on Google emulator API31
Screen_recording_20250220_045658.webm

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

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 8, 2025

Important

Maintainers: This PR contains Strings changes

  1. Sync Translations before merging this PR and wait for the action to complete
  2. Review and merge the auto-generated PR in order to sync all user-submitted translations
  3. Sync Translations again and merge the PR so the huge automated string changes caused by merging this PR are by themselves and easy to review

@criticalAY criticalAY changed the title Ref(MediaCheckDialog): Media check to use ViewModel Refactor(MediaCheckDialog): Media check to use ViewModel Feb 8, 2025
@criticalAY criticalAY added the Blocked by dependency Currently blocked by some other dependent / related change label Feb 8, 2025
@david-allison david-allison mentioned this pull request Feb 9, 2025
Merged
5 tasks
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch from bc072d9 to 7a503ca Compare February 9, 2025 17:14
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch 2 times, most recently from 18ef3d5 to 6b62901 Compare February 10, 2025 13:52
@criticalAY criticalAY added Needs Review and removed Blocked by dependency Currently blocked by some other dependent / related change Has Conflicts labels Feb 10, 2025
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch from 6b62901 to 337a7d4 Compare February 17, 2025 20:42
@criticalAY criticalAY added the Needs Author Reply Waiting for a reply from the original author label Feb 19, 2025
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch 3 times, most recently from 93508d4 to 124a991 Compare February 19, 2025 23:25
@criticalAY criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Feb 19, 2025
@criticalAY
Copy link
Contributor Author

criticalAY commented Feb 19, 2025 •
edited
Loading

  • I have updated the latest test recording

To resolve the issue in

I am migrating the result from dialog to fragment. Why? Because the string builder is taking a lot of time to render strings i.e. 10k or 30k file names etc so its a heavy task for it, I have replicated the Anki behaviour in the fragment i.e. how it closes and how the user can interact with it

@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch 2 times, most recently from 9090d16 to 37301c4 Compare February 19, 2025 23:47
@david-allison david-allison added the Review High Priority Request for high priority review label Feb 20, 2025
lukstbit
lukstbit requested changes Feb 23, 2025
View reviewed changes
Copy link
Member

@lukstbit lukstbit left a comment

Choose a reason for hiding this comment

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

I don't see why you introduced the current granularity with the number of commits. Reverting any of the first three commits will render the other two commits useless as the content is tightly tied together. It also makes reviewing harder.

So please squash the first three commits into a single commit.

Requested some changes, the implementation seems fine.

@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Feb 23, 2025
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch 2 times, most recently from 6ef3989 to 425ac64 Compare February 24, 2025 09:01
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch from 425ac64 to 961d458 Compare February 24, 2025 09:08
@criticalAY criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Feb 24, 2025
lukstbit
lukstbit requested changes Feb 27, 2025
View reviewed changes
AnkiDroid/src/main/java/com/ichi2/anki/mediacheck/MediaCheckFragment.kt
text =
TR.mediaCheckDeleteUnused().toSentenceCase(
requireContext(),
R.string.check_media_delete_unused,
Copy link
Member

Choose a reason for hiding this comment

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

This string is used only here. It should be removed from 03-dialogs.xml and added to sentence case xml.

@lukstbit lukstbit added the Needs Author Reply Waiting for a reply from the original author label Feb 27, 2025
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch 2 times, most recently from 51dc85f to ea860b9 Compare February 28, 2025 07:36
@criticalAY
feat: media check viewmodel
984c88b 
feat: add media check fragment file
- feat: adapter for media check result
- refactor: confirm media check dialog
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch from ea860b9 to a54afbf Compare February 28, 2025 07:39
@criticalAY criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Feb 28, 2025
@criticalAY
refactor: remove unused code
73ce17e 
- refactor: replace media check result dialog with fragment
- refactor: remove unused parameter in Media.kt
@criticalAY criticalAY force-pushed the media-dialog-viewmodel branch from a54afbf to 73ce17e Compare March 2, 2025 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukstbit lukstbit lukstbit requested changes

@david-allison david-allison Awaiting requested review from david-allison

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Needs Review Review High Priority Request for high priority review Strings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

App becomes unresponsive after clicking Check media
3 participants
@criticalAY @lukstbit @david-allison