-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Feat (Progress Dialog): Compat to add new progress bar #19970
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?
Conversation
9ac8ea1 to
f94dfb1
Compare
| } | ||
| } | ||
|
|
||
| fun setMax(max: Int) = runOnUi { setMaxInternal(max) } |
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.
unused variable
| circularIndicator?.max = max | ||
| } | ||
|
|
||
| fun setProgress(value: Int) = runOnUi { setProgressInternal(value) } |
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.
unused variable
| } | ||
| } | ||
|
|
||
| fun incrementProgressBy(diff: Int) = runOnUi { setProgressInternal(progress + diff) } |
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.
unused variable
|
|
||
| fun incrementProgressBy(diff: Int) = runOnUi { setProgressInternal(progress + diff) } | ||
|
|
||
| var isIndeterminate: Boolean |
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.
unused variable
| } | ||
| } | ||
|
|
||
| private fun dismissDialogIfShowing(dialog: Dialog) { |
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.
unused function
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.
The reason there's so many unused functions and variables is because I wanted it to be very similar to progressdialog since it is a compat wrapper. I thought this was best practice but if it's better to remove all the unused parts, I can
f94dfb1 to
5900d22
Compare
|
I implemented the changes which David had mentioned starting here: I'm not sure if I understood it in the right way, so if I'm wrong do let me know |
I'd consider an option in |
I did do some more comprehensive testing when I force pushed and it was working well, is there anything to change ui/code wise? |
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.
I've read the code and description a few times, and I don't understand the intention of this PR well enough.
It would help greatly if there was developer-level functionality to test this UI, as I don't have a 'reasonably slow' operation to try it with.
It doesn't fix the linked issue: optimizing parameters and checking database does not use a 'progress dialog with progress'
The Android deprecation on ProgressBar was introduced in order for developers to move away from a blocking progress bar in most cases. Instead, on-screen progress bars (such as the one we use in the CardBrowser) should be used. Removing the deprecation, whilst keeping the UX paradigm isn't how we should be tackling this.
We do have some operations which are 'truly' blocking: the pre-media sync being the main one.
ProgressDialogs have been in AnkiDroid for a long time, and they are a core concern of our code.
The default for 'wait for an operation' (normally a blocking operation) which anyone reaches for currently is withProgress { }. Android considers this to be bad UX
So:
- This new implementation should also be marked as
@Deprecatedby us (and suppressed appropriately)- Your code exists to add better progress notifications to the
ProgressDialog, and this is useful
- Your code exists to add better progress notifications to the
- We should use Material 3; Expressive if possble
- We should focus on the listed screens in the issue, and see if there's a better abstraction for 'progress bar with linear progress'
- Maybe it looks something like this: #18341
- Ideally we should have a developer option to test this UI (maybe combine with the option I added for
tts-voices)
| @Suppress("Deprecation") // ProgressDialog deprecation | ||
| private fun ProgressContext.updateDialog(dialog: android.app.ProgressDialog) { | ||
| private fun ProgressContext.updateDialog(dialog: ProgressCompat) { | ||
| // ideally this would show a progress bar, but MaterialDialog does not support |
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.
Was this not fixed?
|
Honestly, I started with the intention of fixing the linked issue of course, which was to change the progress dialogs in the check database and full sync dialogs and so the idea I had was to just find a way to make one class that allows every progress dialog with a fixed x/y to be changed into a circular progress bar. This was when I took the route of making a compat wrapper for the progressdialogs, and it's on me for not actually marking that as deprecated but the idea is to use that for now to update the UI, and then move away from it as you said since its blocking.
Based on the above, I think my plan for the pr will be to:
As for the listed screens, the issue I'm having is that for database check, I can't even test it since it finishes instantly and for full sync, since there's no end value, I can't make it a progress bar. I think these for these indeterminate screens, I wouldn't want to tackle them in this pr and possibly leave it for someone else or come back to it later after more discussion. I hope I understood everything and this isn't rambling, is it fine for me to go ahead with the plan I gave above? |
|
That's great. If you state that it fixes 'part of' an issue, then you've moved thing forward, most times in the right direction to solve the bigger picture. It's even better when there's a task list of items which aren't yet fixed, so someone can come in and know where to My current issue when reviewing is that I can't reliably test the dialog, as soon as we all have a 'good' measure for how to see it, I have a lot more confidence in the change. We can then simulate unusual cases such as 'optimize all presets', which effectively have a 'dual layer' progress bar:
|
I'll see if I can get a developer option in place locally first that can help test this so that we can see how the change is reflecting, because even if I find it hard to see it except on a few dialogs, if that works should I commit to this pr with a new commit or open another issue and open it there? |
Purpose / Description
This changes the UI of existing progress dialogs (deprecated in api 26) to newer material ui progress circles.
Fixes
Approach
Created a new compat class ProgressCompat.kt to easily change all the existing progressdialogs to the new ones. We use this class in CoroutineHelpers.kt to change all the withProgress dialogs. The compat class uses regex to check for values like x/y and updates the dialog with a progress circle (defined in a new xml) automatically, if not we use an indeterminate spinner.
How Has This Been Tested?
Screen_recording_20251229_200048.webm
I struggled to test this normally, using one-way sync was the best way I found. If anyone has larger decks, or a better place to test, please let me know.
Learning (optional, can help others)
Referred:
https://developer.android.com/reference/android/app/ProgressDialog
ActionProviderCompat.kt for documentation help
https://m2.material.io/components/progress-indicators/android#circular-progress-indicators : for design (not sure if we want to use m3 since I believe it was still under discussion in the discord)
Checklist
Please, go through these checks before submitting the PR.