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

PingHub wrap up #6437

Merged
merged 6 commits into from
Jan 23, 2017
Merged

PingHub wrap up #6437

merged 6 commits into from
Jan 23, 2017

Conversation

koke
Copy link
Member

@koke koke commented Jan 19, 2017

I've combined a bunch of small fixes into this one for the finishing details for PingHub:

  • If the application is active, it now ignores push notifications, as we assume we'll get them through pinghub. Notifications: InApp Presentation #5894 might change this in the future though
  • Updated Starscream. My previous patch introduced a new bug that meant the delegate/callback wasn't being called correctly on disconnect.
  • This, combined with the delayed retry made the state machine confused when coming from the background, and there was a lot of reconnection, since connect would disconnect the socket if it was already connecting, changing the state again 🙄 I added a guard for sanity so we don't call connect() if we're connect(ing|ed).
  • Besides (dis)connecting from state changes, the application state observers were also calling connect/disconnect on their own, causing more confusion, and even possibly bugs, since they weren't checking if a connection should happen.
  • The console messages were even more confusing because you would see 2-3 "disconnected" messages in a row. Which was OK, since it only prints "connected" when successfully connected. To have a clearer picture, now it also logs "connecting" and "disconnecting".
  • When PingHub connects, trigger a full sync. Since there might have been changes while PingHub was disconnected, this will ensure consistency. The next step would have been to remove the sync on appear for the notifications list, but I'd rather leave it for now as a sanity check.

Fixes #6164

To test:

  • Launch the app
  • Check the console and make sure "PingHub connected"
  • Generate a new note (like a new post), and verify it appears
  • Delete a note (unlike the post), and verify it appears
  • Put the app in the background and resume, verify no odd (dis)connects in the console
  • Give it a good test ride. This should be the final PingHub implementation for a while, unless there are bugs. 🎉

Needs review: @jleandroperez

koke added 6 commits January 19, 2017 11:50
We are already getting changes pushed through PingHub
This introduces a fix for my patch so it properly calls the delegate/callback
on disconnect.
Changing the state will handle connect/disconnect, there's no need to
explicitly do it when switching to background/foreground
@koke koke added this to the 7.0 milestone Jan 19, 2017
@koke koke requested a review from jleandroperez January 19, 2017 17:13
@jleandroperez
Copy link
Contributor

Looking great!! Thanks for fixing that one sir!!

:shipit:

@koke koke merged commit f04014c into develop Jan 23, 2017
@koke koke deleted the feature/pinghub-wrap-up branch January 23, 2017 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Notifications PingHub
2 participants