Simplify will voice and will display APIs #1233
Conversation
Guardiola31337
added
⚠️ DO NOT MERGE
Guardiola31337
force-pushed
the
pg-1224-intercept-voice-banner-instructions
branch
from
5a08670
to
ab5c9ff
Compare
|
@Guardiola31337 will this PR allow me to update the Banner Instruction and the Instruction List? Updating just the Banner Instruction will only update the current Instruction Text I believe if the user taps on this they can see all the upcoming instuctions i would want to overwrite/hide the arrival instructions so this doesn't confuse the users as i'm using the Map Matching API after 100 Coordinates the user actually hasn't arrived at the destination. Many Thanks. |
| String textBanner = bannerInstructionsListener.willDisplay(instructions); | ||
| List<BannerComponents> bannerComponents = new ArrayList<>(ONE); | ||
| bannerComponents.add(instructions.primary().components().get(FIRST).toBuilder().text(textBanner) | ||
| .abbreviation(textBanner).build()); |
There was a problem hiding this comment.
you don't have to set an abbreviation, and there shouldn't be one unless there's an abbreviationPriority as well.
There was a problem hiding this comment.
Nah, if not included the banner is not overridden at all. This is because how the final banner text to display is generated. We should revisit that logic.
| return bannerInstructionsListener.willDisplay(instructions); | ||
| String textBanner = bannerInstructionsListener.willDisplay(instructions); | ||
| List<BannerComponents> bannerComponents = new ArrayList<>(ONE); | ||
| bannerComponents.add(instructions.primary().components().get(FIRST).toBuilder().text(textBanner) |
There was a problem hiding this comment.
The problem with this is it's going to get rid of any formatting (abbreviations, highway signs). So, if the developer is only overriding some of the instructions, it's still going to remove the formatting from all of them. Also, do we think some advanced developers might take advantage of the abbreviations and/or road signs?
There was a problem hiding this comment.
The problem with this is it's going to get rid of any formatting (abbreviations, highway signs). So, if the developer is only overriding some of the instructions, it's still going to remove the formatting from all of them.
Yeah, that's true and the tradeoff that we need to discuss / agree on.
Also, do we think some advanced developers might take advantage of the abbreviations and/or road signs?
Nope as far as I know.
That being said, I'd prefer to keep it simple for now (until a specific request comes in and at that point we can revisit).
| } | ||
| return announcement; | ||
| } | ||
|
|
||
| BannerInstructions onBannerDisplay(BannerInstructions instructions) { | ||
| if (bannerInstructionsListener != null) { | ||
| return bannerInstructionsListener.willDisplay(instructions); | ||
| String textBanner = bannerInstructionsListener.willDisplay(instructions); | ||
| List<BannerComponents> bannerComponents = new ArrayList<>(ONE); |
There was a problem hiding this comment.
can you rename this bannerComponentsList? It's confusing since the object name is plural (BannerComponents).
There was a problem hiding this comment.
Yeah, as you mentioned the problem here is that the object name is plural (not the best one though IMO). Will try to come up with a better name and update. Thanks for flagging.
| */ | ||
| SpeechAnnouncement willVoice(SpeechAnnouncement announcement); | ||
| String willVoice(SpeechAnnouncement announcement); |
There was a problem hiding this comment.
👍 This is a good idea, in the case of speech announcements which are simpler.
There was a problem hiding this comment.
Yeah, as you mentioned voice instructions are simpler because basically there's only one option to override - the announcement that you want the speech engine to speak. To be concrete here I want to note that we're removing the ssmlAnnouncement - which I don't think that any developer would want to take advantage of but as said, if the time comes we can revisit.
|
Hey @Danny-James 👋 thanks a lot for the feedback here. You're correct here >
To achieve this, we will need to revisit how the instructions list works and expose an API that allows so. But I think it's technically feasible. We will prioritize internally and discuss a solution for this and then report back here on this ticket. Thanks again! |
|
Another use case to think about / discuss is what to do with the "then" banner instruction when clients override the banner with the
Maybe, hiding it for now? |
In my case @Guardiola31337 the in my turn i'd like the ability to hide this when required, would this be possible as you can see i would like to overide the main text with something like |
|
@Danny-James |
There was a problem hiding this comment.
Per our discussion, we will simplify these APIs for now as no one has specific needs for SSML voice instructions or complex banner updates. Thanks for running with this @Guardiola31337.
Please rebase and 🚢 🚀
Guardiola31337
force-pushed
the
pg-1224-intercept-voice-banner-instructions
branch
from
ab5c9ff
to
1070610
Compare
|
Approved and moved the topic I brought up into it's own issue #1262 |
|
Let's split this into two efforts, one for voice instructions and one for banner instructions. |
Guardiola31337
force-pushed
the
pg-1224-intercept-voice-banner-instructions
branch
from
d893576
to
b5bd454
Compare
Voice instructions follow up work 👉 #1281 |
|
@Guardiola31337 do you have an idea of when the BannerText changes will be released? |
As you've seen above, the display changes are not trivial. There are some design questions that need answers before implementing a solution that covers all the cases / scenarios. So we're going to keep discussing the intended functionality internally and then report back when we have a plan of attack. That being said, for now, in order to override the banner instruction text (I believe that's what you're asking for in #1224, right?) you can implement the following when @Override
public BannerInstructions willDisplay(BannerInstructions instructions) {
List<BannerComponents> bannerComponents = new ArrayList<>();
bannerComponents.add(instructions.primary().components().get(0).toBuilder().text("All banners will be the same.")
.abbreviation("All banners will be the same.").build());
BannerInstructions bannerToBeDisplayed = instructions.toBuilder().primary(instructions.primary().toBuilder()
.components(bannerComponents).build()).build();
return bannerToBeDisplayed;
}The snippet above showcases how to override the step primary text that is displayed. You can add / modify the logic in Hope this helps. Thanks! |
|
@Guardiola31337 do this mean anything to you? Code @Override
public BannerInstructions willDisplay(BannerInstructions instructions) {
if (instructions.primary().text().contains("arrive") && points.size() > 1){
Log.d(TAG, "Overide Banner Text");
List<BannerComponents> bannerComponents = new ArrayList<>();
bannerComponents.add(instructions.primary().components().get(0).toBuilder().text("Continue along route")
.abbreviation("Continue").build());
BannerInstructions bannerToBeDisplayed = instructions.toBuilder().primary(instructions.primary().toBuilder()
.components(bannerComponents).build()).build();
return bannerToBeDisplayed;
}
return instructions;
}Error |
|
@Guardiola31337 i've taken out the following section of the code However, the See below;
|
|
@Guardiola31337 can the |
|
@Guardiola31337 @danesfeder I've noticed one more issue when overriding the |
|
@Guardiola31337 do we have an update on this? Outstanding Issues I've Found;
|
|
@Guardiola31337 can we close this / write up a follow-up ticket for the |
I thought you get this working per comment #1233 (comment)
How are you overwriting I'm going to reopen #1224 and track the missing Thanks @Danny-James for your thoroughly testing here 🙏 Really appreciate it! |
|
@Override
public BannerInstructions willDisplay(BannerInstructions instructions) {
if (instructions.primary().text().contains("arrive") && points.size() > 1){
Log.d(TAG, "Override Banner Text");
Log.d(TAG, "OVERRIDE TEXT: " + instructions.primary().text() + " with Continue along route");
List<BannerComponents> bannerComponents = new ArrayList<>();
bannerComponents.add(instructions.primary().components().get(0).toBuilder().text("Continue along route").build());
BannerInstructions bannerToBeDisplayed = instructions.toBuilder().primary(instructions.primary().toBuilder()
.components(bannerComponents).build()).build();
return bannerToBeDisplayed;
}
return instructions;
} |
Guardiola31337
deleted the
pg-1224-intercept-voice-banner-instructions
branch
Fixes #1224 and #1107 (comment)
Right now, it's pretty complicated to override
SpeechAnnouncementandBannerInstructionsobjects without knowing their internals and clients ofwillVoiceandwillDisplayAPIs are having a hard time to get them working. This PR simplifieswillVoiceandwillDisplayAPIs so they are user-friendly and work as expected.Ultimately, I think that what clients would want from them is just to voice / display the announcement / banner text they want.
I added the
DO NOT MERGElabel (for now) because I'd love to discuss if the changes introduced make sense / will benefit clients. What do you think @danesfeder @devotaaabel? I'd love to hear your thoughts 🙏cc'ing @akitchen to know:
Refs. #1074