Start/stop engine when entering background/foreground

[kkiermasz]

Hi there,

This PR resolves issue #69.

My original assumption was that usesCustomHaptics was created to cover all the scene phase changes. Unfortunately, it does not cover them all. Hard to tell why, as _AppearanceActionModifier is a hidden API 🤭

func usesCustomHaptics() -> some View {
    modifier(
        _AppearanceActionModifier {
            Haptics.acquire()
        } disappear: {
            Haptics.release()
        }
    )
}

Steps to reproduce: Check the haptics associated with the Spray effect. Then minimize the app and open it again. Before the change, the haptics wouldn't work.

With the change, the Spray effect provides a full experience again 🥰

I'm aware of the @Environment(\.scenePhase). However, Haptics are implemented as a set of static methods, so connecting scenePhases with it does not make much sense to me, but I'm happy to hear your opinion. Haptics are shipped to iOS targets only, so using NotificationCenter is safe.

[mergesort]

Hey there @kkiermasz, thank you for the contribution! I took a quick look at the code and I believe that there are two issues that need to be addressed before this change can be merged in.

1. This change would Pow to not compile on macOS, since the notifications you're watching only exist on iOS. (They may also not exist on tvOS, but I haven't checked before writing this comment.)
2. I'm a little hesitant about the separation of concerns, because of two subtle issues.
   • I don't think an internally referenced CHHapticEngine engine should necessarily know anything about whether an app is entering the background or foreground.
   • This change is being applied to a computed property (static var engine: CHHapticEngine?). This means these notifications will be cumulatively added every time a new engine is referenced, not only the first time an engine is initialized.

Because of that I think the likely the right place to make this change is in the caller who instantiates this internal CHHapticEngine, HapticEngineSoundEffectPlayer.

Could I ask you to rework the code with these considerations in mind, so I can merge this in and fix up the spray animation?

[kkiermasz]

Hey @mergesort,

Thanks for the feedback!

1. Just to clarify what I meant in the PR description by "Haptics are shipped to iOS targets only, so using NotificationCenter is safe." – the project will still compile across all platforms. Here's Haptics declaration:

   
   #if os(iOS)
   import CoreHaptics

internal struct Haptics {

2. The `static var engine: CHHapticEngine?` is a closure-initialized property, not a computed one. It behaves like a lazy property, so the notifications are only added the first time the engine is initialized, not every time it’s accessed. I think this is a good place for that logic.
3. I agree that the `CHHapticEngine` ideally shouldn't need to handle app state changes, and it’s surprising iOS doesn’t manage this automatically. But since it doesn’t, and given how the Haptics utility is currently set up, I think this is the best spot for now, as I don't see a good alternative. Open to suggestions if you see a better way to handle it!

Let me know what you think 😊

[mergesort]

1. Gotcha, that makes sense!

2. You're totally right @kkiermasz, I missed that engine was a closure and not a computed property, that's my bad. Let's just pretend I never said that. 🪄✨

3. The place I was thinking of adding the notification handling is actually in the HapticEngineSoundEffectPlayer, the only caller of the engine property we're currently adding these notifications to. I think that it should be better place to handle side-effects because the actor has it's own state, rather than coupling the code to the internal closure.

   I would probably put this at the end of the setUp method, which should only get called once because it won't re-trigger once didSetUp is set to true at the end of the function. It also appears this would only be shipped on iOS, so that has the benefit you mentioned in point 1!

Would love to hear what you think about this suggestion, would love to get this merged in. 🙂

[kkiermasz]

Hey @mergesort,

Thanks for the suggestion!

Unfortunately, I don't think that would solve the issue. The setUp method you mentioned spawns a new instance of CHHapticEngine and doesn’t refer to any global state. This means that the particular instance it creates is only used for sound effects, and not all of them—since AVAudioSession is used for some as well.

Given this, I’d like to stick with my solution. I believe the issue might be related to the HapticEngineSoundEffectPlayer's engine instance not being aware of background mode entry, which could be causing the other issue mentioned by @dimabiserov. However, as mentioned in the PR description, this change specifically resolves the missing vibrations (#69).

Let me know your thoughts!

[mergesort]

Hey @kkiermasz, I'm not sure I quite understand what you mean here. I gave the refactor that I had proposed a shot, and looks like you're right! I hadn't realized that the CHHapticEngine I was referring to was specifically tied to SoundEffect, as opposed to the HapticFeedbackEffect.

In that case I think you're right to take your approach, and I think we should go with it. I would like to ask you to do one thing though before I can merge it in. Can you take the notification handlers and add them into a private function called addHapticEngineObservers(), and add some documentation that explains the issue (and points back to the issue you'd opened)?

This way if someone comes across the code and wonders why this is there (or me in six months when I forget why we added this), they'll be able to understand why this code that otherwise would look out of place exists.

Thanks a lot, and I appreciate you being so willing to communicate thoroughly over what is a small code change.

[kkiermasz]

@mergesort done

[mergesort]

LGTM, thank you for the good work @kkiermasz!