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

Why doesn't Init(...) follow the same "structure" as all other IFacebook methods? #9

Closed
Whyser opened this issue Nov 12, 2015 · 8 comments

Comments

@Whyser
Copy link

Whyser commented Nov 12, 2015

Sorry for the vague description in the title.

If you look at all the methods available in the IFacebook.cs interface, almost all have a callback which looks like FacebookDelegate<IResult> where IResult may differ depending on the call. But the Init(...) method doesn't follow this. It has it's own InitDelegate delegate which feels kinda out of place and doesn't return any information. And that would be fine...But the method OnInitComplete(string message) will in turn call OnLoginComplete(message) for some reason (and what should message be in this case, since I don't have any data to go on?).

Why does all of this matter to me? I'm trying to wrap the new UWP Facebook SDK (from Microsoft) into your SDK. And I'm trying to make sure everything follows the same event-stucture as Android and iOS and the only thing missing now is the Init-method which is...weird.

Summary:
I want to be able to do the following:
string callback_id = CallbackManager.AddFacebookDelegate(myInitCallback).toString();
var dontKnowWhatItShouldContainOtherThenCallbackId = new Dictionary<string,object>();
dontKnowWhatItShouldContainOtherThenCallbackId.Add("callback_id", callback_id);
OnInitComplete(MiniJSON.Json.Serialize(dontKnowWhatItShouldContainOtherThenCallbackId)); //This should also invoke myInitCallback as it does with all other methods.

@swiese
Copy link
Contributor

swiese commented Nov 12, 2015

Nice! Are you using the SDK from Microsoft at
https://github.com/Microsoft/winsdkfb
Let me know if you need any help getting this working. This is something that we have wanted to do for a bit but haven't had the bandwidth to tackle. The SDK there is missing some of the features of our SDK but it would be awesome to get it included, maybe as a beta feature, and add functionality as it becomes available in that sdk.

Upgrading this is something we never got around to upgrading the sdk from v6 to v7. It could be done pretty easily on the backend but we would want the public method to remain the same.

@Whyser
Copy link
Author

Whyser commented Nov 12, 2015

Yes I'm using the SDK you mentioned. :)

I've successfully implemented everything and it's working and is following the same 'design' as the rest of the SDK, but I'm unsatisfied by the method Init(...) as I've stated in my original post. What do you mean it could easily be done on the back-end but not the front-end? I see no issue with the user being exposed to FacebookDelegate<IInitResult> instead of InitDelegate. It would be logical for IInitResult to maybe contain information if the user is already logged since OnInitComplete already calls OnLoginComplete (for some reason?).

Sorry if there's something I'm missing here!

@swiese
Copy link
Contributor

swiese commented Nov 12, 2015

Generally in our SDKs we try to avoid changes that a breaking unless completely necessary or we move to a new sdk such as v8.

I agree that it would be nice to have some of the information such as the access token. What I think would be nice if we

  1. Add a new init call that takes in a FacebookDelegate
  2. Deprecate the old InitCalls but keep them working by just wrapping the existing OnInitDelegate inside a new FacebookDelegate.

@Whyser
Copy link
Author

Whyser commented Nov 16, 2015

Looking forward to the Init(...) changes!

I've now made a pull request containing the support for UWP.

@hassanselim0
Copy link

Thanks for putting the effort in bringing Windows to the SDK, this is something I needed and wasn't sure how to do myself :)
Does the master branch on your fork have this feature working? anything special I need to do in order to get it working?

@Whyser
Copy link
Author

Whyser commented Nov 30, 2015

The master branch on my fork should be working, but only for UWP apps (Windows 10 (and Mobile)) at the moment. It's worth noting though that the winsdkfb has a lot of bugs which seems to get resolved very slowly. In other words, not production ready, but you should be able to implement it meanwhile waiting for updates. Further questions regarding the winsdkfb should probably be posted here: #10 or on the issue-page on my fork. :)

@swiese
Copy link
Contributor

swiese commented Jan 4, 2017

Windows phone is no longer supported in unity

@swiese swiese closed this as completed Jan 4, 2017
@hassanselim0
Copy link

I just checked the last few unity release notes. It seems that they stopped supporting WP8, but WP8.1 is still supported (as well as Win10 Mobile).

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

No branches or pull requests

3 participants