Skip to content
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

Let speak instructions without route progress. #32

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

StephanPartzsch
Copy link
Contributor

No description provided.

@StephanPartzsch StephanPartzsch force-pushed the speak-without-route-progress branch from 5b91c77 to 7f56bd1 Compare March 16, 2024 11:55
Copy link
Collaborator

@boldtrn boldtrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see my comment, I think your proposed version is not correct yet.

@StephanPartzsch StephanPartzsch force-pushed the speak-without-route-progress branch 2 times, most recently from c1363df to 39f9f27 Compare March 18, 2024 07:39
@StephanPartzsch StephanPartzsch requested a review from boldtrn March 18, 2024 07:51
@StephanPartzsch StephanPartzsch force-pushed the speak-without-route-progress branch from 39f9f27 to e0339ef Compare March 18, 2024 08:48
@@ -264,8 +257,8 @@ public protocol VoiceControllerDelegate {

- parameter voiceController: The voice controller that will speak an instruction.
- parameter instruction: The spoken instruction that will be said.
- parameter routeProgress: The `RouteProgress` just before when the instruction is scheduled to be spoken.
- parameter routeProgress: The `RouteProgress` just before when the instruction is scheduled to be spoken. Could be `nil` if no progress is available or if oit should be ignored.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- parameter routeProgress: The `RouteProgress` just before when the instruction is scheduled to be spoken. Could be `nil` if no progress is available or if oit should be ignored.
- parameter routeProgress: The `RouteProgress` just before when the instruction is scheduled to be spoken. Could be `nil` if no progress is available or if it should be ignored.

Minor typo

@@ -360,7 +360,7 @@ extension ViewController: VoiceControllerDelegate {
print(interruptedInstruction.text, interruptingInstruction.text)
}

func voiceController(_ voiceController: RouteVoiceController, willSpeak instruction: SpokenInstruction, routeProgress: RouteProgress) -> SpokenInstruction? {
func voiceController(_ voiceController: RouteVoiceController, willSpeak instruction: SpokenInstruction, routeProgress: RouteProgress?) -> SpokenInstruction? {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a CHANGELOG entry for the changed APIs? Both here and for the RouteVoiceController?

return
}

open func speak(_ instruction: SpokenInstruction, with locale: Locale?, ignoreProgress: Bool = false) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I am not sure if we really need the ignoreProgress switch here?

We would need to make route progress optional as well for modifiedInstruction.attributedText(for: routeProgress.currentLegProgress).

Copy link
Collaborator

@boldtrn boldtrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We really need a CHANGELOG entry if we change public APIs

@StephanPartzsch StephanPartzsch requested a review from boldtrn March 18, 2024 12:21
Copy link
Collaborator

@boldtrn boldtrn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM 👍

@boldtrn boldtrn merged commit d1e2581 into maplibre:main Mar 18, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants