Ios mute fix after discussion

[MarekSabol]

I've updated the code based on your comments. I would change one if statement after the PR 275 would be accepted.
For now I used different way to check if it is navigating at the moment.

Please review the changes so far.

Closes #194

[ianthetechie]

Thanks! It looks like you switched the navigationDelegate back to let and replaced the navigation active check with something better 👍

However, some other feedback still needs to be addressed:

1. The mute button isn't specific to the demo app but is reusable, so it should move to the FerrostarSwiftUI module. All similar controls are under FerrostarSwiftUI > Views > Controls.
2. The usage is highly specific to the demo app. Ferrostar is fundamentally a library that others use, not just a demonstration app. Have a look at PortraitNavigationOverlayView and LandscapeNavigationOverlayView for how we implement other controls as part of UI overlays. We already built the whole grid layout and have ways to configure what's shown where, as well as overrides. Moving the mute button to the top trailing position there will also conveniently mean it's shown and hidden automatically in the future based on whether the user is navigating (so your check isn't needed).

In the future, please continue pushing changes to the branch for the original PR rather than closing and opening a new one. It's difficult for us to review fresh branches every time :)

[MarekSabol]

I'm sorry :D, I have a development where I adjusted package and everything and try my things among the files, and second that is connected to git :D ... not the best practice but yeah, I will try to do it ASAP after work :D

[MarekSabol]

I will move the button to the correct file, in last commit I adjusted the code so it si way more clear, and it is using your logic "trailing" and so on. Other than moving the file is there something you don't like / I should change ? as it is simple solution for the button itself

[MarekSabol]

I found some problems and I will fix them today

[MarekSabol]

The button need the style I can't delete it.

The padding is in demonav... but it can be easily reused.

Thanks for your patience :D

[ianthetechie]

Thanks @MarekSabol!

Could you please have a read over the comments in #294 again? There is nothing "wrong" with this in the sense that it fulfills some minimal requirements, but we really need to ensure uniformity among how components like this are implemented since the intend of the original issue (#194) is to add something that will work in a wide range of apps using Ferrostar.

We can fill in the final bits if you can give this a solid first pass, but as-is this isn't something we can merge since it's highly specific to the demo app and doesn't use the grid view or general format of the other UI components.

[MarekSabol]

I have troubles to continue, I'm sorry but I cannot continue, have you ever encounter this ?

I dont know why it is not working I tried to implement it as a bool in all the layers and everything needed and then this happend

[ianthetechie]

Yes, this is a fairly normal annoyance with provisioning profiles on iOS. For a quick fix, I would change the bundle identifier to have your own prefix to be unique.

[MarekSabol]

tried that, been fighting with this for an hour :/ I will try to continue, hopefully I will able to manage to fix it and make this issue done

[MarekSabol]

I'm not paying for apple developer if thats issue

[MarekSabol]

I needed to rebase whole project, I have red written unknown team but it is working at the moment, thanks for hint.

I adjusted it based on the other button please let me know what you think :) it was extremely hard for me as I'm working with swift for first time ever :D .

Thanks for all of the patience

[MarekSabol]

at the moment it is showing all the time, but as soon as #275 will be setup we can just add if statement and it should do the work if I'm correct.

EDIT: Just checked the implementation 275, I will need to probably do something like "showZoom" but for this button to showMute or something. and map it based on navigation probably.

[ianthetechie]

I adjusted it based on the other button please let me know what you think :) it was extremely hard for me as I'm working with swift for first time ever :D .

Oh, wow! Jumping in the deep end with SwiftUI 😅

EDIT: Just checked the implementation 275, I will need to probably do something like "showZoom" but for this button to showMute or something. and map it based on navigation probably.

Yeah, exactly ;) It's more like showZoom than anything. You actually don't need to bother with checking if navigation is in progress, since the overlays should not be rendered in that case. By putting it with the other controls and using the grid view, that behavior comes "for free" :)

I probably won't get to a re-review this week, but will definitely have a look early next week by Tuesday.

[MarekSabol]

in demonav it is showing before the navigation starts :/ probably I'm missing something but maybe you will see it right away :D thanks so far :D

[ianthetechie]

I wouldn't worry about that. We're in the process of reworking that anyways. My goal is to have something that looks and behaves like the other optional controls (zoom buttons etc.) do today. We can handle the rest :)

[Archdoog]

The rest of this PR is starting to look pretty close. Very cool for just getting started with Swift!

Aside from the current code. You'll want to update the snapshots for anything that uses the grid view (ideally some with mute, some with muted). To do this you can simply:

1. Delete the snapshots that are failing after running the unit test suite.
2. Run the tests again and they'll automatically regenerate the missing snapshots.
3. Run a 3rd time and make sure everything passes.

[Archdoog]

This doesn't look quite right, but it's probably a result of our AVSpeechSpokenInstructionObserver. That class doesn't really have a good way to publish the isMuted value.

Here's an updated version which you could copy & replace to the apple/Sources/FerrostarCore/Speech.swift file:

public protocol SpokenInstructionObserver {
    
    /// Mute or unmute the speech engine.
    ///
    /// - Parameter mute: Mute the speech engine if true, unmute if false.
    func setMute(_ mute: Bool)
    
    /// Handles spoken instructions as they are triggered.
    ///
    /// As long as it is used with the supplied ``FerrostarCore`` class,
    /// implementors may assume this function will never be called twice
    /// for the same instruction during a navigation session.
    func spokenInstructionTriggered(_ instruction: SpokenInstruction)

    /// Stops speech and clears the queue of spoken utterances.
    func stopAndClearQueue()

    /// If the speech synthisizer is currently muted.
    var isMuted: Bool { get }
}

public class AVSpeechSpokenInstructionObserver: ObservableObject, SpokenInstructionObserver {
    
    @Published public private(set) var isMuted: Bool
    
    private let synthesizer = AVSpeechSynthesizer()

    public init(isMuted: Bool) {
        self.isMuted = isMuted
    }

    public func setMute(_ mute: Bool) {
        guard isMuted != mute else {
            return
        }
        
        // Immediately set the publisher. This will avoid delays updating UI.
        isMuted = mute
        
        if mute && synthesizer.isSpeaking {
            synthesizer.stopSpeaking(at: .immediate)
        }
    }
    
    public func spokenInstructionTriggered(_ instruction: FerrostarCoreFFI.SpokenInstruction) {
        guard !isMuted else {
            return
        }

        let utterance: AVSpeechUtterance = if #available(iOS 16.0, *),
                                              let ssml = instruction.ssml,
                                              let ssmlUtterance = AVSpeechUtterance(ssmlRepresentation: ssml)
        {
            ssmlUtterance
        } else {
            AVSpeechUtterance(string: instruction.text)
        }

        synthesizer.speak(utterance)
    }

    public func stopAndClearQueue() {
        synthesizer.stopSpeaking(at: .immediate)
    }
}

I'd be happy to also post a PR with this change if that'd help.

The key difference is the AVSpeechSpokenInstructionObserver will actually have an isMuted publisher on it that's designed to be used in a SwiftUI instead of a simple bool. A simple bool won't trigger updates to the button, causing the problem you were probably hoping to solve with @State.

[ianthetechie]

This is tricky. You're right that the current protocol isn't going to just drop in to a SwiftUI setup.... but I'm not sure this actually fixes the problem 😅 It handles one half (publishing updates), but it doesn't actually get us to a two-way binding.

[Archdoog]

In a SwiftUI app, I would not expect a @Binding var to actually alter the behavior of a service even though you're correct that it offers a two way street which could be used for that. Having a bespoke func setMute(_ mute: Bool) and a published state that SwiftUI can easily use seems to capture the best of a button running a behavior and a state being published for the visual UI.

A get set pattern was nice, but in this case seems like the wrong solution given the goals and SwiftUI.

[ianthetechie]

Yeah, I see your point. Though I think @Binding is at least somewhat intended for this purpose.

The problem I hit with the method-based approach, which is how you'd do it in Jetpack Compose to preserve UDF, is that things get extremely messy with the Swift type system.

At A high level, here's what I think you're suggesting:

SpokenInstructionObserver:

• Make isMuted get-only
• Add func setMuted(...)

MuteUIButton:

• Switch from @Binding to @ObservedObject and observe the SpokenInstructionObserver directly
• Rather than toggling the binding, invoke spokenInstructionObserver.setMuted(...)
• Since spokenInstructionObserver.isMuted is published, observe that value in the image composition rather than the binding

In order to use a @Published property and observe the object directly, we would need to make SpokenInstructionObserver conform to ObservableObject. This sounds trivial, but this opens up Pandora's box with errors like Type 'any SpokenInstructionObserver' cannot conform to 'ObservableObject' that highlight the flaws of the Swift type system...

I am unable to find a resolution to this at the moment. Any ideas?

[ianthetechie]

Okay... after banging my head against the wall a bit more, I think I have something that works. It's nominally better... Have a look over it and lemme know what you think. I don't really like the way it forces us to include generic bounds. And I couldn't get it to work with an optional T in the constructor, since the desirable default, nil is going to have an unknown type 😂 And any types don't work as noted. So, the only option remaining that seems to work OK is to define a dummy default, which you can override.

[ianthetechie]

@Archdoog I have pushed some changes that work, and which I'm reasonably satisfied with. Would welcome a review from you though (or maybe @michaelkirk if you're interested) to sanity check.

[MarekSabol]

Hi guys, how do you run tests ? if I open only demo app, there are no tests, but if I open the whole project I have 211 errors :D, and I can't build it

[ianthetechie]

@MarekSabol for iOS tests, open the repository in Xcode, configure the scheme if necessary, and press command+u.

However, I think I know why you're getting errors locally.... As part of this PR, I've bumped the version string in Package.swift to 0.20.0, but this hasn't actually been released yet. Maybe we should stop doing that as part of PRs since it is a bit confusing.... the trouble is with how SPM is designed to look at files in git.

To get building again, either:

1. Build locally (./build-ios.sh) and set useLocalFramework = true as described here: https://stadiamaps.github.io/ferrostar/dev-env-setup.html, OR
2. Change the version back to 0.19.0 in your local copy

[MarekSabol]

I'm sorry if I'm annoying you :D but I'm trying I tried to build it with that, and I can't make it work. To be honest I couldn't build it even before as well in core project.

I need to open the demo app as a project in xcode, but if I open that, there are no test.
I can not run any tests, not only of the part I changed.

[ianthetechie]

I'm sorry if I'm annoying you :D

No, it's no problem at all :) In fact, I'm glad that you're letting us know that the build process / instructions could be improved! That is surely discouraging other new developers too.

I need to open the demo app as a project in xcode, but if I open that, there are no test.

Correct. The demo app project does not have tests. The tests are part of the Swift package which is (perhaps confusingly) the root of the repository, because SPM requires that the Package.swift be there!

Just to double check, can you verify that your Package.swift sets the release tag to 0.19.0 like this?

When you first open the project and try to build, you may get an error about macros. Click on the error in the list, then click Trust & Enable at the pop-up:

Then you should be able to build and also to run tests. In Xcode, you'll need to configure it like this. Note that we currently use an iOS 15 simulator (we'll upgrade to 16 soon) to keep the snapshot tests stable.

[MarekSabol]

In package I had 0.17.0, I tried adjusting it to 0.19.0 and building but it didnt help

but my error looks like on the picture

I probably forgot some step or something :D ...

[ianthetechie]

Final note from the discussion: @Archdoog to do a quick pass to try and remove the generic constraint with some boxing.

[ianthetechie]

Had to bump this to resolve some whack build error that I couldn't repro locally. Can bump to 16.1 whenever GitHub updates (final release just dropped).

[ianthetechie]

@MarekSabol the problem in your screenshot looks like a platform issue. You've probably set it to build for macOS, but the project doesn't actually support macOS (since some critical dependencies are iOS-only).