Making `addActivityIndicator` of `FUIAuthBaseViewController` public

[AndriyGo]

As I am implementing the custom authentication via Facebook, I am subclassing the FUIFacebookAuth class. One problem which I ran into is that the addActivityIndicator of FUIAuthBaseViewController is not public. This means that I cannot tell the framework to display an activity indicator during the execution of completeSignInFlowWithAccessToken method.

I think this is not right. As long as I am using the default FUIAuthBaseViewController (or even if I subclass it), I should have access to this helper method.

I am wishing to open a PR to address this by making the addActivityIndicator public. I guess all that I need to do is to move its declaration from FUIAuthBaseViewController_Internal.h to FUIAuthBaseViewController.h.

The only reason I am opening this issue is that you suggest doing so in your contributing guidelines before submitting a PR.

Please let me know what you think about this proposed change and whether you are happy to approve it :)

[morganchen12]

I'm ok with this change.

[AndriyGo]

@morganchen12 👍

To be honest after reading through declarations in the FUIAuthBaseViewController_Internal.h I am wondering whether all of these functions should be exposed? I could not isolate any single one which needs to stay private for security/other reasons.

[morganchen12]

Part of it is keeping functions closed makes it easier to support/update the SDK, since changes to those functions if they're private don't constitute breaking API changes. So each part of the interface we make public should have at least one use case in mind, otherwise we're increasing our support difficulty for no real benefit.

[AndriyGo]

@morganchen12 makes sense. In this case, I would propose to expose a few more methods. Though I do not need them at the moment, I think that others may require them when implementing custom things. The full list is below:

1. incrementActivity and decrementActivity
2. showSignInAlertWithEmail
3. addActivityIndicator

I think that when subclassing authentication providers or authentication view controllers these methods may be required. Other methods seem to be helpers which, even if required, can be easily implemented.

[morganchen12]

👍

Please wait for @protocol86's feedback before continuing.

[AndriyGo]

@protocol86 is there any chance you could have a look at this? I would like to submit this PR as soon as possible :)

[morganchen12]

He's currently out of office, but will be back on Monday.

[AndriyGo]

@morganchen12 @protocol86 sorry guys for being so pushy, but is there any chance you could let me know if such PR can be merged? 😊

[AndriyGo]

@morganchen12 @protocol86 guys, is this still on your radar?

[morganchen12]

Go ahead and make the PR. It should only require a minor version bump, since no existing API is being modified.

[AndriyGo]

@morganchen12 thank you! PR #437 is submitted and it passed all checks.

[morganchen12]

Looks like this was fixed a while ago but not closed in the PR. Closing now.