-
Notifications
You must be signed in to change notification settings - Fork 23
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
feat: do not show modal message if change screens and page rule enabled #731
Changes from 2 commits
2db44f9
d5c5e70
48d14e6
4767e88
adc56e2
6d3f539
2919692
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import CioInternalCommon | ||
import Foundation | ||
|
||
// Allows us to mock EngineWeb instances for testing. | ||
protocol EngineWebProvider { | ||
func getEngineWebInstance(configuration: EngineWebConfiguration) -> EngineWebInstance | ||
} | ||
|
||
// sourcery: InjectRegisterShared = "EngineWebProvider" | ||
class EngineWebProviderImpl: EngineWebProvider { | ||
func getEngineWebInstance(configuration: EngineWebConfiguration) -> EngineWebInstance { | ||
EngineWeb(configuration: configuration) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,14 @@ | |
_ = Gist.shared.getMessageView(Message(messageId: "")) | ||
} | ||
|
||
// For testing to reset the singleton state | ||
func reset() { | ||
clearUserToken() | ||
messageQueueManager = MessageQueueManager() | ||
messageManagers = [] | ||
RouteManager.clearCurrentRoute() | ||
} | ||
|
||
// MARK: User | ||
|
||
public func setUserToken(_ userToken: String) { | ||
|
@@ -46,6 +54,11 @@ | |
} | ||
|
||
public func setCurrentRoute(_ currentRoute: String) { | ||
if RouteManager.getCurrentRoute() == currentRoute { | ||
return // ignore request, route has not changed. | ||
} | ||
|
||
cancelLoadingModalMessage() | ||
RouteManager.setCurrentRoute(currentRoute) | ||
messageQueueManager.fetchUserMessagesFromLocalStore() | ||
} | ||
|
@@ -126,6 +139,41 @@ | |
} | ||
} | ||
|
||
// If someone sets a page rule on a message, they want the message to show on that screen. Because messages can take multiple seconds to finish rendering, there is a chance that | ||
// a user navigates away fron a screen when the rendering finishes. To fix this, cancel showing a modal message if a message is still loading. | ||
// | ||
// Like dismiss message, but does not call event listener. | ||
// Dismiss the currently shown message, if there is one, and then remove message manager allowing us to show a message again in the future. | ||
func cancelLoadingModalMessage() { | ||
guard let messageManagerToCancel = getModalMessageManager() else { | ||
return // no message being shown or loading. | ||
} | ||
let currentMessage = messageManagerToCancel.currentMessage | ||
|
||
if messageManagerToCancel.isShowingMessage { | ||
// The modal is already visible, don't cancel it. | ||
// This can prevent an infinite loop scenario: | ||
// * page rule changed and that triggers showing a Modal | ||
// * Modal message is displayed on screen | ||
// * Modal being displayed triggers an auto screenview tracking event. This triggers a SDK page route change | ||
// * Request to cancel modal message | ||
// * Back to the foreground screen that originally triggered showing a Modal message...repeat... | ||
return | ||
} | ||
|
||
guard currentMessage.gistProperties.routeRule != nil else { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we also need to check regex as well? incase route rule is not nil and reges still matches we don't need to cancel message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At first, I thought that a regex check was overkill because the logic in However, you helped me realize that you can use wildcards in page rules. So, there is indeed a chance that you change the screen and it does match the page rule of the message. I'll fix this in another PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR created #740 |
||
// The message does not have page rules setup so do not cancel showing it. Let it proceed. | ||
return | ||
} | ||
|
||
Logger.instance.debug(message: "Cancelled showing message with id: \(currentMessage.messageId). Will try to show message again in future.") | ||
|
||
removeMessageManager(instanceId: currentMessage.instanceId) // allows us to display a message in the future. Important to do this immediately instead of waiting for current message to dismiss. | ||
|
||
// dismiss to smoothly transition off screen. | ||
messageManagerToCancel.cancelShowingMessage() | ||
} | ||
|
||
// Message Manager | ||
|
||
private func createMessageManager(siteId: String, message: Message) -> MessageManager { | ||
|
@@ -135,7 +183,7 @@ | |
return messageManager | ||
} | ||
|
||
private func getModalMessageManager() -> MessageManager? { | ||
func getModalMessageManager() -> MessageManager? { | ||
messageManagers.first(where: { !$0.isMessageEmbed }) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,3 +1,4 @@ | ||||||||||||||
import CioInternalCommon | ||||||||||||||
import Foundation | ||||||||||||||
import UIKit | ||||||||||||||
|
||||||||||||||
|
@@ -6,17 +7,18 @@ | |||||||||||||
} | ||||||||||||||
|
||||||||||||||
class MessageManager: EngineWebDelegate { | ||||||||||||||
private var engine: EngineWeb? | ||||||||||||||
private var engine: EngineWebInstance | ||||||||||||||
private let siteId: String | ||||||||||||||
private var messagePosition: MessagePosition = .top | ||||||||||||||
private var messageLoaded = false | ||||||||||||||
var messageLoaded = false | ||||||||||||||
levibostian marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||
private var modalViewManager: ModalViewManager? | ||||||||||||||
var isMessageEmbed = false | ||||||||||||||
let currentMessage: Message | ||||||||||||||
var gistView: GistView! | ||||||||||||||
private var currentRoute: String | ||||||||||||||
private var elapsedTimer = ElapsedTimer() | ||||||||||||||
weak var delegate: GistDelegate? | ||||||||||||||
private let engineWebProvider: EngineWebProvider = DIGraphShared.shared.engineWebProvider | ||||||||||||||
|
||||||||||||||
init(siteId: String, message: Message) { | ||||||||||||||
self.siteId = siteId | ||||||||||||||
|
@@ -32,11 +34,17 @@ | |||||||||||||
properties: message.toEngineRoute().properties | ||||||||||||||
) | ||||||||||||||
|
||||||||||||||
self.engine = EngineWeb(configuration: engineWebConfiguration) | ||||||||||||||
if let engine = engine { | ||||||||||||||
engine.delegate = self | ||||||||||||||
self.gistView = GistView(message: currentMessage, engineView: engine.view) | ||||||||||||||
self.engine = engineWebProvider.getEngineWebInstance(configuration: engineWebConfiguration) | ||||||||||||||
engine.delegate = self | ||||||||||||||
self.gistView = GistView(message: currentMessage, engineView: engine.view) | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
var isShowingMessage: Bool { | ||||||||||||||
guard let modalViewManager = modalViewManager else { | ||||||||||||||
return false | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
return modalViewManager.isShowingMessage | ||||||||||||||
Comment on lines
+43
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just return the value of messageLoaded from here? is there a case where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This function is meant to tell you if the message is showing, so it's already been rendered and it's on the screen. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
but looking into this method, it is where we are setting So, both are being done at the same point and rendering is also done, we are just view here?
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I worked on improvements to the test suite today, including The intention of |
||||||||||||||
} | ||||||||||||||
|
||||||||||||||
func showMessage(position: MessagePosition) { | ||||||||||||||
|
@@ -60,6 +68,16 @@ | |||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
func cancelShowingMessage() { | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should be making these methods synchronized, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do agree, better threading support in the in-app SDK is needed. I also agree this is out-of-scope because I don't think there is a quick win for MessageManager, for example, synchronized when the rest of the module could also be improved. I think bigger module-wide refactors could be a more productive approach to this problem. I also think that swift concurrency could solve this problem really well. Do you disagree and think there is a quick win here? Is it a blocker? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could definitely increase the scope, happy for it be a follow up 👍 |
||||||||||||||
if messageLoaded { | ||||||||||||||
return // if message has already finished loading, do not cancel. Keep it shown. | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
engine.delegate = nil // to make sure we do not get a callback when message loaded and we try to show it. | ||||||||||||||
|
||||||||||||||
dismissMessage() // to gracefully animate the removal of modal from UI if it began to show before. | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
func dismissMessage(completionHandler: (() -> Void)? = nil) { | ||||||||||||||
if let modalViewManager = modalViewManager { | ||||||||||||||
modalViewManager.dismissModalView { [weak self] in | ||||||||||||||
|
@@ -82,7 +100,7 @@ | |||||||||||||
|
||||||||||||||
// Cleaning after engine web is bootstrapped and all assets downloaded. | ||||||||||||||
if currentMessage.messageId == "" { | ||||||||||||||
engine?.cleanEngineWeb() | ||||||||||||||
engine.cleanEngineWeb() | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
|
@@ -223,8 +241,7 @@ | |||||||||||||
} | ||||||||||||||
|
||||||||||||||
deinit { | ||||||||||||||
engine?.cleanEngineWeb() | ||||||||||||||
engine = nil | ||||||||||||||
engine.cleanEngineWeb() | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
private func showNewMessage(url: URL) { | ||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,12 @@ public enum MessagePosition: String { | |
} | ||
|
||
class ModalViewManager { | ||
var window: UIWindow! | ||
var window: UIWindow? | ||
var viewController: GistModalViewController! | ||
var position: MessagePosition | ||
var isShowingMessage: Bool { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just a note: if we do end up going with |
||
window != nil | ||
} | ||
|
||
init(gistView: GistView, position: MessagePosition) { | ||
self.viewController = GistModalViewController() | ||
|
@@ -21,8 +24,8 @@ class ModalViewManager { | |
func showModalView(completionHandler: @escaping () -> Void) { | ||
viewController.view.isHidden = true | ||
window = getUIWindow() | ||
window.rootViewController = viewController | ||
window.isHidden = false | ||
window?.rootViewController = viewController | ||
window?.isHidden = false | ||
var finalPosition: CGFloat = 0 | ||
|
||
switch position { | ||
|
@@ -69,6 +72,7 @@ class ModalViewManager { | |
self.window?.isHidden = false | ||
self.viewController.removeFromParent() | ||
self.window = nil | ||
|
||
completionHandler() | ||
}) | ||
}) | ||
|
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 code gets removed in another PR in stack #732
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 code originally existed to prevent an infinite loop but it does not handle test case explained in this PR description.