Room Enter / Exit Local Push Notification Feature #36
Conversation
nesimtunc
changed the title
CoThings/Models.swift
Outdated
| @@ -11,6 +11,9 @@ import Foundation | |||
| let ServerHostNameKey = "serverHostname" | |||
| let PassOnboardingKey = "passOnboarding" | |||
| let LastEnteredRoomIdKey = "lastEnteredRoomId" | |||
| let NotifyOnEnterKey = "notifyOnEnter" | |||
There was a problem hiding this comment.
I think it would be better to collect all these keys under an enum: String so later on we can understand where these keys belongs to. An enum like UserDefaultKeys
There was a problem hiding this comment.
What is the situation where the user wants to get notifications for enter or exit without specifying a special room. I wouldn't like to get all the irrelevant notifications from other rooms while only waiting for the bathroom to be available
There was a problem hiding this comment.
nah this is not that feature, yet. for that we need server side implementation.. this is only for let use know when they entered a room or exited from a room so they don't have to manually update it.
It would be also good to let them know that they have already in the room in the UI (Home screen), or disabled -/+ buttons. But they might want to adjust the number, not everyone going to use the app..
There was a problem hiding this comment.
I see, it is okay as far as it is opt in.
| @@ -53,6 +50,7 @@ class BeaconDetector: NSObject, ObservableObject, CLLocationManagerDelegate { | |||
| private(set) var exits = PassthroughSubject<Room.ID, Never>() | |||
|
|
|||
| private var locationManager = CLLocationManager() | |||
| private var notificationService = NotificationService() | |||
There was a problem hiding this comment.
I believe BeaconDetector is a self contained service and shouldn’t depend on any other services like sessions or notification services. BeaconDetector already publishes enters and exits, so showing notification could be lifted up to place session or somewhere else to make BeaconDetector totally isolated.
|
|
||
| struct NotificationSettings: View { | ||
|
|
||
| @ObservedObject var notificationService = NotificationService() |
There was a problem hiding this comment.
Is this settings view owns the notification service? Because we’re creating a new instance for us, remember this new notification service is going to be created every time this view created from scratch.
There was a problem hiding this comment.
right ... I'll fix it.
| return notifyOnEnter || notifyOnExit || notifyWithSound | ||
| } | ||
|
|
||
| func showPushNotificationIfEnabled(for actionEntered: Bool, title: String, message: String) { |
There was a problem hiding this comment.
Is notification service responsible for showing notifications to the user? If that so enter/exit shouldn’t be part of it’s api. Enter exit is solely known by beacon detector and it’s published properties. Also ifenabled is internal state of this object. Public function shouldn’t leak that detail to outer world. If we want to enable/disable some sort of notifications we can use something like a channel analogy. So for example
showNotification(on channel: NotificationChannel, title: String, message: String, withSound: Bool = true)
disableChannel(_ channel: NotificationChannel)
enableChannel(_ channel: NotificationChannel)There was a problem hiding this comment.
Or instead of enableChannel/disableChannel we can have
var enabledChannels: Set<NotificationChannel>() or even better we can make it immutable and whenever a notification setting changes (like enabling a channel) we can create a new notification settings with enabled channels.
There was a problem hiding this comment.
you mean we let user have optional notification preference for each channel? this is only for the case of rooms who have beacon. rest of the rooms are manually updated, like washing machines, so we don't need notify them.
nesimtunc
changed the title
always update one push notification to prevent notification noisy
|
We got 2 new issues @umurgdk
Shall we merge this pull request and create these issues as bugs? |
TODO:
Closes #29