-
Notifications
You must be signed in to change notification settings - Fork 50
Notification enhancement #31
Notification enhancement #31
Conversation
Oh btw. Today is not my birthday. |
.setContentIntent(intent) | ||
.setNumber(events.getContactCount()) |
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.
This is done right before displaying the notification (https://github.com/alexstyl/Memento-Namedays/pull/31/files#diff-a57d0b3fb13451a13012ab3380f979cfR155) so it is going to be overridden eventually.
Can you remove the later one and keep yours as part of this PR please?
This looks great! I have added some comments in the PR that needs fixing. Don't worry about the testing. The class was written a couple of years ago when I didn't really know how to structure my code properly for testing, so it definitely needs work on my end 💃 |
@@ -19,6 +19,7 @@ | |||
<string name="today_celebrates_one">%1$s</string> | |||
<string name="today_celebrates_two">%1$s and %2$s</string> | |||
<string name="today_celebrates_many">%1$s and %2$d others</string> | |||
<string name="age_today">%1$s %2$d today</string> |
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 app uses a "Turns X" (see string "turns_age"
) when communicating birthdays with age to the user.
Instead of using a new string, I would prefer if you use the existing one instead and add the name of the contact programmatically.
|
||
String lineFormatted = name; | ||
|
||
if (EventType.BIRTHDAY.equals(event.getType())) { |
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.
❌ This is hard to extend. What happens when more events are added in the future? We would have to check all bits of code that do checks with specific events.
Also, a Birthday might not include a year of birth, which means that the following bit will throw an exception if the contact does not have a specified year of birth.
You could just copy-paste the getLabelFor()
method. It checks only for that special case, and fallbacks to using the label of the eventType if there is no special case.
Don't worry about the code duplication from the other class. I'll solve it nicely in a later PR :)
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.
Copy Paste! Oh no!
...
Well, ok then. I'm not yet familiar enough with your code so I'm going to leave such a refactoring up to you. Let me add a big TODO and deprecated
so that it can be found later.
I'm going to create an issue as well so that it is not forgotten
Just one more comment and we are good to go 💯 Also, please update the PR description with the updated screenshot |
@@ -114,8 +167,6 @@ public void forDailyReminder(ContactEvents events) { | |||
builder.setPublicVersion(publicNotification.build()); | |||
} | |||
|
|||
builder.setNumber(contactCount); |
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 know. But the information is there when you click the |
Event labels are centralized this way. Fallback will make sure new events will be handled automatically + check whether birthday includes a year to avoid NPE's This method is copy-pased from ContactEventView and must be refactored.
Good point, but not everyone is going through the history through Github ;) An alternative is to have one commit per change so that you can actually checkout the specific change at any time. |
@@ -188,6 +179,27 @@ public void forDailyReminder(ContactEvents events) { | |||
|
|||
} | |||
|
|||
/** | |||
* TODO duplicated from {@link com.alexstyl.specialdates.upcoming.ui.ContactEventView#getLabelFor(ContactEvent)} |
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.
👍
Yeah indeed. In this case, that would have been a better idea. I have to get used to checking in very small changes though. I'm used to SVN and try to avoid too many small commits. But Git works better that way I guess... |
Thanks for the contribution. It is going to be included in the next update of the app 💯 |
Description
Enhanced the notifications for birthdays:
Test(s) added
no. Frankly I don't know how I could test this 😄
Screenshots
In case of only one birthday
In case of multiple birthdays:
Collapsed
Expanded