Implement SDL 0231 Main Menu Tiles #1117
Conversation
BrettyWhite
changed the title
Codecov Report
@@ Coverage Diff @@
## develop #1117 +/- ##
=============================================
+ Coverage 46.79% 46.87% +0.08%
- Complexity 4024 4070 +46
=============================================
Files 455 457 +2
Lines 22186 22428 +242
Branches 2515 2553 +38
=============================================
+ Hits 10381 10513 +132
- Misses 11196 11298 +102
- Partials 609 617 +8
Continue to review full report at Codecov.
|
BrettyWhite
changed the title
|
|
||
| public class MenuConfiguration { | ||
|
|
||
| private MenuLayout mainMenuLayout, defaultSubmenuLayout; |
There was a problem hiding this comment.
I would either add the word "default" to both vars or remove it from both. Also same applied to the constructor params
There was a problem hiding this comment.
This is as intended in the proposal. There is only one main menu. However, you can override each submenu's layout in the MenuCell constructor, while making whatever you set here the 'default'.
| * @param icon The cell's image | ||
| * @param subCells The sub-cells for the sub menu that will appear when the cell is selected | ||
| */ | ||
| public MenuCell(@NonNull String title, @Nullable MenuLayout subMenuLayout, @Nullable SdlArtwork icon, @Nullable List<MenuCell> subCells) { |
There was a problem hiding this comment.
MenuLayout param should be added to the javadoc
| } | ||
|
|
||
| /** | ||
| * Sets the layout of the submenu screen. |
There was a problem hiding this comment.
| * Sets the layout of the submenu screen. | |
| * Gets the layout of the submenu screen. |
| @@ -83,6 +87,8 @@ | |||
| List<MenuCell> menuCells, waitingUpdateMenuCells, oldMenuCells, keepsNew, keepsOld; | |||
| List<RPCRequest> inProgressUpdate; | |||
| DynamicMenuUpdatesMode dynamicMenuUpdatesMode; | |||
| MenuConfiguration menuConfiguration; | |||
| SdlMsgVersion sdlMsgVersion; | |||
There was a problem hiding this comment.
We don't have to keep a class var for sdlMsgVersion since we can get it any time from Isdl
There was a problem hiding this comment.
going to keep it because i can't test the setter without this
| * This will be used when a menu item with sub-cells has a null value for menuConfiguration | ||
| * @param menuConfiguration - The default menuConfiguration | ||
| */ | ||
| public void setMenuConfiguration(final MenuConfiguration menuConfiguration) { |
There was a problem hiding this comment.
Should we add @ NonNull to the menuConfig param?
| * The main menu layout. See available menu layouts on DisplayCapabilities.menuLayoutsAvailable. Defaults to LIST. | ||
| * @param menuConfiguration - The default menuConfiguration | ||
| */ | ||
| public void setMenuConfiguration(MenuConfiguration menuConfiguration) { |
There was a problem hiding this comment.
Should we add @ NonNull to the menuConfig param?
| @@ -30,6 +31,7 @@ protected RPCMessage createMessage(){ | |||
| msg.setMenuName(Test.GENERAL_STRING); | |||
| msg.setPosition(Test.GENERAL_INT); | |||
| msg.setMenuIcon(Test.GENERAL_IMAGE); | |||
| msg.setMenuLayout(Test.GENERAL_MENU_LAYOUT); | |||
There was a problem hiding this comment.
There are some extra spaces here and in other places in this file
| @@ -69,6 +72,7 @@ public void testRpcValues () { | |||
| assertEquals(Test.MATCH, Test.GENERAL_MEDIACLOCKFORMAT_LIST.size(), mediaClock.size()); | |||
| assertEquals(Test.MATCH, Test.GENERAL_TEXTFIELD_LIST.size(), textFields.size()); | |||
| assertEquals(Test.MATCH, Test.GENERAL_IMAGEFIELD_LIST.size(), imageFields.size()); | |||
| assertEquals(Test.MATCH, Test.GENERAL_MENU_LAYOUT_LIST.size(), menuLayouts.size()); | |||
There was a problem hiding this comment.
There is a spacing issue here
Fixes #1076
This PR is ready for review.
Risk
This PR makes minor API changes.
Testing Plan
Unit tests added and smoke tested against core PR
Summary
Implement proposal as defined
CLA