-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
"Delete your account" next step is not very clear (and not translated) fixed #19436 #19540
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
base: main
Are you sure you want to change the base?
"Delete your account" next step is not very clear (and not translated) fixed #19436 #19540
Conversation
|
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. |
|
Important Maintainers: This PR contains Strings changes
|
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!!
Please fill out the PR template: https://github.com/ankidroid/Anki-Android/blob/main/.github/pull_request_template.md
AnkiDroid/src/main/java/com/ichi2/anki/account/AccountRemovalExplanationDialog.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/account/AccountRemovalExplanationDialog.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/account/AccountRemovalExplanationDialog.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/account/AccountRemovalExplanationDialog.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/account/AccountRemovalExplanationDialog.kt
Outdated
Show resolved
Hide resolved
- Use View Binding for dialog - Switch to FixedTextView in layout - Use android.R.string for standard buttons - Use Context.copyToClipboard() utility - Apply sentence case to dialog title - Simplify dialog message wording - Use AlertDialogFacade.create extension - Move fragment result listener to onCreate() - Remove clearFragmentResultListener from callback - Add listener cleanup in onDestroy()
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heya, could you fill out all the PR Template:
https://github.com/ankidroid/Anki-Android/blob/main/.github/pull_request_template.md
AnkiDroid/src/main/java/com/ichi2/anki/account/LoggedInFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/account/LoggedInFragment.kt
Outdated
Show resolved
Hide resolved
AnkiDroid/src/main/java/com/ichi2/anki/pages/RemoveAccountFragment.kt
Outdated
Show resolved
Hide resolved
|
|
||
| override fun onCreateDialog(savedInstanceState: Bundle?): Dialog { | ||
| val preferences = PreferenceManager.getDefaultSharedPreferences(requireContext()) | ||
| val email = preferences.getString("username", "") ?: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefs.username
AnkiDroid/src/main/java/com/ichi2/anki/account/AccountRemovalExplanationDialog.kt
Outdated
Show resolved
Hide resolved
|
Hi @david-allison,
|
|
Leave a patch on the PR for testing, and remove the method |
Done! I've added a minimal OnWebViewRecreatedListener implementation as a temporary patch to allow testing. It's clearly marked as temporary and references #19561 for the proper fix. |
I've updated the PR description to follow the template. Please let me know if there's anything else I should add or clarify in the description.
I'll push the updated code shortly. |
- Use viewLifecycleOwner for fragment result listener - Remove onDestroy cleanup (handled automatically) - Keep minimal OnWebViewRecreatedListener for testing (per reviewer request) - Use Prefs.username instead of PreferenceManager - Hide email/copy button if username is empty - Remove unnecessary comments
|
Well be merged. Rebase this PR on |
|
Please rebase onto
|
946ee94 to
19036f9
Compare
david-allison
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, cheers. One more round I think
Index: AnkiDroid/src/main/res/layout/dialog_account_removal_explanation.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/res/layout/dialog_account_removal_explanation.xml b/AnkiDroid/src/main/res/layout/dialog_account_removal_explanation.xml
--- a/AnkiDroid/src/main/res/layout/dialog_account_removal_explanation.xml (revision c27970afc599ec0f98ff2c39dafc55d5db6bb253)
+++ b/AnkiDroid/src/main/res/layout/dialog_account_removal_explanation.xml (date 1764299227454)
@@ -1,5 +1,6 @@
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
+ xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:orientation="vertical"
@@ -38,8 +39,9 @@
android:layout_weight="1"
android:textAppearance="?attr/textAppearanceBody1"
android:textColor="?attr/colorOnSurface"
- android:ellipsize="end"
- android:maxLines="1" />
+ android:ellipsize="middle"
+ android:maxLines="1"
+ tools:text="unusual_very_long_email@example.com"/>
<Button
android:id="@+id/copy_email_button"using android:ellipsize="middle leads to a better email experience:
| setPositiveButton(R.string.dialog_ok) { _, _ -> | ||
| setFragmentResult(REQUEST_KEY, bundleOf(RESULT_PROCEED to true)) | ||
| } | ||
| setNegativeButton(R.string.dialog_cancel) { _, _ -> } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: positiveButton and negativeButton are a little less verbose, as you can skip the lambda parameters
| /** | ||
| * Redirect from post-login pages (such as 'verify account') to the required page | ||
| */ | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: revert - the overall 'diff' of your changes shouldn't include whitespace changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
|
@Arthur-Milchior Is this what you intended? |
- Use ellipsize middle for better email display - Add tools:text for layout preview - Simplify button lambdas using positiveButton/negativeButton - Remove accidental whitespace changes
|
Hello 👋, this PR has had no activity for more than 2 weeks and needs a reply from the author. If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing! You have 7 days until this gets closed automatically |
|
First, nice to meet you @infiniteflux ! Great work! Thank you David for pinging me on discord. I've opinions indeed. I'm not fan of "Remove account". It's not clear from what it'll be removed. I guess we should keep it as it's what ankiweb uses. I like the way you implemented the alert. I'd suggest a different phrasing however.
My reasoning is that it's nice for the non-English speaking users to be able to know what the warning says. And what is expected of them (type the word "remove"). And we can use our translators for that. Maybe it'd be better to have distinct sentences for English and non-English, and check in code whether the locale is English to decide which sentence to use. Because some of those sentences are only useful for non english-speaking users. If it's not too hard, I'd appreciate that too. Admittedly, if ankiweb UI changes, we'll need to update our strings. I expect it's rare enough that it's not a big maintenance burden. Admittedly, an alternative solution could be to just open this in an external browser. I expect most brother to have a translate feature, which may simplify everything. Still, I prefer the solution I suggest above, because I don't expect users to necessarily know about the translate feature of their browser. Can you please let me know how this behave when the account is on a personal server? I've never tried ankidroid with a non ankiweb server. It's not a big deal if this rare use case is not perfect, but it'd be nice to know what occurs. |
|
Hi @Arthur-Milchior, nice to meet you too—thanks so much for the thoughtful review! Screenshot This is what I've implemented based on your earlier suggestions. Does it look good, or any changes needed? Happy to tweak! |
|
@Arthur-Milchior FYI: awaiting a response |

Purpose / Description
Add a confirmation dialog before redirecting users to the AnkiWeb account removal page. The current flow immediately opens a WebView, which is confusing for users as they don't understand what will happen next and the page is not translated.
This PR:
Fixes
Approach
How does this change address the problem?
2.Modified LoggedInFragment:
3.User Flow:
This gives users context and prevents accidental account removal attempts.
How Has This Been Tested?
Test Environment:
Test Cases:
1.Happy Path - User proceeds with removal:
2.User cancels:
3.Configuration changes:
4.Empty email handling:
5.Back button:
Learning
Key patterns used:
Resources:
Checklist