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

criticalAY · 2025-02-08T21:41:05Z

Purpose / Description

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

Fixes

Fixes App becomes unresponsive after clicking Check media #17983

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 · 2025-02-08T21:41:16Z

Important

Maintainers: This PR contains Strings changes

Sync Translations before merging this PR and wait for the action to complete
Review and merge the auto-generated PR in order to sync all user-submitted translations
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

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/mediacheck/ConfirmMediaCheckDialog.kt

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/mediacheck/MediaCheckResultDialog.kt

AnkiDroid/src/main/java/com/ichi2/anki/dialogs/viewmodel/MediaCheckViewModel.kt

criticalAY · 2025-02-19T23:31:15Z

I have updated the latest test recording

To resolve the issue in

App becomes unresponsive after clicking Check media #17983

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

lukstbit

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.

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

AnkiDroid/src/main/java/com/ichi2/anki/mediacheck/ConfirmMediaCheckDialog.kt

AnkiDroid/src/main/java/com/ichi2/anki/mediacheck/MediaCheckAdapter.kt

lukstbit · 2025-02-23T14:32:09Z

AnkiDroid/src/main/res/layout/item_media_check.xml

+    android:id="@+id/file_name_textview"
+    android:layout_width="match_parent"
+    android:layout_height="wrap_content"
+    android:textSize="11dp"


No, this needs to be sp or look into one of the textApperance styles.

This wasn't fixed.

AnkiDroid/src/main/java/com/ichi2/anki/mediacheck/MediaCheckFragment.kt

AnkiDroid/src/main/java/com/ichi2/anki/mediacheck/MediaCheckViewModel.kt

AnkiDroid/src/main/res/values/03-dialogs.xml

lukstbit · 2025-02-23T15:44:31Z

AnkiDroid/src/main/java/com/ichi2/anki/mediacheck/MediaCheckFragment.kt

+                    R.string.check_media_delete_unused,
+                )
+
+            setOnClickListener {


This button shows a confirmation dialog before starting to delete and we should do this as well.

This wasn't implemented, show an AlertDialog here and ask for confirmation and then initiate the delete process.

feat: add media check fragment file - feat: adapter for media check result - refactor: confirm media check dialog

- refactor: replace media check result dialog with fragment - refactor: remove unused parameter in Media.kt

lukstbit · 2025-02-27T11:53:48Z

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt

@@ -1226,6 +1221,17 @@ open class DeckPicker :
        }
    }

+    private fun showMediaCheckDialog() {


Name proposal: showMediaCheckConfirmationDialog()

lukstbit · 2025-02-27T11:55:51Z

AnkiDroid/src/main/res/layout/item_media_check.xml

+    android:id="@+id/file_name_textview"
+    android:layout_width="match_parent"
+    android:layout_height="wrap_content"
+    android:textSize="11dp"


This wasn't fixed.

lukstbit · 2025-02-27T12:01:31Z

AnkiDroid/src/main/java/com/ichi2/anki/mediacheck/MediaCheckFragment.kt

+                    R.string.check_media_delete_unused,
+                )
+
+            setOnClickListener {


This wasn't implemented, show an AlertDialog here and ask for confirmation and then initiate the delete process.

lukstbit · 2025-02-27T12:10:17Z

AnkiDroid/src/main/java/com/ichi2/anki/mediacheck/MediaCheckFragment.kt

+            text =
+                TR.mediaCheckDeleteUnused().toSentenceCase(
+                    requireContext(),
+                    R.string.check_media_delete_unused,


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

github-actions bot added the Strings label Feb 8, 2025

criticalAY changed the title ~~Ref(MediaCheckDialog): Media check to use ViewModel~~ Refactor(MediaCheckDialog): Media check to use ViewModel Feb 8, 2025

criticalAY added the Blocked by dependency Currently blocked by some other dependent / related change label Feb 8, 2025

david-allison mentioned this pull request Feb 9, 2025

feat: Improve Check Media dialog #17842

Merged

5 tasks

david-allison reviewed Feb 9, 2025

View reviewed changes

AnkiDroid/src/main/java/com/ichi2/anki/DeckPicker.kt Outdated Show resolved Hide resolved

criticalAY force-pushed the media-dialog-viewmodel branch from bc072d9 to 7a503ca Compare February 9, 2025 17:14

github-actions bot added the Has Conflicts label Feb 10, 2025

criticalAY force-pushed the media-dialog-viewmodel branch 2 times, most recently from 18ef3d5 to 6b62901 Compare February 10, 2025 13:52

criticalAY added Needs Review and removed Blocked by dependency Currently blocked by some other dependent / related change Has Conflicts labels Feb 10, 2025

criticalAY requested a review from david-allison February 17, 2025 07:10

david-allison reviewed Feb 17, 2025

View reviewed changes

criticalAY force-pushed the media-dialog-viewmodel branch from 6b62901 to 337a7d4 Compare February 17, 2025 20:42

criticalAY added the Needs Author Reply Waiting for a reply from the original author label Feb 19, 2025

criticalAY force-pushed the media-dialog-viewmodel branch 3 times, most recently from 93508d4 to 124a991 Compare February 19, 2025 23:25

criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Feb 19, 2025

criticalAY force-pushed the media-dialog-viewmodel branch 2 times, most recently from 9090d16 to 37301c4 Compare February 19, 2025 23:47

criticalAY requested a review from david-allison February 19, 2025 23:47

david-allison added the Review High Priority Request for high priority review label Feb 20, 2025

lukstbit requested changes Feb 23, 2025

View reviewed changes

lukstbit added the Needs Author Reply Waiting for a reply from the original author label Feb 23, 2025

criticalAY force-pushed the media-dialog-viewmodel branch from 37301c4 to 6ef3989 Compare February 24, 2025 08:35

feat: media check viewmodel

e14d616

feat: add media check fragment file - feat: adapter for media check result - refactor: confirm media check dialog

criticalAY force-pushed the media-dialog-viewmodel branch from 6ef3989 to 425ac64 Compare February 24, 2025 09:01

refactor: remove unused code

961d458

- refactor: replace media check result dialog with fragment - refactor: remove unused parameter in Media.kt

criticalAY force-pushed the media-dialog-viewmodel branch from 425ac64 to 961d458 Compare February 24, 2025 09:08

criticalAY removed the Needs Author Reply Waiting for a reply from the original author label Feb 24, 2025

lukstbit requested changes Feb 27, 2025

View reviewed changes

lukstbit added the Needs Author Reply Waiting for a reply from the original author label Feb 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

criticalAY commented Feb 8, 2025 •

edited

Loading

github-actions bot commented Feb 8, 2025

criticalAY commented Feb 19, 2025 •

edited

Loading

lukstbit left a comment

lukstbit Feb 23, 2025

lukstbit Feb 27, 2025

lukstbit Feb 23, 2025

criticalAY Feb 23, 2025

lukstbit Feb 27, 2025

lukstbit Feb 27, 2025

lukstbit Feb 27, 2025

lukstbit Feb 27, 2025

lukstbit Feb 27, 2025

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

Are you sure you want to change the base?

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

Conversation

criticalAY commented Feb 8, 2025 • edited Loading

Purpose / Description

Fixes

Approach

How Has This Been Tested?

Checklist

github-actions bot commented Feb 8, 2025

criticalAY commented Feb 19, 2025 • edited Loading

lukstbit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

criticalAY commented Feb 8, 2025 •

edited

Loading

criticalAY commented Feb 19, 2025 •

edited

Loading