Manual-Download-link #17351

disconnect821 · 2024-11-02T17:28:24Z

Purpose / Description

The user can download the deck manually if download fails.

Fixes

Fixes Improve "download failed" message for Shared Decks #17178

Approach

This change allows users to manually download decks from AnkiWeb through an external browser if the download fails in the in-app WebView. By using an Intent, we redirect users to their preferred browser.

Once the download is initiated, the app pops the fragment stack.

Steps to reproduce the download failed error.

1 : Comment out the downloadFile() function call in the onViewCreated method.
2 : Instead, call checkDownloadStatusAndUnregisterReceiver(false, false).

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]
(https://play.google.com/store/apps/details?id=com.google.android.apps.accessibility.auditor)

welcome · 2024-11-02T17:28:27Z

First PR! 🚀 We sincerely appreciate that you have taken the time to propose a change to AnkiDroid! Please have patience with us as we are all volunteers - we will get to this as soon as possible.

github-actions · 2024-11-02T17:28:36Z

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

disconnect821 · 2024-11-02T18:18:39Z

Task Remaining

UI changes (The format of the clickable text "Download from AnkiWeb")
Sync translations.
Other suggested changes.

disconnect821 · 2024-11-03T09:34:43Z

Hi @david-allison ! I've opened a draft PR for the issue #17178. Could you review it to see if it meets the desired result, and let me know if there are any changes needed?

david-allison

Looks good!

A few nitpicks. Main issue is the target of the URL

I would add a few @NeedsTest annotations, or unit tests for the functionality

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

david-allison · 2024-11-04T12:01:05Z

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

+        app:layout_constraintEnd_toEndOf="parent"
+        app:layout_constraintStart_toStartOf="parent"
+        app:layout_constraintTop_toBottomOf="@+id/download_progress"
+        app:layout_constraintVertical_bias="0.958" />


Probably not what you want. Can you represent this another way?

Replaced TextView with Button and updated constraints accordingly.

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

david-allison

Looks great! Sorry for the slow response, been really busy

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

david-allison · 2024-11-10T21:34:54Z

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

+        android:id="@+id/download_from_ankiWeb"
+        android:layout_width="match_parent"
+        android:layout_height="wrap_content"
+        android:text="@string/download_deck_from_AnkiWeb"
+        android:background="@color/material_blue_500"
+        android:textColor="@color/white"
+        android:padding="8dp"
+        android:layout_marginBottom="24dp"
+        android:layout_marginStart="24dp"
+        android:layout_marginEnd="24dp"
+        app:layout_constraintStart_toStartOf="parent"
+        app:layout_constraintEnd_toEndOf="parent"
+        app:layout_constraintBottom_toTopOf="@id/try_again_deck_download"
+        android:visibility="gone"


I couldn't see on the gif, but I'd expect this to either be a material 'text button', or be underline so a user knows it's a link

Updated the button to use the Material text button.

david-allison

Looks great, cheers!

criticalAY

Looks good but just one question why not m3 button?

Arthur-Milchior

Hi,

First, nice to meet you. Thanks for working on this!

First feedback on your PR, if it's not too hard, when you change the behaviour, I'd love that you update the gif in the first message, so that I know what I expect to see.

You implemented what David requested, and I should have commented on the bug report sooner. I admit I'm not entirely convinced this is the best feature we may have for this problem.

I'd expect that, in case of download issue, we'd want to directly send the intent without requesting a user to tap on the button. And have a message stating that the user should must navigate to their download folders. Or otherwise that they can download it manually from this link, and have a button allowing the user to copy the link in the clipboard.
First, because I don't really know a case where the user would not want to go through the browser. And if they always want to go through the browser, let's do it for them. (Admittedly, if they don't yet have any preferred browser, that may be a not optimal experience. I don't know how many people will have opened ankidroid but not a browser. That must exists sometime. I'd expect this is mostly an issue when you are a developer testing on simulator)
More importantly, if it's relatively easy to solve the error through an alternate route, then we can just be silent about the error. I'd expect most users not to care that there was supposed to be a more efficient route that failed. And even if it usually work on their device and that's the first time it failed, it's still no big deal that the route is different for this time.

In the case when we keep with your current implementation, and requiring the user to do the click, I still believe that the button message should be changed

Arthur-Milchior · 2024-11-11T15:15:55Z

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


        val fileToBeDownloaded = arguments?.getSerializableCompat<DownloadFile>(DOWNLOAD_FILE)!!
        downloadManager = (activity as SharedDecksActivity).downloadManager

        downloadFile(fileToBeDownloaded)
-


Why do you remove this line? That's not wrong, but I admit I'd have not expected to see changes in part of the code you didn't touch.

Arthur-Milchior · 2024-11-11T15:28:05Z

AnkiDroid/src/main/res/values/02-strings.xml

@@ -268,6 +268,7 @@
    <string name="percentage">%s%%</string>

    <string name="download_deck">Download deck</string>
+    <string name="download_deck_from_AnkiWeb">Download deck from AnkiWeb</string>


Not fan of this phrasing to be honest. I can't imagine that the average user would know what ankiweb is. And even if they knew, how does it differ from what they did? After all, we were already on ankiweb website

disconnect821 · 2024-11-28T11:12:42Z

Sorry for the late response. Thank you for the feedback and suggestions!

1- I understand your suggestion to directly send the intent on download failure and provide a message guiding users to their download folder. However, if we send the intent every time a download fails, wouldn’t that make the 'Try Again' button redundant? The button currently allows users to retry manually, which could still be helpful. Should we consider removing or repurposing it?

2- Adding a button to copy the link to the clipboard feels similar to the manual download button, except it adds an extra step for users to paste the link themselves.

Could you please confirm if I’ve understood your approach correctly?

And since we’re redirecting to AnkiWeb in the browser, I agree that the button label should be updated. Instead of Download From AnkiWeb, how about using something like Open in Browser to make it clearer that the user will be redirected outside the WebView? Alternatively, we could use Download from Browser, but I think it might cause confusion, as the button is redirecting users to the browser rather than starting a download directly. Please let me know if that works for you.

Arthur-Milchior · 2024-12-11T02:55:54Z

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

@@ -105,7 +122,8 @@
        app:layout_constraintBottom_toBottomOf="parent"
        app:layout_constraintStart_toStartOf="parent"
        app:layout_constraintEnd_toEndOf="parent"
-        android:visibility="gone" />
+        android:visibility="gone"


why this change?

Arthur-Milchior · 2024-12-11T03:05:44Z

Sorry for the late response. Thank you for the feedback and suggestions!

No worries, we are all volunteers. Also, sorry for my delay.
Please, if you need my specific input, as I reviewed it, don't hesitate to ping me. Here or on discord, so that I can answer faster. I admit I had not seen the notification in my huge queue

1- I understand your suggestion to directly send the intent on download failure and provide a message guiding users to their download folder. However, if we send the intent every time a download fails, wouldn’t that make the 'Try Again' button redundant? The button currently allows users to retry manually, which could still be helpful. Should we consider removing or repurposing it?

Ideally, I'd say that, if the intent suceeded, we remove "try again", otherwise we keep it. I have no idea whether it's something we can actually now.
In doubt, I'd keep it. It does no harm, as far as I can tell. And in most case, it should not even have to be used.

To give a better answer, I'd need to know what failures we have. If we expect to have ankiweb failure, then yeah, "retry" is great. If we expect to have failure due to some version of android not accepting to read downloaded file, "retry" is useless. And I've no idea what are the problems we actually encounter. Even if I can guess that ankiweb is down some of the time

2- Adding a button to copy the link to the clipboard feels similar to the manual download button, except it adds an extra step for users to paste the link themselves.

Could you please confirm if I’ve understood your approach correctly?

I agree. I only suggested it in case the first solution failed. I don't know the entire Android API and could imagine we didn't do the first proposed suggestion because of some limitation I didn't know.

And since we’re redirecting to AnkiWeb in the browser, I agree that the button label should be updated. Instead of Download From AnkiWeb, how about using something like Open in Browser to make it clearer that the user will be redirected outside the WebView? Alternatively, we could use Download from Browser, but I think it might cause confusion, as the button is redirecting users to the browser rather than starting a download directly. Please let me know if that works for you.

"Open in Browser seems fine." if you open the html page that contains the download button.
If you open in the browser the page that contains the .apkg file then I prefer "download from browser".

Admittedly, it's not clear the ankiweb apkg link is expected to survive more than a few seconds. For example https://ankiweb.net/svc/shared/download-deck/597112404?t=eyJvc***pnI does not seems a very permanent url due to the t= argument. So I think it's better to open the html page in the browser. In which case "open in browser" sounds nice.

david-allison · 2024-12-11T03:28:54Z

This should also handle the case where the import fails due to something like:

fix(shared-decks): can't download decks on Android 15 #17574

Manual-Download-link

dd7ed40

github-actions bot added the New contributor label Nov 2, 2024

github-actions bot added the Strings label Nov 2, 2024

disconnect821 marked this pull request as draft November 2, 2024 17:34

comments-added

566af18

Comments-Updated

533b922

david-allison reviewed Nov 4, 2024

View reviewed changes

david-allison added the Needs Author Reply Waiting for a reply from the original author label Nov 4, 2024

Target-URL-fixed

29c694f

disconnect821 marked this pull request as ready for review November 9, 2024 05:45

david-allison requested changes Nov 10, 2024

View reviewed changes

Fixed-button-style

abf494e

david-allison approved these changes Nov 11, 2024

View reviewed changes

david-allison added Needs Second Approval Has one approval, one more approval to merge squash-merge The pull request currently requires maintainers to "Squash Merge" and removed Needs Author Reply Waiting for a reply from the original author labels Nov 11, 2024

criticalAY reviewed Nov 11, 2024

View reviewed changes

Arthur-Milchior reviewed Nov 11, 2024

View reviewed changes

mikehardy added this to the 2.20 Release milestone Nov 18, 2024

lukstbit mentioned this pull request Dec 3, 2024

Update fragment_shared_decks_download.xml #17527

Closed

5 tasks

mikehardy modified the milestones: 2.20 Release, 2.21 release Dec 4, 2024

mikehardy added the Needs reviewer reply Waiting for a reply from another reviewer label Dec 7, 2024

mikehardy requested a review from Arthur-Milchior December 7, 2024 18:10

Arthur-Milchior reviewed Dec 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Manual-Download-link #17351

Manual-Download-link #17351

disconnect821 commented Nov 2, 2024 •

edited

Loading

welcome bot commented Nov 2, 2024

github-actions bot commented Nov 2, 2024

disconnect821 commented Nov 2, 2024 •

edited

Loading

disconnect821 commented Nov 3, 2024

david-allison left a comment

david-allison Nov 4, 2024

disconnect821 Nov 5, 2024

david-allison left a comment

david-allison Nov 10, 2024

disconnect821 Nov 11, 2024

david-allison left a comment

criticalAY left a comment

Arthur-Milchior left a comment

Arthur-Milchior Nov 11, 2024

Arthur-Milchior Nov 11, 2024

disconnect821 commented Nov 28, 2024

Arthur-Milchior Dec 11, 2024

Arthur-Milchior commented Dec 11, 2024 •

edited by david-allison

Loading

david-allison commented Dec 11, 2024

Manual-Download-link #17351

Are you sure you want to change the base?

Manual-Download-link #17351

Conversation

disconnect821 commented Nov 2, 2024 • edited Loading

Purpose / Description

Fixes

Approach

Steps to reproduce the download failed error.

Checklist

welcome bot commented Nov 2, 2024

github-actions bot commented Nov 2, 2024

disconnect821 commented Nov 2, 2024 • edited Loading

Task Remaining

disconnect821 commented Nov 3, 2024

david-allison left a comment

Choose a reason for hiding this comment

david-allison Nov 4, 2024

Choose a reason for hiding this comment

disconnect821 Nov 5, 2024

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

david-allison Nov 10, 2024

Choose a reason for hiding this comment

disconnect821 Nov 11, 2024

Choose a reason for hiding this comment

david-allison left a comment

Choose a reason for hiding this comment

criticalAY left a comment

Choose a reason for hiding this comment

Arthur-Milchior left a comment

Choose a reason for hiding this comment

Arthur-Milchior Nov 11, 2024

Choose a reason for hiding this comment

Arthur-Milchior Nov 11, 2024

Choose a reason for hiding this comment

disconnect821 commented Nov 28, 2024

Arthur-Milchior Dec 11, 2024

Choose a reason for hiding this comment

Arthur-Milchior commented Dec 11, 2024 • edited by david-allison Loading

david-allison commented Dec 11, 2024

disconnect821 commented Nov 2, 2024 •

edited

Loading

disconnect821 commented Nov 2, 2024 •

edited

Loading

Arthur-Milchior commented Dec 11, 2024 •

edited by david-allison

Loading