Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

AndroidX migration. #12

Open
wants to merge 2 commits into
base: better-deleting
Choose a base branch
from

Conversation

kdomingo
Copy link

No description provided.

Copy link
Owner

@milosmns milosmns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, thank you for contributing to this repository! TBH, I thought it doesn't work anymore 😄

I left a couple of comments/questions, but there's a big one coming...

Would you be able to target the latest SDK level (and verify the app works)? Assuming that I can find the signing key for this project, Google Play will not allow us to publish an update unless we target the latest SDK.

@@ -241,12 +247,36 @@ public boolean deleteNextContact() {
return true;
}

private String generatePhoneNumber() {
Random rndGenerator = new Random();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a rndPhoneTypeGenerator which can be a generic random generator. There's no security concern with this app, I believe we can just reuse it 🙂

private void close(Closeable closeable) {
if (closeable != null) {
try {
closeable.close();
} catch (Exception e) {
Log.w(TAG, "Cannot close the closeable " + String.valueOf(closeable));
Log.w(TAG, "Cannot close the closeable " + closeable);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had String.valueOf() calls to explicitly convert null values into "null" (string value of 4 chars).
Why do you think it's not necessary anymore?

@@ -108,7 +111,7 @@ public void onTaskRemoved(Intent rootIntent) {
}

private void showNotification() {
mBuilder = new NotificationCompat.Builder(this);
mBuilder = new NotificationCompat.Builder(this, CONTACTS_GENERATOR_CHANNEL);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants