Skip to content
This repository has been archived by the owner on Aug 3, 2019. It is now read-only.

Part 2 of handling session #200

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

konigsoft
Copy link

No description provided.

@bperin
Copy link
Contributor

bperin commented Jul 20, 2014

I added both your patches and still can't get a successful login with a new app. It looks like you're missing some basic formatting too, ie missing ; and there is no variable defined as uiHelper, maybe you mean uiLifecycleHelper

Curious if you got this to work, was your app created before april 30 2014? I think that is the cut off for using the 1.0 v 2.0 graph backend.

@sromku sromku mentioned this pull request Jul 27, 2014
@sromku
Copy link
Owner

sromku commented Jul 27, 2014

@konigsoft First of all thanks a lot for contribution!
Few things about this change:

  1. I can't see the point of adding onResume() and onPause() and of course nor onDestroy(). If you check the code of each of these methods, you can see that SessionManager already manage it. The only thing that is given there is that session callback is registered on resume and unregistered on pause. Please let me know, why do you think we need this?
  2. Please pay attention for formatting of the code and follow the same formatting.
  3. The only thing that I need to see if it's really needed or not is onSaveInstanceState(). If it really needed then you are missing onCreate() method in your pull request. But, as it seems to me from FB source code, the only reason is when the configuration was changed like screen orientation in the middle of facebook request.
  4. Please put all commits of the same feature into one merge request (Handle login state in the new facebook sdk #199), (Part 2 of handling session #200), (Update README.md #201)
    Currently I can't push this merge.

* code to handle session login state
*/
public boolean onResume() {
uiHelper.onResume();
Copy link
Owner

Choose a reason for hiding this comment

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

uiHelper - no such member in this class. It won't compile.

Copy link
Author

Choose a reason for hiding this comment

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

mix the variable from facebook and your code
the correct syntax should be (and the same for the other) uiLifecycleHelper.onResume();

@konigsoft
Copy link
Author

@sromku I have try your code but as I said i was facing the issue of getting a login state to false when i close my app and open it back.
So analysing your code and the one of facebook I have noticed that when i had this line of code I have a proper login state.
I have look at the session manager and I can't see the management I were looking for.
I will try several combination and pull a request for the minimalist code change that handle this.
sorry for puling three pull request will leran more about github pulling request and sorry for the code too.
thank you for this great lib

@sromku
Copy link
Owner

sromku commented Jul 29, 2014

@konigsoft let me go over the code and make some tests. Don't create a new pull request meanwhile. If I see that it works good, I will merge your pull request and do few updates. Thanks anyway! I will update you soon.

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.

3 participants