joeljfischer
commented
5 years ago Hi, I'm happy to see this has moved to review. There's much that I agree with here, but I do have a number of notes:
-
The name of "overlay controller" may be misleading. When doing the review of the Core Certification guidelines, it was argued that some of these operations may not be done in an overlay style like Ford's Gen 3 (specifically, I think AudioPassThru was called out, as it's difficult to imagine others occurring without an overlay, though they could replace the template wholesale as well). I believe I originally criticized the name "Modal" for this very reason, however, I think it's better than "Overlay," and would prefer it if nothing better can be provided. At least "Modal" is describing various "modes of operation" rather than a UI overlay, which may or may not happen based on the OEM implementation.
-
I'm not sure that storing pending overlay controllers is better than just failing when one is already presented. I can see both being useful at different times, and the dev could implement either manually if they wanted it. Personally, I think I'd prefer failing. 🤷♂️ I've re-read the paragraph describing this a few times, but I think I'm still missing the purpose of allowing it, could you elaborate?
-
When a view controller wants to present an overlay controller it must call the presentOverlayController:completion: method of the VC manager. The presenting VC must be listed in the view controller stack but does not need to be the top view controller.
I'm not sure if this is the best policy. Isn't presenting an overlay from somewhere other than the top VC on the stack more likely to lead to bugs than to be useful? Apple does allow this in my testing, however, they log a message to the console whenever it happens:
Presenting view controllers on detached view controllers is discouraged
If really needed, a VC can already grab the VC from the top of the stack and send it a message, right? Can we cut off this possible avenue for bugs?
-
Should the method
- (void)presentOverlayController:(SDLOverlayController *)overlayController completion:(SDLOverlayControllerCompletionHandler)completion;be on theUIViewControllerequivalent as it is in UIKit? -
If the overlay controller has detected the end of the presentation it should notify the VC manager. This can also be done through an encapsulated overlay controller completion handler.
How would this work? With at least some of these (such as Alert), the AlertResponse is sent immediately, not when the overlay closes. Would it need an internal timer set to the timeout and hope they line up, plus listeners for the soft buttons since the alert closes once one is hit?
-
As it inherits SDLViewController it is possible to manipulate the head unit during a presentation. To do that the app developer must create a new view and place it in the view property of the overlay controller.
I'm not sure I'm understanding this point. It's possible to manipulate the view controller underneath the overlay? Or to manipulate the overlay?
-
Depending on the subclass (and the min/max values of the parameter) the duration value will be ignored if it exceeds the value range.
I agree with points 1 & 2, however, might it be better that if it's set to 0 (or not set at all due to an initializer not including that parameter) to set it to the default value? e.g. for ScrollableMessage to set it to 30 seconds instead of 65.535 seconds?
- I would vastly prefer to see
duration(and all other properties on the subclasses) bereadonlyto set developer expectations about changing it during presentation as well as to prevent bugs from us. We certainly will not be expecting them to change properties during presentation and it could cause significant issues that we don't want to have to account for. This is how many objects are designed in the SDL iOS library (e.g. SDLChoiceSet for the Choice Set Manager). There can be an initializer that takes the duration, or not and it's set to the default.
The main reason for this desire is these SDL UI controllers work fundamentally differently than UIKit ones. When you set those properties on UIKit, the values change on screen while it's on screen, with SDL, that will not happen. To help set expectations, I would prefer developers to understand they can set these values once. If they want something different, they need to make a new object.
-
SDLAlertControllerbuttonsshould perhaps note that it should fail to initialize anything with buttons containing more than one state? -
@property (nonatomic, copy) (^onAudioData)(NSData *audioData);
The name of this block and corresponding initializer names probably shouldn't include on, since that's an SDL RPC paradigm and not an iOS one? Perhaps audioDataProcessor, or audioDataHandler?
-
Is
@property (nonatomic, nonnull, copy) SDLAudioPassThruCapabilities *capabilities;intended to benullable? -
Am I crazy if I think
muteAudioSourceshould actually default totrue? The vast majority of the time the developer will prefer to pause the audio to make sure they get clear voice, right? -
I know I've made this point several times, but I'd be remiss not to note it in the review: I strongly disagree with the use of the
valueproperty of theSDLSliderControllerandSDLValuePickerController. I understand that's how UIKit does it, but the SDL version works entirely differently. AUISlideris an interface item in UIKit, not a time-limited overlay controller.
The slider's value therefore ought to be a value, as nothing else makes sense. We cannot KVO observe the slider value over time as we can in UIKit. However, because our Sliders are time-limited controllers, they ought to pass back the selected value in a completion handler style block, not require the developer to remember to check the value on the view controller after the response occurs. In the current proposal, there's no way to know except by repeatedly checking the value in a timed loop or checking in the SDLOverlayControllerCompletionHandler which doesn't even pass back the view controller, so they'd have to store it themselves or even if it was modified to include it, they'd have to cast it themselves.
It just makes sense to have a completion handler here that returns the value.
- I don't think
minimumValueandmaximumValuemake much sense here. They can set it to25and50but those values won't be reflected on the overlay, so what's the point?
What I'm thinking of for these classes is something like this:
@interface SDLSliderController : SDLOverlayController
@property (nonatomic, nonnull, copy) NSString *title;
@property (nonatomic, nullable, copy, readonly) NSString *message;
// must not exceed "maxvalue" of "Slider.numTicks"
@property (nonatomic, readonly) NSInteger numValues;
// maps to "Slider.position"
@property (nonatomic, readonly) NSInteger initialIndex;
// Passes back the selectedIndex
@property (nonatomic, readonly) SDLSliderCompletionHandler completionHandler;
- (instancetype)initWithTitle:(nonnull NSString *)title numPositions:(NSInteger)numPositions initialPosition:(NSInteger)initialPosition completionHandler:(SDLSliderCompletionHandler)completionHandler;
- (instancetype)initWithTitle:(nonnull NSString *)title message:(nonnull NSString *)message numPositions:(NSInteger)numPositions initialPosition:(NSInteger)initialPosition completionHandler:(SDLSliderCompletionHandler)completionHandler;
@end
@interface SDLValuePickerController : SDLOverlayController
@property (nonatomic, nonnull, copy, readonly) NSString *title;
@property (nonatomic, nonnull, copy, readonly) NSArray<NSString *> *valueLabels;
@property (nonatomic, readonly) NSInteger initialIndex;
// Passes back the selectedIndex (NSInteger) and selectedValue (NSString)
@property (nonatomic, readonly) SDLValuePickerCompletionHandler completionHandler;
- (instancetype)initWithTitle:(nonnull NSString *)title valueLabels:(nonnull NSArray<NSString *> *)valueLabels initialIndex:(NSInteger)initialIndex completionHandler:(SDLValuePickerCompletionHandler)completionHandler;
@endsomething like that.
SDLOverlayControllerCompletionHandlershould include theSDLOverlayViewControllerto which it corresponds.
kshala-ford
jordynmackool
NicoleYarroch
theresalech
Hello SDL community,
The review of "SDL 0201 - High level interface: Overlay Controllers" begins now and runs through February 19, 2019. The proposal is available here:
https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0201-high-level-interface-overlay-controllers.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/606
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, Jordyn Mackool
Program Manager - Livio jordyn@livio.io