-
Notifications
You must be signed in to change notification settings - Fork 262
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
Fixing redisplaying for dynamic trigger IAMs #745
Conversation
ad8af10
to
b34e530
Compare
#pragma mark OSTriggerControllerDelegate Methods | ||
- (void)triggerConditionChanged:(NSString *)triggerId { | ||
[OneSignal onesignal_Log:ONE_S_LL_VERBOSE message:[NSString stringWithFormat:@"trigger condition changed for triggerId: %@", triggerId]]; | ||
[self makeRedisplayMessagesAvailableWithTriggers:@[triggerId]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this line needed? on Android, we are only depending on dynamicTriggerCompleted, on the case the dynamic trigger is scheduled then ti will call triggerConditionChanged and dynamicTriggerCompleted, dynamicTriggerCompleted will override triggerConditionChanged changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this delegate method but I needed to add redisplayEnabled to the initWithCoder methods. Otherwise it hthinks redisplayEnabled is false
[self makeRedisplayMessagesAvailableWithTriggers:@[triggerId]]; | ||
} | ||
|
||
- (void)makeRedisplayMessagesAvailableWithTriggers:(NSArray<NSString *> *)triggerIds { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic is deferring a bit from Android, do we want to come with a common logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After working on it I think that not duplicating the state in the redisplayedMessages might be better what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that can be confusing having 2 different lists, although I see that having the redisplay messages separated can optimize the iteration. Because on worst case redisplay list will be the same length as the iam list, but in the best case redisplay iams will be empty. But is also true that there are not huge lists, or they shouldn't be, so there is not that much impact on the performance. So if you see that is more readable to only iterate iams list then I'm ok with removing the redisplay iteration for trigger change check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I think having it more readable and not trying to synchronize the state is the better approach here. Sorry for making you implement it in the first place!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok agree then, will make the change under major release branch
There is a bug on master this PR also happens to address
|
This PR allows IAMs with only dynamic triggers to be redisplayed by setting
message.isTriggerChanged = YES;
when the trigger fires. AdditionallyevaluateMessages
now evaluates message triggers before setting redisplay dataThis change is