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

[Accepted with Revisions] SDL 0259 - Disabled Softbuttons #859

Closed
theresalech opened this issue Nov 6, 2019 · 19 comments
Closed

[Accepted with Revisions] SDL 0259 - Disabled Softbuttons #859

theresalech opened this issue Nov 6, 2019 · 19 comments

Comments

@theresalech
Copy link
Contributor

theresalech commented Nov 6, 2019

Hello SDL community,

The review of the revised "SDL 0259 - Disabled Softbuttons" proposal begins now and runs through January 14, 2020. The previous review of this proposal took place November 6 - November 12, 2019. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0259-DisabledSoftbuttons.md

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

#859

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:

  • Is the problem being addressed significant enough to warrant a change to SDL?
  • Does this proposal fit well with the feel and direction of SDL?
  • If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    Please state explicitly whether you believe that the proposal should be accepted into SDL.

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
[email protected]

@joeljfischer
Copy link
Contributor

I think this proposal idea generally is good, however I have a few minor notes, and some larger concerns.

Notes

1. The introduction says:

Also this allows an app to subscribe to softbuttons that are disabled.

This should be reworded to "This also allows an app to subscribe to and disable SDL predefined buttons."

2. The motivation is unclear to those not familiar with the iHeart application.

3. I would change disabledButtonPossible to supportsDisabled. The current language isn't used anywhere else that I can find.

4. In the proposed solution is this sentence:

Alternatively we could have the button not send any button events to match what Android and iOS do when a button is on screen but disabled.

Alternatives should be placed within the "Alternatives Considered" section.

Larger Concerns

5. This proposal doesn't cover app library manager API changes.

6. The impact on existing code section is really more of a "downside" section, and it's relatively serious. We generally don't currently have APIs where if you set them but are on an older head unit, the opposite of the intended effect will take place (e.g. a developer sets isEnabled = false, connects to RPC 6.0 and the button is enabled). We try to avoid having a developer need to check version numbers whenever possible.

I think that we should consider a manager-level fallback for disabled buttons where they will automatically not be sent on older head units. It should probably also have a configuration for turning that behavior off if desired.

@MichaelCrimando
Copy link
Contributor

Hey @joeljfischer
1-4 sounds fair enough, those are easy enough to change
5. Who's a good contact for app library manager API changes? I can ask around here but nobody in my immediate area seems too knowledgeable on them
6. I can agree with your propsal improvement there

@joeljfischer
Copy link
Contributor

I can't help you too much with app library manager API change contacts within your company. I can try to come up with a suggestion on the iOS-side, but I don't have the knowledge to help too much on the Java side.

@theresalech theresalech changed the title [In Review] SDL 0259 - Disabled Softbuttons [Returned for Revisions] SDL 0259 - Disabled Softbuttons Nov 13, 2019
@theresalech
Copy link
Contributor Author

The Steering Committee voted to return this proposal for revisions. The author will revise the proposal based on the Project Maintainer's feedback in this comment.

@smartdevicelink smartdevicelink locked and limited conversation to collaborators Nov 13, 2019
@joeljfischer
Copy link
Contributor

joeljfischer commented Nov 14, 2019

Here's my suggestion for iOS manager-level additions:

1. Add a new soft button configuration to the screen manager:

@property (strong, nonatomic, readonly) SDLSoftButtonConfiguration *softButtonConfiguration;

2. This will involve creating the soft button configuration something like this:

@interface SDLSoftButtonConfiguration: NSObject

/// If true, the soft button manager will not send soft button objects with isEnabled=false if on head units that don't support disabled soft buttons. If false, the soft buttons will be sent anyway but will appear enabled on the head unit. Defaults to true.
@property (assign, nonatomic) BOOL enableDisabledButtonFallback;

@end

3. The disabled property will have to be added to SDLSoftButtonState:

@property (assign, nonatomic, getter=isDisabled) BOOL disabled;

The soft button manager will then have to take both the configuration value and the disabled BOOL on the state into account to determine if the button will be sent or not and with what values.

@theresalech theresalech changed the title [Returned for Revisions] SDL 0259 - Disabled Softbuttons [In Review] SDL 0259 - Disabled Softbuttons Dec 18, 2019
@smartdevicelink smartdevicelink unlocked this conversation Dec 18, 2019
@theresalech
Copy link
Contributor Author

The author has revised this proposal to incorporate the requested revisions from the Steering Committee. The revised proposal is now in review and will be voted upon during the 2020-01-14 meeting.

@joeljfischer
Copy link
Contributor

  1. ButtonCapabilities parameters are described as this:
<struct name="ButtonCapabilities">
    .
    .
    .
    <param name="disabledButtonPossible" type="Boolean" mandatory="true">
        <description>The button supports being disabled. If the button is set to disabled, it will still show on the HMI but appear grayed out.
        </description>
    </param>
</struct>

But it should be:

<struct name="ButtonCapabilities">
    .
    .
    .
    <param name="supportsDisabled" type="Boolean" mandatory="false">
        <description>The button supports being disabled. If the button is set to disabled, it will still show on the HMI but appear grayed out.
        </description>
    </param>
</struct>

It should be non-mandatory and a different name to match the SoftButtonCapabilities name.

  1. However, after looking at the parameter name, I think that a better name that matches with the capabilities structs would be disabledAvailable.

@joeygrover
Copy link
Member

1. Agree with Joel
2. Agree with Joel
3. A general question/concern I have is, that with these buttons being "disabled" is there any indication to the user as to why they are? I'm worried without some ability to inform the user this might lead to frustration. Maybe adding some text for a tool tip like dialog, or TTS command to inform the user why the disable is occuring would be a good idea. I am looking for input here, not stating it is a requirement.
4. I would recommend changing the param name isDisabled to simply disabled.
5. The manager layer public methods should be isDisabled not getDisabled or getIsDisabled for Java Suite.
6. Why are we adding a new config for soft buttons, SoftButtonConfiguration? Couldn't the desired effect be achieved by adding this option to the SoftButtonState? It seems to be that it makes the most sense there as it is simply a state that the soft button can transition to.

@Shohei-Kawano
Copy link
Contributor

What happens if the disabling of the button by driver distraction and the disabling of the button by the request from the application overlap?
Of course I think the button will be disabled, but will it be called back? Will not be?

@joeljfischer
Copy link
Contributor

3. The button is still capable of receiving button presses, so a good practice for the app developer would be to show an alert saying why the button won't work in the current situation.
4. I agree with this.
6. This is simply an on / off switch for easier behavior so that they can use the managers with disabled buttons, then choose what they want to have happen to them on systems that don't support disabled buttons. That's easier than choosing fallback behavior per state. If they want to turn that on / off individually, they would - based on the current proposal - not send the individual buttons they don't want to send when they check themselves if disabled is supported.

@Shohei-Kawano I'm going to number your questions.
7. Soft buttons probably should not be disabled by driver distraction on the HMI. That would be a very odd, and very poor, UX. However, if an OEM did want that to be the case, it would depend on the OEM and the HMI. For example, if the HMI wanted to disable the buttons so that no button presses went through, they would prioritize that.

8. The proposal states that the button will be called back. The isDisabled parameter only affects appearance.

@ghost
Copy link

ghost commented Jan 14, 2020

  1. The wording for the param in SoftButtonCapabilities should rather be disabledSupported or disabledStateSupported which follows existing params imageSupported and textSupported. I agree this param should be non-mandatory.

  2. The wording for the param in ButtonCapabilities should be disabledAvailable to match existing params but disabledSupported to match above param could make more sense.

  3. I'm worried with still returning the button notifications to the app developer. It'll be the developers burden to check the button state (if is disabled) before performing an action. I see this to be a common pitfall for app bugs. ... From an rpc_spec perspective, returning RPC notifications could make sense. By default the button manager should drop the notifications for disabled buttons but allow app developers to enforce forwarding those notifications even for disabled buttons.

  4. To be clear. The getter should be isDisabled and the setter setDisabled ?

public boolean isDisabled() { ... }
public void setDisabled(bool disabled) { ... }
@property (assign, nonatomic, getter=isDisabled) BOOL disabled;
  1. I understand the configuration allowing to choose between sending or not sending disabled buttons on head units not supporting. However, I disagree adding the configuration because it acts as a global on/off switch although the capability for disabled state is button specific. May be I'm mistaken but I believe we would need a ButtonConfiguration as well handling hard buttons. Instead I think adding this setting to the button classes allow more flexibility. Downside is that the developer need to set the configuration to every button...

  2. I think the question is "what if the button performs a DD sensitive action like keyboard or scrollable message?". In this case disabling a button or notifying the user on a button press when in motion makes sense and it's definitely not odd or very poor UX.

@joeljfischer
Copy link
Contributor

1/2. I'm okay with disabledSupported as well. It's probably a slightly better name.

3. Personally, I don't see this as a big issue. Plus, I think that it is important that the disabled button does something when pressed; otherwise, the user will be confused. Your alternative of having the soft button manager drop notifications by default is interesting, but I think it's unnecessary.

6. I think that in the majority of cases, the developer will want the fallback behavior to happen on all their disabled buttons, and not on some but not others. If they do want that, they can create that custom behavior without too much effort.

May be I'm mistaken but I believe we would need a ButtonConfiguration as well handling hard buttons.

There is no manager APIs for hard buttons, so that is not currently possible.

Instead I think adding this setting to the button classes allow more flexibility. Downside is that the developer need to set the configuration to every button...

That's the problem. For the extra flexibility that, in my opinion, few developers will need, we're creating a bunch of extra work for them. IMO, if a single on/off switch covers 90%+ of devs and the custom behavior can be documented and isn't difficult, that works well.

It sounds like @kshala-ford and @joeygrover disagree. The alternative API would be to add a property to SoftButtonObject called disabledButtonFallback. It would default to a value and the dev would have to change it on every SoftButtonObject that they wanted to have different behavior.

7. That wasn't the question, the question was what if the HMI disables the button and the app disables the button, and I tried to answer that question. As far as your revision of the question goes, you're correct, but that's not currently possible on the HMI-side unless SDL-0250 is accepted. As SDL currently stands, it certainly is odd and poor UX for the HMI to disable soft buttons when the vehicle is in motion.

@MichaelCrimando
Copy link
Contributor

MichaelCrimando commented Jan 14, 2020

Author here, just trying to follow up to make sure I understand everything.
First of all, thanks for organizing the points numberically and keeping them consistent so we can track what everyone is talking about.

  1. Change param to disabledSupported when proposal accepted
  2. Change param to disabledSupported when proposal accepted
  3. Agreed with @joeljfischer on this one. It'll be a very minor burden on the app to track if disabled or not. Plus the button notification lets the app do what it wants when tapped. Maybe during live radio, iHeartRadio doesn't want to do anything at all when someone hits seek left or seek right. Maybe they want to do a speak RPC. The app can choose
  4. Are we actually getting too in depth on code for this one? Either way, it looks like @kshala-ford summarized the change if we need it nicely.
public boolean isDisabled() { ... }
public void setDisabled(bool disabled) { ... }
@property (assign, nonatomic, getter=isDisabled) BOOL disabled;
  1. Answered with code in point Stringly Typed Enums #4
  2. Seems like a discussion point for today's call. I personally don't see why some buttons would support the disabled state but not others. iHeartRadio has been asking for us for this proposal for a while so we're meeting that use case and this meets their needs.
  3. Great question - First, SDL-0250 would have to implemented for this to actually happen.

Now I'm trying to picture a specific use case where this would happen. Maybe an "info" button for a song that brings up a scrollable message. In the normal state, the app would send a show RPC and include the "info" button with the nextFunctionInfo" set to a scrollableMessage`.

Now consider the case where the the app can't get info for the current song. In order for the app to disable the info button, it would have to send a new show RPC and include the "info" button with a disabled = true. In the same RPC it would set the nextFunctionInfo to default. default wouldn't be locked out while driving so the clash wouldn't ever happen. I'm not sure if we need to upgrade app expectations with this info somewhere though

@Shohei-Kawano
Copy link
Contributor

  1. Depending on the JP OEM, the button on Template may be disabled by DD.
    It may seem strange, but I think it is necessary to confirm the facts.
    OEMs have different DD policies.

@MichaelCrimando
Copy link
Contributor

@Shohei-Kawano -san
We did bring this up at the SDL call but nobody mentioned that they'd have issues with it. Do you know of something in particular? I work for Ford and feel like we're pretty restrictive on DD, but we don't have any softbuttons locked out while driving for any of the apps templates.

@theresalech theresalech changed the title [In Review] SDL 0259 - Disabled Softbuttons [Accepted with Revisions] SDL 0259 - Disabled Softbuttons Jan 15, 2020
@theresalech
Copy link
Contributor Author

The Steering Committee voted to accept this proposal with the following revisions:

  • Use disabledSupported in place of disabledButtonPossible param within ButtonCapabilites and make parameter mandatory="false".
  • Use disabledSupported in place of disabledAvailable param within SoftButtonCapabilites param.
  • Change param isDisabled to the following:
public boolean isDisabled() { ... }
public void setDisabled(bool disabled) { ... }
@property (assign, nonatomic, getter=isDisabled) BOOL disabled;
  • Specify in the proposal that once SDL-0250 is implemented, if there is a case where both the HMI and app attempt to disable a button, the HMI disabling will override the app's.

@theresalech
Copy link
Contributor Author

@MichaelCrimando Please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in the respective repositories for implementation. Thanks!

@theresalech
Copy link
Contributor Author

@Shohei-Kawano as mentioned during our call, we will need to close and lock conversation on this issue. However, if you have any follow up questions, please feel free to raise them on the SDL Slack Team, or during an upcoming Steering Committee meeting. Thank you!

@smartdevicelink smartdevicelink locked as resolved and limited conversation to collaborators Jan 15, 2020
@theresalech
Copy link
Contributor Author

theresalech commented Jan 29, 2020

Proposal has been updated to reflect agreed upon revisions and issues have been entered:

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants