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

[Accepted with Revisions] SDL 0156 - High level interface: Foundation #448

Closed theresalech closed 6 years ago

theresalech commented 6 years ago

Hello SDL community,

The review of the revised "SDL 0156 - High level interface: Foundation" begins now and runs through May 22, 2018. The original review of "High level interface: Foundation" occurred April 4 - April 10, 2018. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0156-high-level-interface-foundation.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/448

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 6 years ago

Questions / Concerns:

Application Lifecycle

properties to access (appliation related) capabilities (from RegisterAppInterfaceResponse or GetSystemCapabilities)

There will be a SystemCapabilityManager accessible via LifecycleManager in v. 5.3.

- (void)appDidConnect:(SDLApplication *)app; - (void)appDidDisconnect:(SDLApplication *)app;

I would rather see these actually say application instead of app to match iOS. Same for things like didFinishLaunch, which in iOS are didFinishLaunching.

View Controller Lifestyle

I also have some concerns about this and would like some clarifications.

View controllers are instantiated when they are pushed to the view controller manager. In the case of the root view controller it would be instantiated once a connection is established to the head unit (app is registered). In this state there is no view loaded. The app developer should not load data or perform other memory/time intense code at this time.

But the developer would need to instantiate the view controllers in order to pass them to us. Even in iOS UI development, unless a developer is using storyboards and has segues setup the developer must instantiate their own view controllers. I think a more likely flow is for the developer to instantiate their view controller, pass them to us, we call viewDidLoad, and then when the view comes on screen, viewWillAppear / viewDidAppear.

viewWillAppear

The app launched and became (in)active (entered HMI_LIMITED or HMI_FULL). The root view controller will appear.

I don't think the VC is visible in LIMITED.

SDLViewController

SDLViewControllerManager

I'm not sure that a stack is the best way to do this for SDL. SDL doesn't have a common navigation bar / back button, which leaves devs to either implement it themselves, or basically constantly go forward. e.g. imagine a weather app that has 3 screens, now, daily, hourly, each accessible via menu command. The user goes from now -> daily -> now -> hourly -> daily -> now. Would the developer have those all push forward, or do a forward, back, forward, forward, forward?

I think instead, that an array, or more semantically accurate for what I'm thinking, a set of view controllers (or perhaps even a set of arrays with references to view controllers? That might be too complicated) which describe the different screens and allow the developer to easily switch between them (the data they have may also be able to be cached and re-sent?) so that a developer isn't initializing VCs within other VCs and having them show the same data. So, instead of the current API, a dev would call something like - (BOOL)showViewControllerWithName:(NSString *)name and if that name is known to the VCManager, it would present that VC with the cached data (viewDidAppear only). If it's the first time the VCManager has instantiated it, viewDidLoad will be called. If that model was used, the dev would have to pass VCs using something like addViewController (or setting an array of VCs) which the dev could later reference.

Finally, this proposal doesn't speak of caching VC data and resending it, which would likely be expected (a dev thinking that this would mirror iOS would probably expect data set in viewDidLoad to still be there after a viewDidAppear).

ModalViewControllers

kshala-ford commented 6 years ago

Thank you @joeljfischer for the detailed review. Let me try to provide some explanation/answers. Please let me know if I missed someting.

Application lifecycle

I wrote that the application lifecycle could be proposed separately because this section isn't specified to the end when writing this proposal. Anyway I wanted to provide as much information as possible for understanding. I'm more than happy for any input. The question is do we want everything in a single proposal or separately? I'm OK to expand this proposal and include the app lifecycle.

How does this relate to the existing lifecycle manager / other managers. If it matches iOS, UIApplication is a singleton, so does it expose it's lifecycle manager? I know this proposal says the application lifecycle will be expanded in a different proposal, but I think it's pretty necessary to have that information.

The very root of every iOS app is UIApplicationMain() creating instances of UIApplication and an implementation of UIApplicationDelegate.

I thought of adding SDLApplicationMain and making SDLApplication a singleton. This way the app developer could add the single line SDLApplicationMain(nil, nil); into the main.m file in order to support SDL. This single line would mean "Start SDL with default SDLApplication and no specific app delegate" and under the hood the library could register as "Hello SDL".

In order to configure the app I had the idea of a .plist file similar to Info.plist... never spent time to think through it... An alternative would be a method in SDLApplicationDelegate asking for app configurations (name, id etc.) or a lifecycle configuration... SDLApplication could expose the lifecycle manager.

Again I never thought through it. I'm open to suggestions.

There will be a SystemCapabilityManager accessible via LifecycleManager in v. 5.3.

Thanks for #264. I'll take a look at it and make sure it's used within SDLApplication.

I would rather see these actually say application instead of app to match iOS. Same for things like didFinishLaunch, which in iOS are didFinishLaunching.

Sure. There's no intention to use App instead of Application.

Background -> Limited transition says appDidBecomeInactive, but that doesn't make much sense to me. Should it be active? Though can an app actually switch from Background -> Limited? They're kind of alternatives to each other, are they not?

I struggle with ...Inactive in general but I continued using it to match the native API... Inactive and LIMITED don't really match... an app in HMI_LIMITED is still ... active ... media apps still play music and react on seek buttons... nav apps still navigate and provide TBT... What about calling it appDidBecomeLimited? I think this wording makes sense in both transitions (full->limited or bg->limited).

The transition from BACKGROUND to LIMITED is not impossible. Every transition is possible. I can think of an RC app restarting a media app which was started before but without moving the media app to foreground. The transition table covers every possible transition between HMI levels to not be in trouble for future use cases.

I also assume that if you disconnect, you also get the appDidClose notification.

I had the same thought but then appDidClose could be called although the app is not connected anymore. I actually wanted to avoid that because I don't know if the app tries to do something which is allowed in HMI_NONE...

View controller lifecycle

Why do we need both loadView and viewDidLoad? Most developers don't use loadView and it doesn't make too much sense in our case (I personally, have never used loadView, I just use viewDidLoad). I also think that it simplifies how we expect devs to use the library. ...

See UIViewController.loadView. We don't have the capabilities for an interface builder. Therefore the proposal follows the same direction as UIKit to create views programmatically.

But the developer would need to instantiate the view controllers in order to pass them to us. Even in iOS UI development, unless a developer is using storyboards and has segues setup the developer must instantiate their own view controllers. I think a more likely flow is for the developer to instantiate their view controller, pass them to us, we call viewDidLoad, and then when the view comes on screen, viewWillAppear / viewDidAppear.

Actually this is not true. View controllers need to be created and passed to e.g. the navigation controller but the views are not loaded at this point. Your proposal loads the views in a very early phase where the VC doesn't contain any data. Anyway the developer needs to reset the views after the data was set... I don't think this is a good idea. Furthermore we cannot predict what views are necessary and the app developer might want to have references to each subview.

View controller lifecycle is not trivial. Therefore I propose to follow UIKit and keep loadView and tell the developer to override it. The learning curve will be short. The developer will understand the SDL view controller lifecycle in no time. Changing the lifecycle is unecessary work for us to document and wasted time for the developer to understand.

How does this work with modal VCs (e.g. alerts)? What do we sent when HMI_OBSCURED is active?

This is a good question. The library should still send the Alert but if it's rejected by the HMI the completion handler should send back an NSError. Alert view controllers could also send Show RPCs before sending the Alert if the app developer sets a view to the controller.

I don't think the VC is visible in LIMITED.

Actually it is. On SYNC3 media apps are visible on the home screen.

SDLViewController

What is the difference between showViewController and the VCManager's pushViewController? Same question for a VC's presentModal and the VCManager's presentModal?

I was following the method names from UIKit. SDLViewController.showViewController should redirect to SDLViewControllerManager.pushViewController similar to show UIViewController.showViewController redirects to UINavigationController.pushViewController. See here. Same redirect with presentModal.

I just realize that showViewController behaves differently depending if the app uses navigation or split view controller as the root.

I would like to help the developer with the layout. I know we need the basic string, but 99.99% of the time the dev wants one of the enums. It would be much better to have a way for them to use the enum directly, especially in swift.

Are you talking about template names for display layout? I think we can have two properties for this case. The first property ... let's call it layout of (string) enum type SDLDefaultDisplayLayouts. The second property of type string called customLayout could override the layout property if set. The SC should agree to a set of default layouts head units should support. Agree?

How do we detect and alert the developer if they set incompatible subviews

Incompatible views like the image view? Within the library we can avoid using such views if graphics are not supported. Not sure if we can detect if a template uses primary and secondary graphic. We could use #344 for soft buttons.

I wouldn't let the views fail. Instead we should at least log out the error. May be it makes sense to send back an NSError to the app.

and layout?

In RegisterAppInterfaceResponse there is the parameter templatesAvailable that can be used in order to avoid. NSError could be send to the developer immediately after settings the property... Fallback to DEFAULT can be added to the library.

SDLViewControllerManager

I'm not sure that a stack is the best way to do this for SDL. SDL doesn't have a common navigation bar / back button, which leaves devs to either implement it themselves, ... (Don't want to quote all of it)

This is the reason why I didn't propose an SDLNavigationController. We don't have a navigation bar with a title and back button. My previous proposal included an SDLWindow with a .rootViewController so the app developer can decide if a stack is desired using SDLNavigationController as the root or a simple concept with replacing the window's root ...

I'm worried about the idea of an array holding view controller instances with regards to memory. There's a good reason why UIKit deallocates whole VC objects if not used anymore. In iOS background apps are killed quickly if memory is needed. This doesn't necessarily mean that the current app is killed. Other apps could be killed that the user is not currently using. This means over time the driver will end up with less and less apps. It's impossible for us to control the VC data loaded unless we deallocate VC objects expecting data was strongly referenced within the VC object. Furthermore it's very different to how UIKit works and I also believe many VC instances depend on current conditions and should therefore be created at the time when needed.

Technically the app developer can manipulate the stack.

now -> daily -> now -> hourly -> daily -> now. Would the developer have those all push forward, or do a forward, back, forward, forward, forward?

now would be the root view controller. So every transition to it would be popToRootViewController (not proposed but can be added). Transition from hourly to daily can be made by replacing the top view controller. This is how I would do it if the proposal would be accepted this way.

I get your point. a hard coded stack might not be the best idea. May be we should add showViewController to the manager and mode with options like

The SDLViewController.showViewController would take this option into account.

ModalViewControllers

I worry a bit about the base class in case we have a modal-like in the future without either of those properties.

Naturaly every modal view controller should time out and disappear as they are transient overlays. It's not specified anywhere but it's highly recommended that overlays have a text based message. Every overlay we have today includes a text based message. I cannot think of a case without a message.

I also think it would be better on the subclasses in order to make the name on each of them perhaps clearer, e.g. you're missing two alert lines because the message is on the base class.

I think this wasn't clear in the proposal. The .message should support multiline with newline chars. I'm not sure if we can calculate the possible line length for text fields today but the app developer can add newline chars in the string.

I would rename the onAudioDataBlock, that's quite out of place with the rest of this API.

It's more of a placeholder to explain how audio data is passed to the app. Any suggestion for naming?

The sliderController is said to provide the value in the modal controller. I'm not sure that's the best way to do it because with iOS, the slider's value is updated as the user moves it and the dev can KVO on it. We don't have any of those advantages. I think it's better to have a block on it.

UISlider is a view but SDLSlider is an overlay. They behave very differntly. The mobile API doesn't provide the value when changed but when saved. KVO or block wouldn't work with the today's API but it's a good idea for a proposal to add OnSliderInput or something...

I think the keyboard should be broken out into its own thing at least, or taken out entirely. The only use case is navigation, which at least in the current iteration, is unsupported by this framework.

I don't think we can say "The only use case is navigation". There are other apps for POI or media that use keyboards in combination with choices for suggestions or history. As there are interaction layouts for boths (choices and keyboard) we should not separate it.

The alert controller should have a better way of managing soft buttons so that devs don't need to worry about ids and images. I think we can expand the soft button manager to deal with them if they use either the softButtonObject we have now or if they pass a generic soft button without an id.

Absolutely. SDLSoftButton should not be used directly. The alert controller should make use of the new classes added with the show manager. Actually... I just reviewed the show manager's button classes and I see that they don't perfectly fit to the behavior of alert buttons as button states cannot be changed and system context is missing. May be we can take this offline and discuss if the show manager needs to be revised or if alert buttons should be treated separately.

I would also recommend seeing the Choice Manager proposal. I like the delegate pattern in it.

I took a quick glance and it looks pretty good. Will review it in depth and provide feedback. This proposal will probably be deferred anyway until #449 is approved.

This proposal is missing a menu ability, there's a menu manager proposal that may help.

I left out this topic for now. Wanted to focus on the foundation first. I did see your proposal already but I didn't take a closer look.

joeljfischer commented 6 years ago

Application Lifecycle

Regarding UIApplication etc.

I don't know that we should go so far as to have devs alter main.m or create custom info.plist files. We could use a singleton, though we've avoided it up to this point with SDLManager.

What about calling it appDidBecomeLimited?

I think that would be better/clearer.

I also assume that if you disconnect, you also get the appDidClose notification.

I had the same thought but then appDidClose could be called although the app is not connected anymore. I actually wanted to avoid that because I don't know if the app tries to do something which is allowed in HMI_NONE

That's a good point, e.g. if the USB is pulled. I'm just trying to make sure they have the cleanup they need, because if they restart they'll get a transition from NONE again.

View Controller Lifecycle

See UIViewController.loadView. We don't have the capabilities for an interface builder. Therefore the proposal follows the same direction as UIKit to create views programmatically. Your proposal loads the views in a very early phase where the VC doesn't contain any data. Anyway the developer needs to reset the views after the data was set... I don't think this is a good idea. Furthermore we cannot predict what views are necessary and the app developer might want to have references to each subview. View controller lifecycle is not trivial. Therefore I propose to follow UIKit and keep loadView and tell the developer to override it. The learning curve will be short. The developer will understand the SDL view controller lifecycle in no time. Changing the lifecycle is unecessary work for us to document and wasted time for the developer to understand.

I think there's a misunderstanding of my comments. I think we should skip having loadView for 3 reasons:

  1. I don't believe most developers use loadView and that most instead setup their view in viewDidLoad (because they leave a standard UIView as the base view of their view controller, and I think your proposal would move the work that most devs do in viewDidLoad to be kind of required for loadView and this could be confusing.
  2. I think it divides into two overridden methods what could be a single method, and I would prefer the simpler and easier (for developers) option.
  3. I think that our loadView doesn't do what iOS' loadView does and so is not necessary for us. loadView is primarily to allow developers to replace the root view of the heirarchy with something custom. But your scheme doesn't have that. We provide them the view, they build the subviews, and that can be done within viewDidLoad.

Hopefully that helps clarify things.

How does this work with modal VCs (e.g. alerts)? What do we sent when HMI_OBSCURED is active?

I was thinking more about the viewDidAppear / disappear aspects of this.

SDLViewController

I was following the method names from UIKit. SDLViewController.showViewController should redirect to SDLViewControllerManager.pushViewController similar to show UIViewController.showViewController redirects to UINavigationController.pushViewController. See here. Same redirect with presentModal.

There's no modal one on the nav view controller, as a note.

I just realize that showViewController behaves differently depending if the app uses navigation or split view controller as the root.

Yeah...I'm personally leaning toward the desire to make it simpler for developers and just have it on the manager, not on the VCs.

Are you talking about template names for display layout? I think we can have two properties for this case. The first property ... let's call it layout of (string) enum type SDLDefaultDisplayLayouts. The second property of type string called customLayout could override the layout property if set. The SC should agree to a set of default layouts head units should support. Agree?

Yes, I agree with that.

How do we detect and alert the developer if they set incompatible subviews

Incompatible views like the image view? Within the library we can avoid using such views if graphics are not supported. Not sure if we can detect if a template uses primary and secondary graphic. We could use #344 for soft buttons.

I wouldn't let the views fail. Instead we should at least log out the error. May be it makes sense to send back an NSError to the app.

Yeah, there are additional possibilities, such as setting text fields on an ICONS_ONLY template. There's a number of possibilities here. I think we could ignore elements in a template that aren't supported and provide warnings / error logs. Though I think we may also want to provide an error object of some sort so that devs can see their errors from the production field as well as in the development console.

In RegisterAppInterfaceResponse there is the parameter templatesAvailable that can be used in order to avoid. NSError could be send to the developer immediately after settings the property... Fallback to DEFAULT can be added to the library.

Definitely possible, I just wanted to make sure we thought of this case as most of the methods in your proposal don't have error objects.

SDLViewControllerManager

I'm worried about the idea of an array holding view controller instances with regards to memory.

Very valid concern. I don't think we'll be using an especially significant amount of memory, and I think certainly considerably less than a UIKit navigation stack (or tab stack, which is closer to my proposal in concept though not in UI) would. In the worst case, our framework would need to call viewDidLoad again and reload it.

now would be the root view controller. So every transition to it would be popToRootViewController (not proposed but can be added).

I don't think that's how iOS UIKit works...you can correct me if I'm wrong, but I'm pretty sure you can push the VC again. At least, I think the last now -> hourly -> daily -> now wouldn't pop back.

I get your point. a hard coded stack might not be the best idea. May be we should add showViewController to the manager and mode with options like...

If we really do need a stack, I would propose having a push method, and then a replace or show method to replace the root VC of that stack; I would propose that we do that via my proposed solution in the comment above.

Every overlay we have today includes a text based message. I cannot think of a case without a message.

I think you're right that they do, however they may not. I maintain that there should not be a base class with properties here, though I can see leaving timeout. Here are my reasons:

  1. Our iOS UI framework here would constrain future RPCs; future modal RPCs would have to have the timeout and the message.
  2. The "message" naming isn't always the most specific for each individual modal subclass. e.g. "alertText1" would be message, while alertText2/3 (not in your AlertController) would be named differently.

So basically, I'm fine with a base class, I'd prefer with no properties, but a timeout is maybe okay. I'd strongly argue that the message property should be specific to the subclasses.

I think this wasn't clear in the proposal. The .message should support multiline with newline chars. I'm not sure if we can calculate the possible line length for text fields today but the app developer can add newline chars in the string.

While it's an interesting idea, I'm not personally in favor. It would require too much documentation and is too different between the subclasses. I'd be interested to hear others' thoughts here, but I think it's too different from everything else in SDL.

I would rename the onAudioDataBlock, that's quite out of place with the rest of this API.

It's more of a placeholder to explain how audio data is passed to the app. Any suggestion for naming?

Umm...audioDataHandler maybe? Or just handler?

UISlider is a view but SDLSlider is an overlay. They behave very differntly. The mobile API doesn't provide the value when changed but when saved. KVO or block wouldn't work with the today's API but it's a good idea for a proposal to add OnSliderInput or something...

To be clear, I'm proposing adding a handler to this modal VC subclass that provides the input value to the presenting controller, since that's what probably really wants the value and needs to know when it's available. It would just send it when the response arrives. An onSliderInput might be distracting? Maybe that's why it's not there?

I don't think we can say "The only use case is navigation". There are other apps for POI or media that use keyboards in combination with choices for suggestions or history. As there are interaction layouts for boths (choices and keyboard) we should not separate it.

I'm thinking more of the KEYBOARD layout, not the ICON_WITH_SEARCH and LIST_WITH_SEARCH layouts. The KEYBOARD layout isn't available to non-Nav devs, if I'm not mistaken.

Actually... I just reviewed the show manager's button classes and I see that they don't perfectly fit to the behavior of alert buttons as button states cannot be changed and system context is missing. May be we can take this offline and discuss if the show manager needs to be revised or if alert buttons should be treated separately.

Yes, they would have to be single-state soft buttons, or even just use the state object by itself. I'm not sure how system context works into that.

This proposal is missing a menu ability, there's a menu manager proposal that may help. I left out this topic for now. Wanted to focus on the foundation first. I did see your proposal already but I didn't take a closer look.

I'd like to kind of see it as a UITableView(Controller) if possible, I think it may fit well in that context and fit with how devs use similar looking UI. The non-visible voice commands would be different and have to be set on there. So they'd have to subclass a UIMenuViewController or something with that property. I think the menu manager would be fine for a model like that, though there's no built in delegate right now. I'm considering putting in a new proposal to alter the accepted menu manager to use a delegate instead of blocks.

kshala-ford commented 6 years ago

Application lifecycle

I don't know that we should go so far as to have devs alter main.m or create custom info.plist files. We could use a singleton, though we've avoided it up to this point with SDLManager.

I agree to have a shared instance in SDLApplication. SDLApplicationMain() should not be called in the main file but e.g. in applicationDidFinishLaunchingWithOptions:. The main.m is a bad idea as UIApplicationMain() never returns. The main file doesn't need to exist in swift apps anyway. After calling this function from this point the app on the phone waits for an SDL enabled head unit.

Info.plist was just an idea (not saying it's a good one...). Thought it would be nice to have some kind of UI (web or Xcode extension) to edit/export the SdlInfo.plist. I think SDLConfiguration can be used as is.

I struggle with preventing multiple registrations. I think we can provide both without increasing complexity for regular apps. What do you say about the following (but incomplete) design:

/**
 * Creates the shared instance of the SDL application.
 * The function is non-blocking and should be called only once by the app.
 * SDL creates a shared instance of the `SDLApplication` class or subclass when the phone
 * is connected to an SDL enabled device and notifies the delegate when the SDL application 
 * finished launching. The shared instance is deallocated on a disconnect soon after the delegate
 * is notified about the disconnect.
 * @param applicationClass The class of the `SDLApplication` class or subclass.
 * If you specify `nil`, `SDLApplication` is assumed. 
 * @param delegateClass The name of the class from which the application delegate is instantiated.
 * If you specify `applicationClass` to a subclass of `SDLApplication` you may want to implement
 * the delegate in that subclass. In this case `delegateClass` should point to the same class.
 * If you specify `nil`, the application can still track application lifecycle from the notification center.
 * @param configuration The SDL configuration to be used by the shared instance.
 */
void SDLApplicationMain(Class applicationClass, Class delegateClass, SDLConfiguration *configuration);

/**
 * 
 */
@interface SDLApplication

@property (class, nonatomic, readonly) instancetype sharedInstance;
@property (nonatomic, weak) id<SDLApplicationDelegate> delegate;
@property (nonatomic, readonly) SDLApplicationState applicationState; 

@property (nonatomic, strong, readonly) SDLViewControllerManager *viewControllerManager;
// and many more properties
@end

View Controller Lifecycle

I can understand you idea. I still would like to include loadView. It's not required to override the method. The SDLViewController.loadView implementation creates the root view and done. It would follow UIKIt to 100% and still matches to your proposal. At the end the app developer can manage view stuff in loadView and call [super loadView] to not create an own root view. Alternatively viewDidLoad can be used to create subviews like text view, button views etc. Whatever is preferred.

I guess I was missing a few more sentenses in the proposal to clarify. Bottom line: the lifecycle follows UIKit but without the ability of nibs or storyboards.

How does this work with modal VCs (e.g. alerts)? What do we sent when HMI_OBSCURED is active?

I was thinking more about the viewDidAppear / disappear aspects of this.

viewDidDisappear is called for the presenting view controller. The modal/presented view controller's view will appear. This way the app developer can customize the main fields by adding subviews to the modal root view while presenting an e.g. SDLAlertController. This is optional but very helpful for interactions so the app can add a text view with "Loading..." to the interaction controller. The "Loading..." text is shown while the choice sets are being created.

SDLViewController

Yeah...I'm personally leaning toward the desire to make it simpler for developers and just have it on the manager, not on the VCs.

I don't think is simplier. Every time when I want another VC to be shown/pushed I need to call [SDLApplication.sharedApplication.viewControllerManager pushViewController:myNextVC];. Especially if we add a mode to the VC manager I propose to use [self showViewController:myNextVC];.

Yeah, there are additional possibilities, such as setting text fields on an ICONS_ONLY template. There's a number of possibilities here. I think we could ignore elements in a template that aren't supported and provide warnings / error logs. Though I think we may also want to provide an error object of some sort so that devs can see their errors from the production field as well as in the development console.

Do we know the all those details of a template? If so we can make it to ignore. I personally still prefer to send all data regardless if it's not visible in the selected layout. This way we don't need to refresh the head unit when the display layout of the visible VC changes.

Definitely possible, I just wanted to make sure we thought of this case as most of the methods in your proposal don't have error objects.

True. It's missing. I didn't focus on error handling at this stage as I thought it's more a development detail.

SDLViewControllerManager

Very valid concern. I don't think we'll be using an especially significant amount of memory, and I think certainly considerably less than a UIKit navigation stack (or tab stack, which is closer to my proposal in concept though not in UI) would. In the worst case, our framework would need to call viewDidLoad again and reload it.

I'm not worried about memory used by objects of SDL classes. Pure UIKit doesn't consume much memory and our view concept will use less for sure. I'm worried about data retained and stored in VC instances because app developers have to balance between productivity and well designed code. Sometimes a lot of memory is consumed because of a forgotten reference.

If we really do need a stack, I would propose having a push method, and then a replace or show method to replace the root VC of that stack; I would propose that we do that via my proposed solution in the comment above.

Depending on the mode of the VC manager the method SDLViewController.showViewController behaves differently (similar to how UISplitViewController or UINavigationController override UIViewController.showViewController).

ModalViewController timeout and message (multiline) parameter

My goal is to unify all the variants of modal text fields into a single, multiline parameter called .message. There affected text fields are

All of them are very individual parameters. I work for AppLink/SDL for years but I still double checked the param names with the mobile API... .message is so quick and easy to understand and to find by app developers.

I still prefer the base class for .message but alternatively it can be added through a protocol called SDLModalMessage to modalVC subclasses. I would like to hear the opinion of other members.

While it's an interesting idea, I'm not personally in favor. It would require too much documentation and is too different between the subclasses. I'd be interested to hear others' thoughts here, but I think it's too different from everything else in SDL.

I think I don't understand why the proposed message param requires much documentation. Text fields for each line is very uncommon for UIKit and they require documentation and effort by the app developer to split text.

SDLSliderController

Again the slider value is returned only once together with the response when saved. A block provided might be confusing as the app developer could expect immediate value changes. Second there are two blocks that the app developer would take care of. My suggestion was to propose the practice of reading the value property of the modal VC in the completion handler of .presentModalViewController:completion. The block can be added but different to AudioPassThru there is no event during the modal presentation.

I'm thinking more of the KEYBOARD layout, not the ICON_WITH_SEARCH and LIST_WITH_SEARCH layouts. The KEYBOARD layout isn't available to non-Nav devs, if I'm not mistaken.

Keyboards can be used by any app. ...WITH_SEARCH and KEYBOARD are equivalents, too. Interactions should come with keyboard and choice set support.

Yes, they would have to be single-state soft buttons, or even just use the state object by itself.

Isn't that an overkill? This is the reason why I was asking if there are shortcuts for creating single-state buttons.

I'm not sure how system context works into that.

Whoops I mean SystemAction. I don't see that supported in your porposal and I must admit I should have double checked that in the first place... It's very important that KEEP_CONTEXT and STEAL_FOCUS is supported for Alerts.

I'd like to kind of see it as a UITableView(Controller) if possible, I think it may fit well in that context and fit with how devs use similar looking UI. The non-visible voice commands would be different and have to be set on there. So they'd have to subclass a UIMenuViewController or something with that property. I think the menu manager would be fine for a model like that, though there's no built in delegate right now. I'm considering putting in a new proposal to alter the accepted menu manager to use a delegate instead of blocks.

That would be a total separate VC next to the root view controller. UIMenuController inherits NSObject, not UIViewController.... I guess you mean something different?

I still didn't take a look at the menu manager. But I believe a high level interface to the menu can be realized e.g. with a separate VC but it should be proposed separately. Let's not add more things to this proposal. Agree?

To summarize to what we already agreed:

theresalech commented 6 years ago

The Steering Committee voted to defer this proposal, keeping it in review until the next meeting on 2018-04-17, to allow more time for discussion on the issue. It was noted that a high level management proposal will be dependent on this proposal, and that we may want to separate this proposal into smaller ones if further discussion needs to take place on particular components. This will also help to be clear on exactly what's being committed to when this proposal is approved.

joeljfischer commented 6 years ago

@kshala-ford @theresalech I would recommend that we put this proposal on hold for our own sanity. Reviewing both this and #449 takes a large amount of time (and both are iOS proposals), and seeing how this proposal isn't nearly as pressing as #449 is, I would prefer to see it put on hold until #449 is resolved. I imagine that for someone who cannot devote multiple hours to reviewing proposals, having both of these in review is a nightmare, even if they are interested in the outcome.

AndrewRMitchell commented 6 years ago

I also think breaking it up would make it easier to digest and come to a consensus on.

kshala-ford commented 6 years ago

I guess the proposal can be separated into:

More proposal can be added later on e.g. Application menu and voice commands, media player view controller etc.

theresalech commented 6 years ago

The Steering Committee voted to defer this proposal until SDL 0157 is resolved, per joeljfischer's comment. It was also agreed that when this proposal is brought back into review, it may be separated into multiple proposals to allow for more detailed discussions on smaller components of the feature.

theresalech commented 6 years ago

As SDL 0157 has been resolved, the review of this revised proposal begins now and runs through May 22, 2018.

joeljfischer commented 6 years ago

SDLApplication

This should probably provide a reference to its own SDLManager for manually sending RPCs when required, etc. If that's so though, then the fileManager, etc. references aren't really necessary, right?

The only exception should be the SDLPermissionManager as it is very focused on RPC permissions. Instead high level features should monitor the required RPCs internally and provide named properties and notifications similar to CLLocationManager.authorizationStatus and locationManager:didChangeAuthorizationStatus: in order to monitor permissions. For violations the high level features should provide errors using something like locationManager:didFailWithError: and an SDL version of kCLErrorDenied.

I like this in theory, but there are a lot of reasons that various aspects can fail, and the high level manager won't be able to take care of all of them (e.g. RPCs that still have to be sent manually, such as Vehicle Data). Something like Show can even fail when in NONE, but we don't have a manager for that. If we did as I suggested above, and provide a reference to the SDLManager the SDLApplication is using, then it wouldn't be necessary to provide a SDLPermissionManager reference anyway.

SDLApplicationDelegate

Minor notes:

- (void)application:(SDLApplication *)application didEnterAudibleState:(SDLAudioStreamingState)audibleState fromState:(SDLAudioStreamingState)oldState;
- (void)application:(SDLApplication *)application didEnterSystemContext:(SDLSystemContext)systemContext fromContext:(SDLSystemContext)oldContext;

Perhaps these should be split out into multiple methods as this is essentially doing for the HMI states? Or multiple protocols with delegates on SDLApplication? Something like the following for the methods?

- (void)applicationDidBecomeAudible:(SDLApplication *)application;
- (void)applicationDidBecomeAttenuated:(SDLApplication *)application;
- (void)applicationDidBecomeInaudible:(SDLApplication *)application;
- (void)applicationDidEnterMainContext:(SDLApplication *)application;
- (void)applicationDidEnterVoiceRecognitionSession:(SDLApplication *)application;
- (void)applicationDidEnterMenu:(SDLApplication *)application;
- (void)applicationDidBecomeObscured:(SDLApplication *)application;
- (void)applicationDidDisplayAlert:(SDLApplication *)application;

Just throwing that out there as an idea.

SDLApplicationMain

Because (virtually) no developers actually use, much less call UIApplicationMain() – it's implemented for them, I would prefer that this method actually be a traditional method call, something like:

+ (SDLApplication *)initializeSDLWithConfiguration:(SDLConfiguration *)configuration rootViewController:(__kindof SDLViewController *)viewController delegate:(id<SDLApplicationDelegate>)delegate;

An additional note, is that if this is a C function, it's automatic translation to Swift could be odd.

I didn't really follow:

A potential limitation is that the class implementing the delegate must not exist before calling this function and it must specify -init; as the designated initializer.

in reference to the delegate object. Shouldn't they have already created the object and reference is passed here?

AndrewRMitchell commented 6 years ago

To avoid any confusion with the Foundation part of the iOS stack (formerly CoreFoundation), and since its only used in the document name and the first paragraph, can we call this something other then Foundation? (Groundwork maybe?) I realize I may be being a bit pedantic here, but isn't that what you all expect of me by now?

kshala-ford commented 6 years ago

SDLApplication

The long term idea of the managers and this high level interface is to hide/map all the RPCs. The more RPCs we abstract (VehicleDataManager, ChoiceSetManager etc) the less attractive it is for the app developer to use the SDL manager and the permission manager. Each manager should try to notify the app about permission related errors.

Anyway this is not the case today (but hopefully in the near future). I'll add the permission manager to read permisions and the SDL manager (or at least add a send method) to send RPCs. Both could be marked as deprecated once all RPCs are mapped by future proposals (three more high level interface proposals are in the pipeline and more are coming). May be it makes sense to create a paper showing which RPCs (and parameters) are already coverered.

I would keep the other manager references in the application class to avoid the extra level to access e.g. the file manager.

SDLApplicationDelegate

I had the exact same thought of adding methods for each enum value but I decided to propose two methods. I'm open to both ways.

SDLApplicationMain

I'm sorry for the confusion. The way UIApplicationMain works is it creates objects on behalf of the app developer using NSClassFromString. What I'm trying to do is to behave similar to this function. SDLApplicationMain would store the provided class names and the configuration and wait for external accessory to connect before creating the objects. There should not exist any SDL related object (except a config object) while the app is not connected to an SDL head unit.

I like the idea of the function. If follows UIKit and provides the shared SDLApplication. Regarding Swift it would act just as CGRectMake() (worse case it would need NS_SWIFT_NAME). The question is more to what extend do we want to follow UIKit (see alternatives)? My primary proposed function fully matches UIKit behavior. Personally however I'm pro No SDLApplication subclass alternative.

@AndrewRMitchell I'm sorry. I called it foundation because it'll be the ... foundation for the upcoming proposals. This proposal has nothing to do with CoreFoundation.

AndrewRMitchell commented 6 years ago

@kshala-ford CoreFoundation no longer exists, it's now called Foundation. By calling it Foundation, my fear is that when you get to implementation you end up with calling your header file Foundation, and then boom, you have a name space conflict with Apple. Its unfortunate that Apple now use Foundation, but I get it, Foundation is the base for all Apple development.

joeljfischer commented 6 years ago

SDLApplication

Absolutely, ideally we will eventually have high-level managers for all RPCs. However, that is unlikely to happen for at least a year (or significantly more, depending on how long this framework takes to build out and new features are of course constantly added) and some RPCs don't directly fit into a manager framework (like Speak).

I believe we should expose the underlying SDLManager even if, as you note, and I agree with, the goal is to eventually have this UI-like framework handle everything for template-based apps. I don't think we should (yet, even if that is the goal) have a hard distinction between the user using this framework, and having access to SDLManager.

SDLApplicationDelegate

I'm leaning more towards a verbose delegate framework, so long as they're optional. But either way is probably okay.

SDLApplicationMain

Thank you for your explanation, I believe I understand better what you're trying to do. You want to create the delegate and view objects once a connection is established. I understand that desire, but I think we need to refine this to keep it understandable for all developers, including beginners.

  1. I believe that we should not copy UIApplicationMain. It's awkward, almost nobody looks at it, nobody can or should call it (outside of a very small percentage of Swift devs, but they'd have to do so in this proposal), and Apple hid it in Swift (they replaced with an auto-implemented @UIApplicationMain attribute, though a dev can use it if they have a certain special case), so even fewer people would understand what is being copied here.

    a. I believe a traditional class method, object initializer, or singleton call would be better understood by implementers.

  2. I don't believe that having them pass a string representing their class is the best way to go. It's awkward, doesn't exist in the Apple frameworks (that I'm aware of outside of UIApplicationMain(), which as I noted, the vast majority of devs don't implement themselves), and in my opinion, most implementers wouldn't understand what they're supposed to do (and we already know they don't read the documentation).

    a. If we really want to create the objects ourself, I think we could receive a Class instead as this is better typed. We do this, for example, for security managers: @property (copy, nonatomic, nullable) NSArray<Class<SDLSecurityType>> *securityManagers;.

    b. I don't think receiving objects would be the worst thing in the world. We could probably hold on to the delegate object strongly in this case as well. It would increase memory usage, but by a very small amount.

  3. I agree with you that we don't need to have an SDLApplication subclass parameter.

joeljfischer commented 6 years ago

@AndrewRMitchell If you look at the code, there's no code that's called "Foundation", so no namespace conflict exists. It's just describing what part of the overall proposal this is. This isn't an issue.

NickFromAmazon commented 6 years ago

I think the SDLApplicationMain could be problematic for a few reasons:

Aside from that I generally like the API. I'm indifferent about expanding the appDelegate methods, except for in cases where it communicates unique information. For example, the first time I move from not running to active there's a bunch of configuration I may want to do that will persist across cases where the user exits, but does not disconnect (I think that's what applicationDidFinishLaunching is for?). For audioStreamingState transitions I can't think of any similar scenarios, so would be appreciate a less verbose API.

NickFromAmazon commented 6 years ago

the goal is to eventually have this UI-like framework handle everything for template-based apps.

As a middle-ground, could the SdlApplication expose an initializer with an SDLManger parameter? Clients who need access to the lower layer could use that and wire the managers into their types as needed; those who don't could use one of the initialization mechanisms recommended above. The API at this layer could be defined without needing to cross abstraction boundaries.

theresalech commented 6 years ago

The Steering Committee voted to defer this proposal, keeping it in review until our next meeting on 2018-05-29, to allow the author time to respond to the comments provided.

kshala-ford commented 6 years ago

@joeljfischer @NickFromAmazon thank you for your comments. I'll not quote your text but I want to inform you that I read all your comments and try to address all your questions.

SDLApplication

the conversation is moving out of scope. Focusing on this proposal the sdlManager and the permission manager should be accessible from the SDLApplication class as properties. The feasibility of not dealing with RPCs anymore (and not needing permission manager and sdlManager) is another topic.

SDLApplicationDelegate

Will remove the two delegate methods and replace them with methods per state (mentioned here).

SDLApplicationMain

Every downside mentioned about the main function is correct. As I said I was following UIKit and the main function's class names are my biggest concern, too. NSClassFromString is not a good idea and I guess that's why Apple/Xcode/Swift is taking care about it. UIApplicationMain was created with iOS2.0 2008 and mimics NSApplicationMain (added with macOS 10.0 June 2001).

@joeljfischer I've seen your class method +(SDLApplication *)initializeSDLWithConfiguration:(SDLConfiguration *)configuration rootViewController:(__kindof SDLViewController *)viewController delegate:(id<SDLApplicationDelegate>)delegate;. I'm pretty much OK with it. I saw your note about Class<SDLSecurityType>. I'm not experienced with that but it only seems to work for protocols.

@NickFromAmazon The initializer accepting a SDLManager object sounds interesting but I'm afraid of all the manager states the object could be when initializing the SDL application.

I don't know if the initializer should return the application object. For the application class I would like a .sharedApplication singleton (and only for this class) which is set to an object if the phone connects to a head unit (or may be after initialize is called). @NickFromAmazon I know you have a different opinion about this but for most app developers they are looking for only one SDL instance and is what I want to offer in the public API. To get multiple SDLApplications (but only one would be the primary, the shared application) please read this part of the proposal:

A private extension will be used in order to create objects out of SDLApplication. This extension should be located in a separate private header file but should be implemented in the application's .m file. This would hide the technical possibility for creating multiple SDLApplication objects. The function SDLApplicationMain() should use the initializer and set the shared application instance. Those who master SDL and need to have multiple SDL applications can reuse the extension and create new objects manually.

@interface SDLApplication()

/** Returns the singleton app instance. */
@property (strong, nonatomic, class, readwrite) SDLApplication *sharedApplication;

- (instancetype)initWithConfiguration:(SDLConfiguration *)configuration delegate:(id<SDLApplicationDelegate>)delegate;

@end
theresalech commented 6 years ago

The Steering Committee has voted to accept this proposal with revisions. The revisions will include the details outlined in this comment from the author.

theresalech commented 6 years ago

@kshala-ford 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 an issue in the iOS repository for implementation. Thanks!

theresalech commented 6 years ago

Proposal has been updated to reflect the agreed upon revisions, and an issue has been entered: [SDL 0156] High level interface: Foundation