-
Notifications
You must be signed in to change notification settings - Fork 121
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 0156 - High level interface: Foundation #448
Comments
Questions / Concerns: Application Lifecycle
There will be a
I would rather see these actually say
View Controller LifestyleI also have some concerns about this and would like some clarifications.
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
viewWillAppear
I don't think the VC is visible in SDLViewController
SDLViewControllerManagerI'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 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 ModalViewControllers
|
Thank you @joeljfischer for the detailed review. Let me try to provide some explanation/answers. Please let me know if I missed someting. Application lifecycleI 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.
The very root of every iOS app is I thought of adding 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 Again I never thought through it. I'm open to suggestions.
Thanks for #264. I'll take a look at it and make sure it's used within
Sure. There's no intention to use
I struggle with 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 had the same thought but then View controller lifecycle
See
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
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
Actually it is. On SYNC3 media apps are visible on the home screen. SDLViewController
I was following the method names from UIKit. I just realize that
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
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
In SDLViewControllerManager
This is the reason why I didn't propose an 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.
I get your point. a hard coded stack might not be the best idea. May be we should add
The ModalViewControllers
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 think this wasn't clear in the proposal. The
It's more of a placeholder to explain how audio data is passed to the app. Any suggestion for naming?
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.
Absolutely.
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.
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. |
Application LifecycleRegarding 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 think that would be better/clearer.
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
I think there's a misunderstanding of my comments. I think we should skip having
Hopefully that helps clarify things.
I was thinking more about the viewDidAppear / disappear aspects of this. SDLViewController
There's no modal one on the nav view controller, as a note.
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.
Yes, I agree with that.
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.
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
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
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.
If we really do need a stack, I would propose having a
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:
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
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.
Umm...
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
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.
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.
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. |
Application lifecycle
I agree to have a shared instance in 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 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 LifecycleI 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 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.
SDLViewController
I don't think is simplier. Every time when I want another VC to be shown/pushed I need to call
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.
True. It's missing. I didn't focus on error handling at this stage as I thought it's more a development detail. SDLViewControllerManager
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.
Depending on the mode of the VC manager the method
My goal is to unify all the variants of modal text fields into a single, multiline parameter called
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... I still prefer the base class for
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.
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
Keyboards can be used by any app. ...WITH_SEARCH and KEYBOARD are equivalents, too. Interactions should come with keyboard and choice set support.
Isn't that an overkill? This is the reason why I was asking if there are shortcuts for creating single-state buttons.
Whoops I mean
That would be a total separate VC next to the root view controller. UIMenuController inherits 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:
|
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. |
@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. |
I also think breaking it up would make it easier to digest and come to a consensus on. |
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. |
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. |
As SDL 0157 has been resolved, the review of this revised proposal begins now and runs through May 22, 2018. |
SDLApplicationThis should probably provide a reference to its own
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 SDLApplicationDelegateMinor 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 - (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. SDLApplicationMainBecause (virtually) no developers actually use, much less call
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:
in reference to the delegate object. Shouldn't they have already created the object and reference is passed here? |
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? |
SDLApplicationThe 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. SDLApplicationDelegateI 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. SDLApplicationMainI'm sorry for the confusion. The way I like the idea of the function. If follows UIKit and provides the shared @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. |
@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. |
SDLApplicationAbsolutely, 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 I believe we should expose the underlying SDLApplicationDelegateI'm leaning more towards a verbose delegate framework, so long as they're optional. But either way is probably okay. SDLApplicationMainThank 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.
a. I believe a traditional class method, object initializer, or singleton call would be better understood by implementers.
a. If we really want to create the objects ourself, I think we could receive a 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.
|
@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. |
I think the
Aside from that I generally like the API. I'm indifferent about expanding the |
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. |
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. |
@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. SDLApplicationthe 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. SDLApplicationDelegateWill remove the two delegate methods and replace them with methods per state (mentioned here). SDLApplicationMainEvery 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. @joeljfischer I've seen your class method @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
|
…sue_447 Fix a NPE when calling handleInitResult
The Steering Committee has voted to accept this proposal with revisions. The revisions will include the details outlined in this comment from the author. |
@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! |
Proposal has been updated to reflect the agreed upon revisions, and an issue has been entered: [SDL 0156] High level interface: Foundation |
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:
#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:
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
The text was updated successfully, but these errors were encountered: