Clean up all deprecated and unchecked methods - #317
Conversation
dektar
left a comment
dektar
left a comment
There was a problem hiding this comment.
It's really important that this doesn't cause regressions or behavior changes, so my comments are all around verifying the state is still OK. Thanks.
| refreshButton.getCompoundDrawablesRelative()[0].mutate().setColorFilter( | ||
| ContextCompat.getColor(itemView.getContext(), R.color.colorAccent), | ||
| PorterDuff.Mode.MULTIPLY); | ||
| new PorterDuffColorFilter(ContextCompat.getColor(itemView.getContext(), R.color.colorAccent), |
There was a problem hiding this comment.
can you verify this looks the same as before?
There was a problem hiding this comment.
Here's a screenshot from the emulator of the main page, shows the refresh, search and location buttons.
|
|
||
| LocationManager locationManager = (LocationManager) getSystemService(LOCATION_SERVICE); | ||
| String provider = locationManager.getBestProvider(new Criteria(), false); | ||
| String provider = LocationManager.NETWORK_PROVIDER; |
There was a problem hiding this comment.
can you make sure this works the same way as it did?
| builder.setChannelId(CHANNEL_ID); // O and above require Notification Channels. | ||
| } | ||
| builder.setColor(ContextCompat.getColor(context, R.color.colorPrimary)); | ||
| .setSmallIcon(R.drawable.app_icon_bw) |
There was a problem hiding this comment.
please confirm the notification still works as it did previously. notifications have been a headache!
There was a problem hiding this comment.
You've worked on this code before, could you describe your testing process please?
There was a problem hiding this comment.
I basically set up a reoccurring notification (reminder) in settings to happen 1-2 minutes from now, minimize/close the app, wait for it to come in, and then look at it. There's also a FREQUENT_NOTIFICATION_DEBUG_MODE you can set in NotificationUtils.java that should make them happen more often -- although it's been a while since I tried that.
There was a problem hiding this comment.
This doesn't look quite right. The stars should be bigger in the circle. Here's the screenshot from my device, live app:
There was a problem hiding this comment.
I suspect it's just the resolution of my emulator, here's what it looks like for the same emulator built from the main branch
There was a problem hiding this comment.
why are you deleting icons?
There was a problem hiding this comment.
Lint reported none of these are used, since the minimum sdk is 23.
Seems like in a story about deprecated methods, removing deprecated/unused resources would be included?
| @@ -1,4 +1,4 @@ | |||
| <resources xmlns:xliff="http://schemas.android.com/tools"> | |||
| <resources xmlns:xliff="urn:oasis:names:tc:xliff:document:1.2" xmlns:tools="http://schemas.android.com/tools"> | |||
There was a problem hiding this comment.
can you explain why you are adding urn:oasis... here?
There was a problem hiding this comment.
will remove here, now in the gradle upgrade PR
There was a problem hiding this comment.
no, git correctly marks this as outdated
| testOptions { | ||
| unitTests { | ||
| includeAndroidResources = true | ||
| returnDefaultValues = true |
There was a problem hiding this comment.
this doesn't seem related to the change description. should it be done in a separate change?
There was a problem hiding this comment.
no, git correctly marks this as outdated
|
@dektar hey just FYI as of Thursday Sept 10 I'll be traveling for a month and won't have my laptop, so won't be responding to comments. If you want to work on any of my PRs in order to get them merged in the next month, feel free to do so. I'd be interested in continuing to contribute to this project when I'm back in October, I'll pay more attention to issues that are marked priority, if you want to review the list of issues and add/remove such labels. Thanks. ...bjc |
2 participants
What type of PR is this? (check all applicable)
Description
Got rid of all the deprecated and unchecked errors.
See the attached file that showed what the build originally reported, now it runs clean
Related Issues
Were the changes tested?
have not been included
5Calls_deprecated.txt