Skip to content

Allow the GcmBroadcastReceiver to be subclassed #638

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

Merged
merged 6 commits into from
Apr 30, 2017

Conversation

mmimeault
Copy link
Contributor

You can only define one Broadcast Receiver to act as a GCM receiver.
In our case, we want to receive pushes from Parse and another provider. So I was going to write a Proxy that wrap both receivers.

But the Parse SDK prevent that by doing a strong validation of the Manifest. (https://github.com/parse-community/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ManifestInfo.java#L446-L448)
The GCM receiver defined need to be explicitly an instance of GcmBroadcastReceiver.
And that class can't be extended as the onReceive method is final.
(If the validations do not pass, the PushType is None and the registration will not happen) (https://github.com/parse-community/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ManifestInfo.java#L212-L214)

The current PR is a proposition.
But there are many solutions to my problem.

  • Remove the final
  • Remove the strong validation
  • other suggestion

@codecov
Copy link

codecov bot commented Apr 18, 2017

Codecov Report

Merging #638 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #638   +/-   ##
=========================================
  Coverage     52.86%   52.86%           
  Complexity     1676     1676           
=========================================
  Files           131      131           
  Lines         10146    10146           
  Branches       1408     1408           
=========================================
  Hits           5364     5364           
  Misses         4339     4339           
  Partials        443      443
Impacted Files Coverage Δ Complexity Δ
.../src/main/java/com/parse/GcmBroadcastReceiver.java 0% <ø> (ø) 0 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c650b2c...ec63659. Read the comment docs.

@natario1
Copy link
Contributor

Just curious, how are you going to get the gcm receiver instance to call add on it?

@mmimeault
Copy link
Contributor Author

In the constructor of a class extending it. (protected method). You call add for each other receiver.

But I would prefer just remove the final on the onReceive signature.
I don't really understand why the SDK add that limitation.

One or the other is good for me.

@natario1
Copy link
Contributor

I would also rather remove the final and maybe add the @CallSuper annotation

@Jawnnypoo
Copy link
Member

Jawnnypoo commented Apr 19, 2017

@CallSuper is a part of the support annotations library, which Parse does not currently have as a dependency. But I would be happy if it did!

@natario1
Copy link
Contributor

@Jawnnypoo
Copy link
Member

Ahh, I stand corrected. +1 for @CallSuper then!

@natario1
Copy link
Contributor

Not sure when it was introduced, but it would be great to also annotate the rest of the SDK (thinking mostly of @nullable and @nonnull )

@mmimeault
Copy link
Contributor Author

Anyway... it doesn't work because of that
https://github.com/parse-community/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ManifestInfo.java#L368-L378

The class declared needs to be GcmBroadcastReceiver.class, not only an instanceOf.

Do anybody know why there is so much validation around that GCM receiver?
https://github.com/parse-community/Parse-SDK-Android/blob/master/Parse/src/main/java/com/parse/ManifestInfo.java#L212-L214

We are using mixpanel push system and we want also to use parse for differents campaign.
I would really like to have both working together.

@rogerhu
Copy link
Contributor

rogerhu commented Apr 20, 2017

Speaking of which, does anyone have bandwidth to consider ripping out GCMService and putting them into a separate repo? Sort of like ParseGCM like we have for Parse Facebook.

This way people can start using Firebase GCM v4 but this library would provide all the necessary setup to make things work with Parse (i.e. InstandID service)

See: #445

@Jawnnypoo
Copy link
Member

@rogerhu Do you think it needs to be its own repo? Maybe it would be better to just have it as another module in this repo? (which could be deployed as its own dependency)

@rogerhu
Copy link
Contributor

rogerhu commented Apr 20, 2017

I think the challenge is that when you deploy via Gradle, it will attempt to deploy across all modules unless you explicitly specify it.

If it makes things easier to get started, we can do it until we're ready to release..

@mmimeault mmimeault changed the title Allow the GcmBroadcastReceiver to act as a composite to add features Allow the GcmBroadcastReceiver to be subclasses Apr 20, 2017
@mmimeault mmimeault changed the title Allow the GcmBroadcastReceiver to be subclasses Allow the GcmBroadcastReceiver to be subclassd Apr 20, 2017
@mmimeault mmimeault changed the title Allow the GcmBroadcastReceiver to be subclassd Allow the GcmBroadcastReceiver to be subclassed Apr 20, 2017
@rogerhu
Copy link
Contributor

rogerhu commented Apr 27, 2017

rebase

@rogerhu
Copy link
Contributor

rogerhu commented Apr 28, 2017

one more rebase needed

@racheal31ayers
Copy link

racheal31ayers commented Apr 28, 2017 via email

@rogerhu rogerhu merged commit a9de2f4 into master Apr 30, 2017
@natario1 natario1 mentioned this pull request Sep 7, 2017
@Jawnnypoo Jawnnypoo deleted the feature/composite-broadcast branch July 23, 2018 22:29
@mtrezza mtrezza added type:feature New feature or improvement of existing feature and removed type:improvement labels Dec 6, 2021
@parse-github-assistant
Copy link

The label type:feature cannot be used here.

@parse-github-assistant parse-github-assistant bot removed the type:feature New feature or improvement of existing feature label Dec 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants