Skip to content

Commit

Permalink
moved setScreenName to background thread
Browse files Browse the repository at this point in the history
  • Loading branch information
@robertarnesson
robertarnesson committed Mar 12, 2018
1 parent a17435d commit afb93d2
Showing 1 changed file with 7 additions and 5 deletions.
12 changes: 7 additions & 5 deletions src/ios/FirebasePlugin.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,12 +263,14 @@ - (void)logEvent:(CDVInvokedUrlCommand *)command {
}

- (void)setScreenName:(CDVInvokedUrlCommand *)command {
NSString* name = [command.arguments objectAtIndex:0];

[FIRAnalytics setScreenName:name screenClass:NULL];
[self.commandDelegate runInBackground:^{
NSString* name = [command.arguments objectAtIndex:0];

[FIRAnalytics setScreenName:name screenClass:NULL];

CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK];
[self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId];
CDVPluginResult *pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK];
[self.commandDelegate sendPluginResult:pluginResult callbackId:command.callbackId];
}];
}

- (void)setUserId:(CDVInvokedUrlCommand *)command {
Expand Down

7 comments on commit afb93d2

@JonSmart
Copy link
Contributor

Choose a reason for hiding this comment

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

@robertarnesson I am not sure if my Pull request flowed very well, but I actually found this method needs to be on the main thread but the iOS Firebase library needs to be on 4.10, I have since been updating the plugin to fix. Hope that message makes sense?

@robertarnesson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JonSmart ok, but going back to 4.10 is not an option (other features rely on >= 4.5.0). Should it still be in main or not?

@JonSmart
Copy link
Contributor

Choose a reason for hiding this comment

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

4.10.0 is greater than 4.5?

There is actually now a 4.10.1 released on the weekend. I know 4.10.0 fixes the issue as I was able to replicate.

https://firebase.google.com/support/release-notes/ios

@robertarnesson
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JonSmart ops sorry I read that as 4.1.0. now it all makes sense. so your PR contains an upgrade to 4.10.0 and revert back to main thread? the commit message said upgrade to 4.5.0 so it was a bit confusing

@JonSmart
Copy link
Contributor

Choose a reason for hiding this comment

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

@robertarnesson Sorry, I was rushing. Been a busy day. Yes the PR upgrades to 4.10.0 and reverts the above code change.

@sandfield-andy
Copy link

Choose a reason for hiding this comment

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

@JonSmart and @robertarnesson I've had a read of this thread and it's a bit confusing. You have mentioned that the setScreenName needs to be executed using the main thread, but it is being run in the background. This is causing issues in the current version of the plugin 1.0.5. Can this please be changed to execute in the main thread?

@JonSmart
Copy link
Contributor

@JonSmart JonSmart commented on afb93d2 Jun 26, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@sandfield-andy There was another Pull Request following this one, at some point someone must have put the background wrapper back in. I see you have made a new PR (#751). @robertarnesson will need to accept and merge for this change to be put back.

Please sign in to comment.