Remote Messaging Framework for macOS #2937
Conversation
| @@ -103,6 +105,26 @@ extension HomePage.Views { | |||
| } | |||
| } | |||
|
|
|||
| @ViewBuilder | |||
| func remoteMessage() -> some View { | |||
| if let remoteMessage = activeRemoteMessageModel.remoteMessage, let modelType = remoteMessage.content, modelType.isSupported { | |||
There was a problem hiding this comment.
maybe guard would be nicer here to avoid this extra indentation?
There was a problem hiding this comment.
Yep, that would be great, but guard won't work here because it's a view builder – guard requires a return statement and ViewBuilder doesn't expect return statements :/
| func startRefreshingRemoteMessages() { | ||
| guard remoteMessagingAvailabilityProvider.isRemoteMessagingAvailable else { | ||
| stopRefreshingRemoteMessages() | ||
| return | ||
| } | ||
| /// Put the actual timer start into a separate function to allow it to be called unconditionally from | ||
| /// `isRemoteMessagingAvailablePublisher` event handler, where the new value (true) is emitted but it's | ||
| /// not yet available from `remoteMessagingAvailabilityProvider.isRemoteMessagingAvailable` property getter. | ||
| startMessagesRefreshTimer() | ||
| } | ||
|
|
||
| private func startMessagesRefreshTimer() { | ||
| scheduledRefreshCancellable = Timer.publish(every: Constants.minimumConfigurationRefreshInterval, on: .main, in: .default) | ||
| .autoconnect() | ||
| .prepend(Date()) | ||
| .asVoid() | ||
| .sink { [weak self] in | ||
| self?.refreshRemoteMessages() | ||
| } | ||
| } |
There was a problem hiding this comment.
Having these two methods makes the code somewhat confusing, though I understand the use case. To simplify, consider adding a default argument to startRefreshingRemoteMessages, like this:
func startRefreshingRemoteMessages(forceStart: Bool = false). If forceStart is set to true, it bypasses the guard, allowing safe calls from the publisher. This also enables you to place your comment next to the forceStart argument, slightly making the documentation more obvious to the user :)
There was a problem hiding this comment.
Yep, that makes perfect sense. Let me update it.
| import NetworkProtection | ||
| import Subscription | ||
|
|
||
| final class RemoteMessagingConfigMatcherProvider: RemoteMessagingConfigMatcherProviding { |
There was a problem hiding this comment.
do you think this code could be also unified between platforms?
There was a problem hiding this comment.
This can't be unified (at least not without a fundamental rewrite which would also be worth considering), because the userAttributeMatcher argument is a different object on iOS and macOS (macOS doesn't have isWidgetInstalled and will get a few more attributes in the future).
| case .autoRenewable: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "auto_renewable")) | ||
| case .notAutoRenewable: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "not_auto_renewable")) | ||
| case .gracePeriod: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "grace_period")) | ||
| case .inactive: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "inactive")) | ||
| case .expired: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "expired")) | ||
| case .unknown: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "unknown")) | ||
| case nil: break |
There was a problem hiding this comment.
perhaps consider making subscription status conform to String, or if it is not possible make custom rawValue func so you can just change it to
guard let status = subscription?.status else { break }
queryItems.append(URLQueryItem(name: parameter.rawValue, value: status.rawValue))
There was a problem hiding this comment.
Yep, great idea. I haven't spent much time with this class, it was copied over 1:1 from iOS, and I actually considered moving it to BSK. Let my try to do it now.
There was a problem hiding this comment.
Alright, I've moved DefaultRemoteMessagingSurveyURLBuilder to BSK (which resulted in an extra commit to BSK and iOS 😇 , but it was worth it!) cc @samsymons
| case .apple: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "apple")) | ||
| case .google: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "google")) | ||
| case .stripe: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "stripe")) | ||
| case .unknown: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "unknown")) | ||
| case nil: break | ||
| } | ||
| case .privacyProBilling: | ||
| switch subscription?.billingPeriod { | ||
| case .monthly: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "monthly")) | ||
| case .yearly: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "yearly")) | ||
| case .unknown: queryItems.append(URLQueryItem(name: parameter.rawValue, value: "unknown")) | ||
| case nil: break |
| private func hardwareModel() -> String { | ||
| var systemInfo = utsname() | ||
| uname(&systemInfo) | ||
|
|
||
| let machineMirror = Mirror(reflecting: systemInfo.machine) | ||
| let identifier = machineMirror.children.reduce("") { identifier, element in | ||
| guard let value = element.value as? Int8, value != 0 else { return identifier } | ||
| return identifier + String(UnicodeScalar(UInt8(value))) | ||
| } | ||
|
|
||
| return identifier |
There was a problem hiding this comment.
looks like it belongs to some common class, not necessarily here
Task/Issue URL: https://app.asana.com/0/72649045549333/1202913520695928/f
Description:
RMF is now hidden behind a feature flag on macOS.
Steps to test this PR:
https://www.jsonblob.com/api/1258315611053613056for debug builds../clean-app.sh debug.Also check multiple windows:
Definition of Done:
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation