Skip to content
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

feat(fcm): Added Notification Count parameter to `AndroidNotification` class #309

Merged
merged 10 commits into from Nov 12, 2019

Conversation

@knocknarea
Copy link
Contributor

knocknarea commented Sep 9, 2019

Discussion

As discussed in issue #308

Testing

  • Added Test for setNotificationCount. I noticed that your JsonParser implementation prefers BigDecimal and it would appear that Integer, Long, etc. are deserialized into BigDecimal.

API Changes

  • None
* @param notificationCount the badge count null=unaffected, zero=reset, positive=count
* @return This builder
*/
public Builder setNotificationCount(Integer notificationCount) {

This comment has been minimized.

Copy link
@hiranya911

hiranya911 Sep 9, 2019

Member

This should take an int

This comment has been minimized.

Copy link
@knocknarea

knocknarea Sep 9, 2019

Author Contributor

Not really, the spec allows for null/empty in which case the badge count is unaffected.

This comment has been minimized.

Copy link
@knocknarea

knocknarea Sep 9, 2019

Author Contributor

But I see what you mean, however you will have to break the fluidity of the builder to determine if you should set this if it was an int. Whereas if it's an Integer you can use a variable set to null or badge count before calling the builder.

Integer count = determineCount();

AndroidNotification.builder().setNotificationCount(count).build();

Vs

Integer count = ...

Builder builder = AndroidNotification.builder();

if(count != null) {
   builder.setNotificationCount(count);
}

This comment has been minimized.

Copy link
@hiranya911

hiranya911 Sep 9, 2019

Member

The public API should accept an int. The internal representation could be Integer with a default value of null.

This comment has been minimized.

Copy link
@knocknarea

knocknarea Sep 9, 2019

Author Contributor

But null is part of the public spec, int breaks the builder pattern.

This comment has been minimized.

Copy link
@knocknarea

knocknarea Sep 9, 2019

Author Contributor

Ok, is it not the case that if a property annotated with @@key is null, it is not included in the resultant json? Thus fulfilling the requirement of unspecified?

By using a primitive type in the setter, you have no way of indicating this other than not calling the setter.

And so, as a user of the builder, you would then have no way to use the builder fluently if you had to check if the value of notification count was null.

Mindful of the fact that deriving that value might be complex and that the point of calling the builder may have been supplied this value from a higher level of code. Instead of just handing the value to the builder you would have to create a whole branch in code just switching on that value.

This comment has been minimized.

Copy link
@hiranya911

hiranya911 Sep 9, 2019

Member

Ok, is it not the case that if a property annotated with @@key is null, it is not included in the resultant json? Thus fulfilling the requirement of unspecified?

Right. Set the default value to be null, and you get the right semantics. See how Aps.badge is implemented for an example.

Mindful of the fact that deriving that value might be complex and that the point of calling the builder may have been supplied this value from a higher level of code

The higher-level code should never return null either. It should return 0. If it must return null, it's fairly easy to handle it as:

Integer count = determineCount();
builder.setNotificationCount(count != null ? count : 0); 

This comment has been minimized.

Copy link
@knocknarea

knocknarea Sep 10, 2019

Author Contributor

It may be that the higher level code determines that the badge count on the client device is left unchanged by this push notification. The only way to Express this is to return a count == null.

If the fire base builder can handle null properly and convey this by setting notification count to null (such that it is not included in the json). Then what is the problem here?

Null is a valid value for the attribute that says 'leave the count as it is on the client device'

It seems to me that Optional Integer is a good candidate for differentiation, but primitive int is not sufficient to express all values possible without breaking the fluency of the builder.

I can set it to primitive int, no problem, it just does not feel right in this case

This comment has been minimized.

Copy link
@hiranya911

hiranya911 Sep 10, 2019

Member

Null is a valid value for the attribute that says 'leave the count as it is on the client device'

Null is not a valid value for a field declared as number in the backend. Also in this case the backend allows both 0 and undefined to mean "keep as is". So it's easy to deal with any null-returning user code by mapping null to 0.

Furthermore JSON null and JSON undefined usually have different semantics:

{
  "foo": "test",
  "bar": null // bar is null
}

{
  "foo": "test",
  // bar is undefined
}

When one calls an API as setBar(null), it's typical to expect the value to get serialized as null.

It seems to me that Optional Integer is a good candidate for differentiation

Optional is a Java8 API and we are still on Java7. Plus, that's a somewhat overkill solution for such a simple problem. We already have an established pattern in the SDK to deal with type of fields, and we should be using it here to be consistent.

This comment has been minimized.

Copy link
@knocknarea

knocknarea Sep 10, 2019

Author Contributor

@hiranya911

Ok, I think this comes down to a misunderstanding on my part.

My assumption (which is obviously wrong) is that if an attribute annotated with @key is not initialized or set to a specific value. Then its default value in java is null. The assumption here is that your json serializer does not actually add the attribute to the resultant json,

...

@Key("my_var")
private String myVar;

...

i.e. If the value is null, then instead of this:

{ 
  ...
   "my_var": null,
  ...
}

You'd get this:
{
...
...
}

Copy link
Member

hiranya911 left a comment

Thanks @knocknarea. Mostly looks good. Just a few nits on API docs and the test case.

@knocknarea

This comment has been minimized.

Copy link
Contributor Author

knocknarea commented Sep 12, 2019

@hiranya911 Is this ready now?

Copy link
Member

hiranya911 left a comment

Thanks @knocknarea. LGTM 👍

@hiranya911 hiranya911 changed the title #308 Added Notification Count to AndroidNotification feat(fcm): Added Notification Count parameter to `AndroidNotification` class Sep 12, 2019
chong-shao added 5 commits Oct 29, 2019
@chong-shao

This comment has been minimized.

Copy link
Contributor

chong-shao commented Oct 29, 2019

LGTM!
I did some commits from the website trying to fix the merge issues.

@chong-shao chong-shao merged commit e918da8 into firebase:master Nov 12, 2019
3 checks passed
3 checks passed
Trigger: 3413a0d4-6450-4b83-aeb3-ee8e3eeb9d70 Summary
Details
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.