-
Notifications
You must be signed in to change notification settings - Fork 130
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
[0238] Keyboard Enhancements #1587
[0238] Keyboard Enhancements #1587
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1587 +/- ##
=============================================
+ Coverage 53.94% 54.04% +0.10%
- Complexity 5077 5105 +28
=============================================
Files 540 543 +3
Lines 23425 23488 +63
Branches 2885 2892 +7
=============================================
+ Hits 12637 12695 +58
+ Misses 9749 9747 -2
- Partials 1039 1046 +7 |
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.
@KostyaBoss I have reviewed this PR. I have suggested some changes. Please take a look, and make the changes as needed.
List<KeyboardInputMask> enumValueList = Arrays.asList(KeyboardInputMask.values()); | ||
|
||
List<KeyboardInputMask> enumTestList = new ArrayList<KeyboardInputMask>(); | ||
enumTestList.add(KeyboardInputMask.DISABLE_INPUT_KEY_MASK); |
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.
@KostyaBoss I would switch lines 62 and 63 so ENABLE
comes before DISABLE
. In this way, throughout the code the ordering will be consistent (you have ENABLE
, DISABLE
, and then USER_CHOICE
).
@@ -0,0 +1,74 @@ | |||
package com.smartdevicelink.test.rpc.datatypes; | |||
|
|||
import com.smartdevicelink.proxy.rpc.ClusterModeStatus; |
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.
@KostyaBoss You can remove the unused import on line 3.
|
||
import com.smartdevicelink.proxy.rpc.ClusterModeStatus; | ||
import com.smartdevicelink.proxy.rpc.ConfigurableKeyboards; | ||
import com.smartdevicelink.proxy.rpc.enums.CarModeStatus; |
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.
@KostyaBoss You can remove the unused import on line 5.
import com.smartdevicelink.proxy.rpc.ConfigurableKeyboards; | ||
import com.smartdevicelink.proxy.rpc.enums.CarModeStatus; | ||
import com.smartdevicelink.proxy.rpc.enums.KeyboardLayout; | ||
import com.smartdevicelink.proxy.rpc.enums.PowerModeQualificationStatus; |
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.
@KostyaBoss You can remove the unused imports on lines 7 and 8.
*/ | ||
|
||
public class KeyboardProperties extends RPCStruct { | ||
public static final String KEY_KEYPRESS_MODE = "keypressMode"; | ||
public static final String KEY_KEYBOARD_LAYOUT = "keyboardLayout"; | ||
public static final String KEY_LIMITED_CHARACTER_LIST = "limitedCharacterList"; | ||
/** | ||
* @since SmartDeviceLink 3.0.0 |
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.
@KostyaBoss I would suggest deleting lines 134-137. It is correct since it is generated by the code generator tool, but it is out of scope of the PR.
* | ||
* @return Boolean Availability of capability to mask input characters using keyboard. True: Available, | ||
* False: Not Available | ||
* @since SmartDeviceLink 7.1.0 |
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.
@KostyaBoss I would remove line 131 since it is not part of the proposal's XML.
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.
@santanamk, This file is new and introduced in the XML related. Do we still need to rollback (these changes are from the generator)
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.
@KostyaBoss So I went through the KeyboardCapabilities.java
file and the only differences I see is (when comparing my generated file with yours) is in a few places you have added the @ s i n c e SmartDeviceLink 7.1.0
where the generator didn't generate it. I think this is okay. You don't need to change anything.
Hashtable<String, Object> hashTest = JsonRPCMarshaller.deserializeJSONObject(underTestArray.getJSONObject(i)); | ||
assertTrue(TestValues.TRUE, Validator.validateConfigurableKeyboards(new ConfigurableKeyboards(hashReference), new ConfigurableKeyboards(hashTest))); | ||
} | ||
}/* else if (key.equals(KeyboardCapabilities.KEY_SUPPORTED_KEYBOARD_LAYOUTS)) { |
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.
@KostyaBoss Do you need to uncomment lines 81-91 for the KEY_SUPPORTED_KEYBOARD_LAYOUTS
?
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.
@santanamk, thanks for bringing this up. I will delete, the layouts test case is covered by the final else branch
KeyboardCapabilities msg = new KeyboardCapabilities(); | ||
assertNotNull(TestValues.NOT_NULL, msg); | ||
|
||
// Keypress mode is created in the object constructor |
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.
@KostyaBoss Do you need this comment on line 51?
return keyboard1.getKeyboardLayout().equals(keyboard2.getKeyboardLayout()) && keyboard1.getNumConfigurableKeys().equals(keyboard2.getNumConfigurableKeys()); | ||
} | ||
|
||
public static boolean validateKeyboardCapability(KeyboardCapabilities keyboard1, KeyboardCapabilities keyboard2) { |
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.
@KostyaBoss On line 3944 I would rename the parameters to keyboardCapabilities1
and keyboardCapabilities2
.
return keyboard1.getKeyboardLayout().equals(keyboard2.getKeyboardLayout()) && keyboard1.getNumConfigurableKeys().equals(keyboard2.getNumConfigurableKeys()); | ||
} | ||
|
||
public static boolean validateKeyboardCapability(KeyboardCapabilities keyboard1, KeyboardCapabilities keyboard2) { |
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.
@KostyaBoss I would rename the function on line 3944 to validateKeyboardCapabilities
.
@santhanamk I've updated the PR, please, re-review |
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.
@KostyaBoss I have reviewed the code review changes. The changes look good. I have a few suggestions mostly around the new manager code that you checked in. Please take a look whenever you get the chance.
@@ -84,4 +84,6 @@ | |||
* @param currentInputText - The user's full current input text | |||
*/ | |||
void onKeyboardDidSendEvent(KeyboardEvent event, String currentInputText); | |||
|
|||
void onMaskHasChanged(KeyboardEvent event); |
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.
@KostyaBoss On line 88 I would rename this to onKeyboardInputMaskHasChanged()
.
@@ -338,6 +338,8 @@ public void onNotified(RPCNotification notification) { | |||
if (onKeyboard.getEvent().equals(KeyboardEvent.ENTRY_VOICE) || onKeyboard.getEvent().equals(KeyboardEvent.ENTRY_SUBMITTED)) { | |||
// Submit Voice or Text | |||
keyboardListener.onUserDidSubmitInput(onKeyboard.getData(), onKeyboard.getEvent()); | |||
} else if (onKeyboard.getEvent().equals(KeyboardEvent.INPUT_KEY_MASK_ENABLED) || onKeyboard.getEvent().equals(KeyboardEvent.INPUT_KEY_MASK_DISABLED)) { |
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.
@KostyaBoss Since these enums were most recently added I would move the else if
block on lines 341-342 below the last else if
(around lines 362-363).
@@ -338,6 +338,8 @@ public void onNotified(RPCNotification notification) { | |||
if (onKeyboard.getEvent().equals(KeyboardEvent.ENTRY_VOICE) || onKeyboard.getEvent().equals(KeyboardEvent.ENTRY_SUBMITTED)) { | |||
// Submit Voice or Text | |||
keyboardListener.onUserDidSubmitInput(onKeyboard.getData(), onKeyboard.getEvent()); | |||
} else if (onKeyboard.getEvent().equals(KeyboardEvent.INPUT_KEY_MASK_ENABLED) || onKeyboard.getEvent().equals(KeyboardEvent.INPUT_KEY_MASK_DISABLED)) { | |||
keyboardListener.onMaskHasChanged(onKeyboard.getEvent()); | |||
} else if (onKeyboard.getEvent().equals(KeyboardEvent.KEYPRESS)) { |
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.
@KostyaBoss Do the unit tests in PresentChoiceSetOperationTests.java
need to be updated?
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.
@santanamk there were no tests covering the mask previously so I didn't add some new ones. To my mind, the PresentChoiceSetOperationTests are more about thread-safe testing, but we are not introducing some major changes at this point, so I don't think we should push the changes. Please, let me know if you still think we need to update them
@@ -284,6 +284,8 @@ public void onNotified(RPCNotification notification) { | |||
if (onKeyboard.getEvent().equals(KeyboardEvent.ENTRY_VOICE) || onKeyboard.getEvent().equals(KeyboardEvent.ENTRY_SUBMITTED)) { | |||
// Submit Voice or Text | |||
keyboardListener.onUserDidSubmitInput(onKeyboard.getData(), onKeyboard.getEvent()); | |||
} else if (onKeyboard.getEvent().equals(KeyboardEvent.INPUT_KEY_MASK_ENABLED) || onKeyboard.getEvent().equals(KeyboardEvent.INPUT_KEY_MASK_DISABLED)) { |
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.
@KostyaBoss Since these enums were most recently added I would move the else if block
on lines 287-288 below the last else if (around line 310).
@@ -284,6 +284,8 @@ public void onNotified(RPCNotification notification) { | |||
if (onKeyboard.getEvent().equals(KeyboardEvent.ENTRY_VOICE) || onKeyboard.getEvent().equals(KeyboardEvent.ENTRY_SUBMITTED)) { | |||
// Submit Voice or Text | |||
keyboardListener.onUserDidSubmitInput(onKeyboard.getData(), onKeyboard.getEvent()); | |||
} else if (onKeyboard.getEvent().equals(KeyboardEvent.INPUT_KEY_MASK_ENABLED) || onKeyboard.getEvent().equals(KeyboardEvent.INPUT_KEY_MASK_DISABLED)) { | |||
keyboardListener.onMaskHasChanged(onKeyboard.getEvent()); |
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.
@KostyaBoss Do the unit tests in PresentKeyboardOperationTests.java
need to be updated?
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.
@santhanamk there were no tests covering the mask previously so I didn't add some new ones. To my mind, the PresentChoiceSetOperationTests are more about thread-safe testing, but we are not introducing some major changes at this point, so I don't think we should push the changes. Please, let me know if you still think we need to update them
@@ -541,6 +546,12 @@ public void onCapabilityRetrieved(Object capability) { | |||
int currentWindowID = windowCapability.getWindowID() != null ? windowCapability.getWindowID() : PredefinedWindows.DEFAULT_WINDOW.getValue(); | |||
if (currentWindowID == PredefinedWindows.DEFAULT_WINDOW.getValue()) { | |||
defaultMainWindowCapability = windowCapability; | |||
|
|||
KeyboardCapabilities keyboardCapabilities = windowCapability.getKeyboardCapabilities(); | |||
if (keyboardCapabilities.getMaskInputCharactersSupported() != null |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
if (keyboardCapabilities.getMaskInputCharactersSupported() != null | ||
&& !keyboardCapabilities.getMaskInputCharactersSupported()) { | ||
keyboardConfiguration.setMaskInputCharacters(KeyboardInputMask.DISABLE_INPUT_KEY_MASK); | ||
} |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@santhanamk I've updated the pr, please, re-review |
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.
@KostyaBoss Code looks good. I have approved the PR.
Once you have tested against Core and HMI (if applicable), please put the links for those PRs. Please mark the PR as ready for review, and you can tag Livio once the testing is done on your end.
@bilal-alsharifi , @jordynmackool the PR is ready for the review |
@KostyaBoss the proposal markdown file has been updated per the revisions included in the accepted with revisions review issue: Revise SDL-0238 Keyboard Enhancements. Please make the needed updates to this PR and then tag @bilal-alsharifi to review when ready. Thanks! |
@KostyaBoss @AKalinich-Luxoft is the PR ready for re-review? |
@bilal-alsharifi yes, it is ready. Please check for revision changes in cd1136e |
# Conflicts: # android/sdl_android/src/androidTest/java/com/smartdevicelink/test/TestValues.java # android/sdl_android/src/androidTest/java/com/smartdevicelink/test/Validator.java
@bilal-alsharifi I've processed the MR conflicts, could you please re-check the PR? |
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardCapabilities.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardCapabilities.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardCapabilities.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardCapabilities.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardProperties.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/KeyboardListener.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
@bilal-alsharifi I've made corresponding changes, could you please check? |
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardProperties.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardProperties.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardProperties.java
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardProperties.java
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Show resolved
Hide resolved
@bilal-alsharifi I've processed the comments, please check the PR |
base/src/main/java/com/smartdevicelink/proxy/rpc/KeyboardProperties.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Outdated
Show resolved
Hide resolved
…et/BaseChoiceSetManager.java Co-authored-by: Bilal Alsharifi <599206+bilal-alsharifi@users.noreply.github.com>
…rties.java Co-authored-by: Bilal Alsharifi <599206+bilal-alsharifi@users.noreply.github.com>
…nts' into feature/0298_keyboard_enchancements # Conflicts: # base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
@@ -110,6 +113,7 @@ | |||
// capabilities | |||
currentSystemContext = SystemContext.SYSCTXT_MAIN; | |||
currentHMILevel = HMILevel.HMI_NONE; | |||
keyboardConfiguration = defaultKeyboardConfiguration(); |
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.
@KostyaBoss this move is not required
if (keyboardConfiguration == null) { | ||
this.keyboardConfiguration = defaultKeyboardConfiguration(); | ||
} else { | ||
KeyboardProperties properties = new KeyboardProperties(); | ||
properties.setLanguage((keyboardConfiguration.getLanguage() == null ? Language.EN_US : keyboardConfiguration.getLanguage())); | ||
properties.setKeyboardLayout((keyboardConfiguration.getKeyboardLayout() == null ? KeyboardLayout.QWERTZ : keyboardConfiguration.getKeyboardLayout())); | ||
properties.setKeypressMode((keyboardConfiguration.getKeypressMode() == null ? KeypressMode.RESEND_CURRENT_ENTRY : keyboardConfiguration.getKeypressMode())); | ||
properties.setLimitedCharacterList(keyboardConfiguration.getLimitedCharacterList()); | ||
properties.setAutoCompleteText(keyboardConfiguration.getAutoCompleteText()); | ||
this.keyboardConfiguration = properties; | ||
this.keyboardConfiguration = createValidKeyboardConfigurationBasedOnKeyboardCapabilitiesFromConfiguration(keyboardConfiguration); | ||
} |
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.
Because createValidKeyboardConfigurationBasedOnKeyboardCapabilitiesFromConfiguration
could return null
we need to avoid setting null to this.keyboardConfiguration
please consider my suggestion below:
KeyboardProperties properties = null;
if (keyboardConfiguration == null) {
properties = createValidKeyboardConfigurationBasedOnKeyboardCapabilitiesFromConfiguration(keyboardConfiguration);
}
if (properties == null) {
this.keyboardConfiguration = defaultKeyboardConfiguration();
} else {
this.keyboardConfiguration = properties;
}
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.
@vladmu Good point. But I think we should always call createValidKeyboardConfigurationBasedOnKeyboardCapabilitiesFromConfiguration
even if the passed keyboardConfiguration
is not null right? do you agree with this suggestion?
public void setKeyboardConfiguration(@Nullable KeyboardProperties keyboardConfiguration) {
KeyboardProperties properties = createValidKeyboardConfigurationBasedOnKeyboardCapabilitiesFromConfiguration(keyboardConfiguration);
if (properties == null) {
this.keyboardConfiguration = defaultKeyboardConfiguration();
} else {
this.keyboardConfiguration = properties;
}
}
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.
@vladmu Thanks for the updates! @bilal-alsharifi I've included this as a separate commit, please, let me know if we should revert it
@bilal-alsharifi I've processed the changes, please, check the PR |
base/src/main/java/com/smartdevicelink/managers/screen/choiceset/BaseChoiceSetManager.java
Show resolved
Hide resolved
@bilal-alsharifi I've processed the comments, please, review |
@KostyaBoss Thanks for making the changes. I noticed that the CI is taking much longer than usual to run the tests. Can you check that all unit tests pass locally? |
@bilal-alsharifi I've run the tests locally, will report the results soon |
@bilal-alsharifi here are the results of the tests, those 3, I believe have failed due to run on the emulator, sometimes they work for me in this way |
I believe the ones that you listed fail because the device that you are testing on is not connected to the internet and thus not able to download the icon. |
@bilal-alsharifi observing the same behaviour (with CI) for other PR's as well, seems to be general problem |
@KostyaBoss I restarted the tests multiple times and they pass now. |
@bilal-alsharifi Hi! We've noticed that iOS and JS suites have been merged already, do you have additional feedback on this PR? |
@KostyaBoss it should be good now. I was just waiting for the other PRs to be ready to make sure all PRs are aligned. |
Fixes #1121
This PR is [ready] for review.
Related PR's
Core
HMI
Risk
This PR makes [minor] API changes.
Testing Plan
Unit Tests
Added tests to cover new structures and updated existing ones according to the introduced changes
Summary
Changes according to the #1121
CLA