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
Screen manager #824
Screen manager #824
Conversation
…ure/issue_782_screen_manager
…anager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java
…2_screen_manager_softbutton_manager # Conflicts: # sdl_android/src/main/java/com/smartdevicelink/api/SdlManager.java
…ates feature/text and graphic manager fixes
…_manager_sbm_fixes_per_comments Feature/issue 782 ScreenManager & SoftButtonManager fixes
@Override | ||
public synchronized void onComplete(boolean success) { | ||
if(!success){ | ||
Log.d(TAG, "Sub manager failed to initialize"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a check against if the submanagers failed to initialize so that the screen manager transitions to error state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it sets the screen manager state to ERROR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If both managers are not null and are in the ERROR state the ScreenManager will transition to the READY state after this call. However, this callback is essentially pointless at this point. Both the SoftButtonManager
and TextAndGraphicManager
will be in the READY state after their constructors return. See here, the start()
methods are not used in either managers and will set the completionListneer, check state, realize they are in READY state (as at this point neither have a logic path to allow them to go into ERROR state), notify the callback, then clear the reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only place that I can think of for switching to state ERROR is that when they cannot retrieve capabilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The start method is called, but it is not overridden (not used) in either submanager so they will always return immediately.
|
||
//Constructors | ||
|
||
TextAndGraphicManager(ISdl internalInterface, FileManager fileManager, SoftButtonManager softButtonManager) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All vars should be considered @nonnull right?
|
||
for (TextField field : textFields) { | ||
if (field.getName() != null) { | ||
TextFieldName name = field.getName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move the declaration for TexFieldName name;
before the for
loop and just set the name through each iteration rather than create a new reference.
@@ -18,7 +18,7 @@ | |||
// states - if this gets more complicated we can move elsewhere | |||
private int state; | |||
private final Object STATE_LOCK = new Object(); | |||
public static final int SETTING_UP = 0x00, READY = 0x30, SHUTDOWN = 0x60, ERROR = 0x90; | |||
public static final int SETTING_UP = 0x00, READY = 0x30, SHUTDOWN = 0x60, ERROR = 0x90, LIMITED = 0xC0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change this as follows:
public static final int SETTING_UP = 0x00, READY = 0x30, LIMITED = 0x50, SHUTDOWN = 0x80, ERROR = 0xC0;
Fixes #782
This PR is ready for review.
Risk
This PR makes minor API changes.
Testing Plan
Summary
This PR implements the
ScreenManager
as described in the proposal.SoftButtonManager
TextAndGraphicManager
ScreenManager
CLA