smartdevicelink / sdl_evolution

Tracking and proposing changes to SDL's public APIs.
https://smartdevicelink.github.io/sdl_evolution/
BSD 3-Clause "New" or "Revised" License
33 stars 122 forks source link

[Withdrawn] SDL 0310 - Perform Interaction Multipick #1052

Closed theresalech closed 4 years ago

theresalech commented 4 years ago

Hello SDL community,

The review of "SDL 0310 - Perform Interaction Multipick" begins now and runs through July 28, 2020. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0310-PerformInteractionMultipick.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

https://github.com/smartdevicelink/sdl_evolution/issues/1052

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you, Theresa Lech

Program Manager - Livio theresa@livio.io

joeljfischer commented 4 years ago
  1. This is missing TextFieldName updates.
  2. I think that choiceIDArray could be replaced with something like multipickChoiceIDs to make it more explicit.
  3. There are currently screen managers for choice sets in the app libraries. Based on this proposal, it looks like this update is not supported by those managers. Is that intentional? 4. The proposal states:

The SDL Mobile libraries will do some handling of providing the choiceID or choiceIDArray response back to the app.

This paragraph doesn't make much sense and is confusing terminology. Are you saying that the app libraries will modify the RPC before the developer receives it? If so, I would strongly urge that this paragraph be removed and updates be provided to the screen manager instead.

  1. What are the driver distraction implications for this. I don't see any way for a head unit to only restrict this to some apps via policies or to enable it only in non-DD situations. Are there guidelines for HMIs that implement this on how to handle DD? Generally a normal perform interaction is modified when DD is on to only show the first few items.

6. Your first (though unnumbered) alternative is:

We could have a layout mode of PerformInteraction for multipick. Though this might be limiting on the types on layouts that could be used.

I prefer this over what you have in the proposed solution. It eliminates weird cases where the layout is incorrect for multipick and could allow the developer to use default head unit language for the confirmation button (though I think that providing that as an optional text for the developer to modify is still a good idea).

MichaelCrimando commented 4 years ago

@joeljfischer

  1. Added TextFieldName in #1057
    
    <enum name="TextFieldName" since="1.0">
     .
     .
     .
        </element>
             <element name="multiPickConfirmationButtonText">
            <description>If this parameter is set, then this is a multipick performInteraction. This text will be the button that a person uses to finish choosing items.</description>
        </element>


2. Changed from `choiceIDArray` to `multipickChoiceIDs` in #1057 
3. I am not 100% sure the question here at least in our test app we more or less manually make a choice sent and perform interaction
4. I am saying that there'd just be a possible response of type single item or array. The SDL Mobile Libraries might have to do SOMETHING with that. Probably just expect a response in the form of a single item array or actual array.
5. As with all DD, it's totally up to the OEM on what they want to do with it. I can't even comment on how Ford would handle it yet as that's a bag of worms by itself.
6. We discussed this internally and our team generally felt the main way I proposed would be overall better. But we can see if anyone else has any opinions
joeljfischer commented 4 years ago

Hi @MichaelCrimando, please include the comments / changes in the comments here instead of a PR. It becomes impossible to track changes in a PR and having them here is better for discussion and visibility.

MichaelCrimando commented 4 years ago

@joeljfischer ok updated the comments to be more detailed on this issue

joeljfischer commented 4 years ago

@MichaelCrimando this is still not what I've asked for. Please post the actual changes that you want to make in your PR here for review. e.g. Post the XML that you're adding for TextFieldName. Linking to a PR is not acceptable because it requires people to go to the PR and find the changes and it doesn't allow for tracking conversation over a point over time in the issue. This is not the forum for reviewing a revisions PR.

MichaelCrimando commented 4 years ago

@joeljfischer ok updated again.

theresalech commented 4 years ago

The Steering Committee voted to keep this proposal in review, to allow for additional discussion on the review issue between the author, PM, and any other SDLC members.

joeljfischer commented 4 years ago

1. 👍 2. 👍 3. Right, but the vast majority of developers probably no longer do that. So if you're asking them to leave the manager system and use raw RPCs, you're making things significantly more difficult for them. Additionally, since the manager system manages ids itself, mixing and matching using raw RPCs and the manager system becomes much more difficult. So any developer using the manager system for choice sets essentially cannot use this feature unless you add manager support in this proposal. 4. Okay, so there would be a response with a single item or an array. I still don't see why the mobile library has to do anything with that. It provides both parameters to the developer like every other RPC. Again, I must ask what concrete things you're trying to say here. 5. I think that restricting the number of items like with single pick isn't going to work with multi-pick as well. I think that having at least one potential option that OEMs can use as a basis for deciding what they want to do would be a good idea. 6. Can I ask why? I provided several reasons why I think the alternative seems superior, could you do the same for your point of view?

MichaelCrimando commented 4 years ago

@joeljfischer

  1. ok let me research it and update you

  2. I think overall im just trying to avoid a breaking change, but we can do whatever is needed

  3. See the trouble with defining DD items in a SDL proposal is that different OEMs have different amounts of tolerance for what they consider distracted driving. Generally speaking (As of like a few years ago) European based OEMs generally leave their infotainment system pretty open because it's less distracting than using a cellphone. Vs US based OEMs tend to be more restrictive because NHTSA has a more strict guideline to follow. Like if I go through Ford legal to see what thy be happy with (which will typically take months btw), there's no guarantee that other OEMs would actually agree with it and then that's a whole point of contention that would come up with this. Plus considering DD also depends on the overall HMI design I don't see any reason why we would need to define DD here.

  4. I'm checking my notes from October of last year so I don't 100% remember. One thing being that what if an app wants to do VR with a layout of MULTIPICK? So we decided to just force it for MANUAL_ONLY mode. But then decided to make it cleaner by making a new RPC

MichaelCrimando commented 4 years ago

@joeljfischer What about something like

[self.sdlManager.screenManager presentMultiPick:<#(nonnull SDLFieldSet *)#> confirmationButtonText:<#(nonnull String)#>];
joeljfischer commented 4 years ago

3. I'm not a big fan of adding a new method here at all. I think this is another potential improvement of the alternative I mentioned in (6). You could add merely add new layouts and an optional string for the button. As the proposal stands, I think you'd just want to add a string to SDLChoiceSet, which if set, becomes a multi-pick. It's rather ugly and not intuitive, but you already know I prefer the alternative. You also would have to update SDLChoiceSetDelegate with a new method that receives an array of choices. I think you could deprecate the existing one that only passes one choice and make it optional. Then if it receives only one option it would pass it as the only item in the array.

4. Could you explain why this would be a breaking change? PerformInteraction.choiceID is non-mandatory, so if it's empty, it won't break anything.

5. Please read my comment again. I'm not asking you to put any requirements in the proposal. My ask is to put one or more potential options. I think that having some sort of potential option as a springboard for OEMs to understand ways they could restrict this for DD is important. You can, of course, refuse to provide that, but I will continue to lodge this objection. For example, one option off the top of my head is for the HMI to show an alert that this option is not allowed while driving.

6. I don't understand this comment, and I think you're confusing InteractionMode and LayoutMode. Your current solution still has the same problem.

theresalech commented 4 years ago

The Steering Committee voted to keep this proposal in review, so that the author can respond to the last comment from the Project Maintainer.

MichaelCrimando commented 4 years ago

@joeljfischer

  1. Kinda dependent on 6
  2. Ahh ok I thought the choiceID was mandatory for some reason. Ok updated that sectino to instead just say this and not mention the libraries handling anything
    On older head units, the app might request a multipick `performInteraction` but the head unit would ignore the parameter of `multiPickConfirmationButtonText` and the response would only end up being a single choiceID.
  3. Ah ok, added a section

Driver distraction

As always, driving restrictions will be up the OEM. One potential suggestion would be to not allow a Multipick Perform Interaction while driving while providing instead a popup that says "Some items may not be available while driving".


6.  Oh sorry , i included some jumps in logic there. if the `LayoutMode` can be "List with Multipick" for example, you have to consider the possible `Interaction` modes `VR_ONLY` and `BOTH`. And even thinking about how something like Alexa handles a voice - there's currently no real good way to do a multipick via voice IMO
joeljfischer commented 4 years ago

4. 👍 5. 👍 6. I understand that, but I don't understand how your current solution is any better. The layout mode will remain normal, the developer will set the multipick button text, and the interaction mode to VR_ONLY. You can reject the RPC in that case, but you could do the same thing if the layout mode is multipick and the interaction mode is VR_ONLY. My point is that I'm not understanding how this situation is improved with your current proposal.

theresalech commented 4 years ago

@MichaelCrimando, I've summarized the agreed upon items, and noted the open items (including two new comments) below. Please provide your thoughts on the open items when possible, thanks!

Agreed upon Revisions

1. Add multiPickConfirmationButtonText to TextFieldName with the following description: "If this parameter is set, then this is a multipick performInteraction. This text will be the button that a person uses to finish choosing items."

2. Change param choiceIDArray to multipickChoiceIDs in PerformInteraction response.

4. Add the following to the Proposed solution:

On older head units, the app might request a multipick performInteraction but the head unit would ignore the parameter of multiPickConfirmationButtonText and the response would only end up being a single choiceID.

and remove the following:

The SDL Mobile libraries will do some handling of providing the choiceID or choiceIDArray response back to the app. On older head units, the app might request a multipick performInteraction but the head unit would ignore the parameter of multiPickConfirmationButtonText and the response would only end up being a single choiceID. If the choiceID and choiceIDArray are both present in the response from the HMI, then Mobile libraries will just send back choiceIDArray.

5. Add the following to the Proposed solution section:

Driver distraction

As always, driving restrictions will be up the OEM. One potential suggestion would be to not allow a Multipick Perform Interaction while driving while providing instead a popup that says "Some items may not be available while driving".

Open Items

3. This is dependent on the author's response to item 6. 6. Please see latest comment from PM here. 7. We think multiPickConfirmationButtonText should be updated to multipickConfirmationButtonText as all other references use a lowercase "P" in "pick". 8. We suggest a rename from multipick to multiselect because "multiple selection" is a more common UI idiom than "multiple pick". If you agree, this would make Item 7 a non-issue.

MichaelCrimando commented 4 years ago

@joeljfischer

  1. Ah yea i see what you're saying, it is kinda the same. I'm wondering if someone else has a second opinin @joeygrover what do you think? Should we do this proposal with a layout mode with regular perform interaction or should we do a new RPC?
  2. Ok updated all cases of "multipick" to "multiselect" (except in like urls and filename just cuz its annoying to rework)
joeygrover commented 4 years ago

6. Looking through all the options I have to agree that the one chosen is probably my least favorite. Adding checks based on parameter availability can lead to a lot of confusion. It would be better if an actual parameter stated that the interaction should be one of multi select. I think the first alternative that Joel seems to agree with is better than the one proposed. Though I do also see shortcomings with having to add additional layout enum values based on each layout. My thinking brought me to yet another possibility, MANUAL_MULTISELECT could be added as an InteractionMode enum value. This would be a clear indication that the interaction should be that of a multiselect and avoid the VR use case. The enum BOTH could have a reworded description to state it is a manual and VR interaction and does not include the multiselect option. We could also give these values new internal name values that better describe their purposes.

theresalech commented 4 years ago

The Steering Committee voted to return this proposal for revisions. The author will revise to include the agreed upon items (1, 2, 4, and 5) in this comment, as well as the following:

theresalech commented 4 years ago

As requested here, the author has withdrawn this proposal.