[Accepted] SDL 0054 - ChangeRegistration-Manager

theresalech commented 7 years ago

Hello SDL community,

The review of the revised proposal "ChangeRegistration-Manager" begins now and runs through May 16, 2017. The original review of "ChangeRegistration-Manager" occurred April 25 through May 2, 2017. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0054-change-registration-manager.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/164

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 theresa@livio.io

brandon-salahat-tm commented 7 years ago

@kshala-ford Could you explain the use-case for the BOOL return type? Is this just to signify whether the class conforming to the delegate was able to successfully modify the configuration file?

kshala-ford commented 7 years ago

@Toyota-BSalahat: This is important in case the app got connected to a HU which is configured to an unsupported language. The BOOL return type gives the app a chance to tell the SDK not to change the registration. Only if the app has implemented the method and the app returns YES the SDK will change registration.

joeljfischer commented 7 years ago

I have a few comments about this proposal:

Would it be easier / simpler for a developer to provide a dictionary of app names, etc. of the format [languageName: value] and allow the SDK to automatically handle this?
I'm not a fan of the current delegate method, and it wouldn't even work as currently written (the configuration parameter isn't written as an inout), unless you're relying on pass by reference (and I believe its copied currently). Additionally, the method as written is confusing, as it says fromLanguage:configuration:, but it's not "from language and configuration" which is the implication. I think something like: - (nullable SDLLifecycleConfiguration *)manager:(SDLManager *)manager willChangeRegistrationToLanguage:(SDLLanguage *)language currentConfiguration:(SDLLifecycleConfiguration *)configuration would work better. Note that the return parameter is nullable, and would therefore simply be nil if the developer doesn't want to change the configuration.

Finally, just to note, this would be a minor version change. I support this idea in general, but I think we should consider the above notes, and cannot support the proposal as is.

kshala-ford commented 7 years ago

@joeljfischer

Would it be easier / simpler for a developer to provide a dictionary of app names, etc. of the format [languageName: value] and allow the SDK to automatically handle this?

I thought about that, too. It would require more changes to the configuration than the currently proposed solution and it has impact to apps that are already using the new API. Do you have an idea how to provide this data to the app lifecycle?

I'm not a fan of the current delegate method, and it wouldn't even work as currently written (the configuration parameter isn't written as an inout), unless you're relying on pass by reference (and I believe its copied currently).

It will work. There are various ways to pass the data to the lifecycle manager.

Additionally, the method as written is confusing, as it says fromLanguage:configuration:, but it's not "from language and configuration" which is the implication. I think something like:

I cannot find fromLanguage:configuration in the proposal. Can you please mention the line or the file you're looking at?

- (nullable SDLLifecycleConfiguration *)manager:(SDLManager *)manager willChangeRegistrationToLanguage:(SDLLanguage *)language currentConfiguration:(SDLLifecycleConfiguration *)configuration would work better. Note that the return parameter is nullable, and would therefore simply be nil if the developer doesn't want to change the configuration.

I don't think it would work better but different. This would motivate app developers to reuse and return the configuration instance provided.

Changing the registration requires a change of only a few parameters. Doing those changes on the original configuration would be benefical for the lifecycle manager. The next time the app connected to SDL it would automatically match the language.

joeljfischer commented 7 years ago

Would it be easier / simpler for a developer to provide a dictionary of app names, etc. of the format [languageName: value] and allow the SDK to automatically handle this?

I thought about that, too. It would require more changes to the configuration than the currently proposed solution and it has impact to apps that are already using the new API. Do you have an idea how to provide this data to the app lifecycle?

It would need to be a breaking change or a deprecate & replace situation.

I'm not a fan of the current delegate method, and it wouldn't even work as currently written (the configuration parameter isn't written as an inout), unless you're relying on pass by reference (and I believe its copied currently).

It will work. There are various ways to pass the data to the lifecycle manager.

This is non-standard to not explicitly pass an inout or to return what's required. No apple API I have ever seen operates this way.

Additionally, the method as written is confusing, as it says fromLanguage:configuration:, but it's not "from language and configuration" which is the implication. I think something like:

I cannot find fromLanguage:configuration in the proposal. Can you please mention the line or the file you're looking at?

You have the method name as - (BOOL)manager:(SDLManager *)manager willChangeRegistrationToLanguage:(SDLLanguage *)language configuration:(SDLLifecycleConfiguration *)configuration;, I shortened it, and accidentally changed the "to" to a "from". toLanguage:configuration: has the same problem in that it grammatically implies something other than what is actually the case.

- (nullable SDLLifecycleConfiguration *)manager:(SDLManager *)manager willChangeRegistrationToLanguage:(SDLLanguage *)language currentConfiguration:(SDLLifecycleConfiguration *)configuration would work better. Note that the return parameter is nullable, and would therefore simply be nil if the developer doesn't want to change the configuration.

I don't think it would work better but different. This would motivate app developers to reuse and return the configuration instance provided.

That's why I like the approach I gave better. It gives them a configuration to alter and explicitly return. It makes much more sense to me.

Changing the registration requires a change of only a few parameters. Doing those changes on the original configuration would be benefical for the lifecycle manager. The next time the app connected to SDL it would automatically match the language.

This approach would do the same thing. The manager would copy its current configuration and pass it to the delegate, then, if the developer returns one, sets it as the new configuration and runs ChangeRegistration. That way on reconnection, it will be the current configuration.

kshala-ford commented 7 years ago

You have the method name as - (BOOL)manager:(SDLManager )manager willChangeRegistrationToLanguage:(SDLLanguage )language configuration:(SDLLifecycleConfiguration *)configuration;, I shortened it, and accidentally changed the "to" to a "from". toLanguage:configuration: has the same problem in that it grammatically implies something other than what is actually the case.

No it's still correct because the API would change the registration to that specified language and to that configuration.

joeljfischer commented 7 years ago

No it's still correct because the API would change the registration to that specified language and to that configuration.

No, that is a parameter being passed into the method, not back out. So it can't semantically mean that it's changing to it.

brandon-salahat-tm commented 7 years ago

@kshala-ford Thank you for the clarification. I agree with @joeljfischer that it might be better to return a nullable object instead of the current setup. I think this would provide a more defined interface.

If there is a standard set of changes an app is expected to make during this delegate call to the configuration, it might be even better to define a class that represents those changes, and have the delegate simply ask for that object as its return type. Something along the lines of (pseudo code): class LanguageConfigurationUpdate { property languageName etc }

In my experience in-out parameters in Objective C can cause sneaky problems. It is best to pass a copy in and return something separate.

joeljfischer commented 7 years ago

@Toyota-BSalahat That's a pretty good idea. That makes it even more defined what can / should be updated.

kshala-ford commented 7 years ago

@Toyota-BSalahat @joeljfischer In the first place I wanted to keep the amount of changes as small as possible but the new class sounds more elegant.

It would contain all parameters of ChangeRegistration except the language parameters (they can be set internally by the manager). The lifecycle manager should send a ChangeRegistration request and update the existing lifecycle configuraiton (that's why I came up with the class name SDLLifecycleConfigurationUpdate) if the app developer returns an instance of that class. Below you'll find how that class would look like. I've renamed the method so it sounds closer to what it would actually do... Please let me know if there's anything unclear or not as expected.

@interface SDLLifecycleConfigurationUpdate
@property (copy, nonatomic, nullable) NSString *appName;
@property (copy, nonatomic, nullable) NSString *shortAppName;
@property (copy, nonatomic, nullable) NSArray<SDLTTSChunk *> *ttsName;
@property (copy, nonatomic, nullable) NSArray<NSString *> *voiceRecognitionCommandNames;
@end

@protocol SDLManagerDelegate
...
@optional
- (nullable SDLLifecycleConfigurationUpdate *)manager:(nonnull SDLManager *)manager willUpdateLifecycleToLanguage:(nonnull SDLLanguage *)language
@end

theresalech commented 7 years ago

The Steering Committee has requested that this proposal be revised to reflect the changes in the last comment from @kshala-ford. Once these revisions are complete, the proposal will be brought to another vote by the Steering Committee.

theresalech commented 7 years ago

The review of the revised proposal "ChangeRegistration-Manager" begins now and runs through May 16, 2017.

joeljfischer commented 7 years ago

As a note, this is a minor version change.

Harshacharan commented 7 years ago

@Kujtim Shala Hello! Currently the app provides (or can provide) the list of languages it supports. This is done by using the lifecycle configuration. Any app parameter that has to be localized (TTS name, VR names) is localized to the app's default language. The app still has to check manually if it should change the registration and eventually modify the localizable app parameters using the ready handler Could you eloborate on the manual check performed by app for changing the registration and modifying localizable parameters?

kshala-ford commented 7 years ago

@Harshacharan

First the app has to properly set .language and .languagesSupported on the `SDLLifecycleConfiguration object.
After creating a manager using the configuration the app would startWithReadyHandler:
Once the SDK has called the provided block the app has to manually find the language replied in self.manager.registerResponse.language in the list of supported languages.
If it's supported the app has to manually change registration which would override the lifecycle-configuration.

theresalech commented 7 years ago

After the edits made to this proposal, the Steering Committee has voted to accept.

theresalech commented 7 years ago

Issue entered: [SDL 0054] ChangeRegistration-Manager

smartdevicelink / sdl_evolution

[Accepted] SDL 0054 - ChangeRegistration-Manager #164