-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add help icon to the sync error dialog #20147
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
Conversation
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!!!
There's lots of changes which don't seem relevant to the PR (maxLines), and lots of line noise
I would expect in the layout:
- Add the icon to the left of the screen
- add a constraint on the icon
- add 'end' padding on the icon
| }.setNegativeButton(R.string.sync_conflict_keep_remote_new) { _, _ -> | ||
| requireSyncErrorDialogListener().showSyncErrorDialog(DIALOG_SYNC_CONFLICT_CONFIRM_KEEP_REMOTE) | ||
| }.setNeutralButton(R.string.dialog_cancel) { _, _ -> | ||
| AlertDialog.Builder(requireContext()).create { |
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.
In this case, continue to use dialog: have titleWithHelpIcon return this
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.
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.
Here's a before and after with my changes:
Index: AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml b/AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml
--- a/AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml (revision 0c3c88c1659c1eec012fcc20f8e498497eed03e0)
+++ b/AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml (revision 6a54c83ee785b0a6062742c669274b953f80c9d4)
@@ -14,7 +14,6 @@
android:id="@+id/title_icon"
android:layout_width="24dp"
android:layout_height="24dp"
- android:layout_marginEnd="12dp"
android:visibility="gone"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="@android:id/title"
@@ -29,6 +28,8 @@
android:layout_height="wrap_content"
android:textAppearance="?attr/textAppearanceHeadlineSmall"
android:textColor="?attr/colorOnSurface"
+ android:layout_marginStart="12dp"
+ app:layout_goneMarginStart="0dp"
app:layout_constraintEnd_toStartOf="@+id/help_icon"
app:layout_constraintStart_toEndOf="@+id/title_icon"
app:layout_constraintTop_toTopOf="parent"
This is almost there.
- Check to see what's going on with the title, I may have made a mistake with the size, or it may be dynamic based on content to fit a line
- Maintain the 'old' color, we'll handle these in:
- The margin is wrong, there should be more space between the icon and the title
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.
Not sure if you're still working on this, or the push was a nudge for me to re-review.
Still pending on an investigation of the title
8908f5a to
0c3c88c
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.
Thanks!!
AnkiDroid/src/main/java/com/ichi2/anki/dialogs/SyncErrorDialog.kt
Outdated
Show resolved
Hide resolved
| }.setNegativeButton(R.string.sync_conflict_keep_remote_new) { _, _ -> | ||
| requireSyncErrorDialogListener().showSyncErrorDialog(DIALOG_SYNC_CONFLICT_CONFIRM_KEEP_REMOTE) | ||
| }.setNeutralButton(R.string.dialog_cancel) { _, _ -> | ||
| AlertDialog.Builder(requireContext()).create { |
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.
Here's a before and after with my changes:
Index: AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml b/AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml
--- a/AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml (revision 0c3c88c1659c1eec012fcc20f8e498497eed03e0)
+++ b/AnkiDroid/src/main/res/layout/alert_dialog_title_with_help.xml (revision 6a54c83ee785b0a6062742c669274b953f80c9d4)
@@ -14,7 +14,6 @@
android:id="@+id/title_icon"
android:layout_width="24dp"
android:layout_height="24dp"
- android:layout_marginEnd="12dp"
android:visibility="gone"
app:layout_constraintStart_toStartOf="parent"
app:layout_constraintTop_toTopOf="@android:id/title"
@@ -29,6 +28,8 @@
android:layout_height="wrap_content"
android:textAppearance="?attr/textAppearanceHeadlineSmall"
android:textColor="?attr/colorOnSurface"
+ android:layout_marginStart="12dp"
+ app:layout_goneMarginStart="0dp"
app:layout_constraintEnd_toStartOf="@+id/help_icon"
app:layout_constraintStart_toEndOf="@+id/title_icon"
app:layout_constraintTop_toTopOf="parent"
This is almost there.
- Check to see what's going on with the title, I may have made a mistake with the size, or it may be dynamic based on content to fit a line
- Maintain the 'old' color, we'll handle these in:
- The margin is wrong, there should be more space between the icon and the title
0c3c88c to
65a3f53
Compare
b52744c to
6aa12b6
Compare
|
The title height has changed from the original dialog (also in my screenshot)
|
6aa12b6 to
0c8c74e
Compare
|
@Fandroid745 is this pending a review, the title issue still looks to be pending |
0c8c74e to
bf6bfa3
Compare
|
Check what happened before/after this commit (b0e6996). That will clarify:
It would also be useful to check with the material dialog specs: https://m3.material.io/components/dialogs/specs |
I checked the previous commit d33b340 and it uses onMeasure which results in the text being autosized and it allows up to two lines. The 26dp is not a bug as it is the height of the title for single line and the height of the top panel is 60dp. |
|
But the behaviour appears incorrect now we're extending it with this new dialog? Let's reuse the material dialog logic to provide a standardized experience between the two title bars? |
Yes, I will update the implementation to reuse the material dialog logic. |
bf6bfa3 to
f644067
Compare
|
I am sorry for the delay, I tried a lot of random changes hoping that I will stumble upon the correct layout for the dialog. I am closing the pr for now, I will reopen it once I am certain the dialog matches the original. |










Purpose / Description
Add help icon to the sync dialog in order to avoid confusion for users.
Fixes
This fixes #17544
Approach
1.SyncErrorDialog.kt :Refactored the DIALOG_SYNC_CONFLICT_RESOLUTION block to use the titleWithHelpIcon extension from AlertDialogFacade.kt
2.AlertDialogFacade.kt: Updated the extension function to use optional icon parameter.
3.alert_dialog_with_xml :Added the title_icon ImageView to alert_dialog_title_with_help.xml and added adjustment so that the title aligns when no icon is present.
4.Constants.xml: added the link_sync_conflict_help string pointing to the url: https://docs.ankiweb.net/syncing.html#conflicts
How Has This Been Tested?
Ran the app on the phone
and verified that the sync dialog shows both the dialog icon on the left and the help_icon on the right.
Verified that the icons are visible on both light and dark theme.
Checklist
Please, go through these checks before submitting the PR.
(https://play.google.com/store/apps/details?id=com.google.android.apps.accessibility.auditor)
screen-20260118-164024.mp4