-
-
Notifications
You must be signed in to change notification settings - Fork 153
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
issue-1054 handle all deprecation warnings #1072
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 for taking the time to do this, the build logs look so clean now! I also very much appreciate how you moved the old compatibility code into their own functions to silence the deprecation warnings as this helps prevent from silencing too much and will make clean-up extra easy the next time support for an old Android release gets dropped.
I haven't gotten to testing all this yet, but reviewing the code got me 2 small notes.
Note for myself, things I noticed that will require extra testing to make sure no new bugs have snuck in:
- API 23 and above need status bar UI checks (checking MainActivity and ViewActivity should be sufficient)
- View activity screen stays unlocked needs testing specifically on API 27 and above
- Fullscreen mode on LoyaltyCardViewActivity on API 30 and above needs testing
|
||
progress.show(); | ||
dialog = builder.create(); | ||
dialog.show(); |
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.
It looks like you're fixing the ProgressDialog being deprecated here by recreating the ProgressDialog in a MaterialAlertDialog?
While that does remove the deprecation warning, it doesn't solve the real issue: importing is done on the main thread, locking up the UI and breaking if the user navigates away and Android stops the activity.
Because this doesn't fix the real issue and just adds a lot of extra code, I'd rather you just leave the ProgressDialog here and cause the in this case very correct deprecation warning to remain shown. #513 exists to track this specific issue.
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.
Reverted changes
Thank you for your feedback :) |
Sorry for the delay, Hacktoberfest has been quite successful for Catima this year and I've been having trouble keeping up with all the PRs. This looks great now yeah :) |
Hello!
This closes #1054.
I tested pretty much every change I made and it seems that it works as it used to. Well, except for ProgressDialog for Import/Export, it looks a bit different now, but it feels totally fitting with application style, so I think it's an ok replacement.
P.S. I'm a participant of Hacktoberfest 2022, so I would really appreciate if you'd mark this PR with label "hacktoberfest-accepted". Thank you!
Have a good day :)