Skip to content
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

SDL 0180 - Fix to support broaden choice cell uniqueness #1886

Merged

Conversation

FrankElias77
Copy link
Contributor

Fixes #1024

Risk

This PR makes no API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have run the unit tests with this PR
  • I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).

Unit Tests

Unit test update to support cells with same title

Core Tests

N/A

Core version / branch / commit hash / module tested against: N/A
HMI name / version / branch / commit hash / module tested against: N/A

Summary

broadens choice and sub-menu uniqueness to allow PerformInteraction and sub-menu AddCommand primary text to be identical.

Changelog

Breaking Changes
  • N/A
Enhancements
  • N/A
Bug Fixes
  • broadens choice and sub-menu uniqueness to allow PerformInteraction and sub-menu AddCommand primary text to be identical.

Tasks Remaining:

N/a

CLA

@FrankElias77 FrankElias77 added proposal Accepted SDL Evolution Proposal manager-screen Relating to the manager layer - screen managers labels Jan 8, 2021
@FrankElias77 FrankElias77 self-assigned this Jan 8, 2021
@FrankElias77 FrankElias77 changed the title Fix to support broaden choice cell uniqueness SDL 0180 - Fix to support broaden choice cell uniqueness Jan 8, 2021
@FrankElias77 FrankElias77 marked this pull request as ready for review January 12, 2021 18:44
Copy link
Contributor

@joeljfischer joeljfischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs to affect MenuCell and the MenuManager

SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
@NicoleYarroch NicoleYarroch linked an issue Jan 29, 2021 that may be closed by this pull request
@NicoleYarroch NicoleYarroch self-requested a review January 29, 2021 15:10
@codecov
Copy link

codecov bot commented Feb 2, 2021

Codecov Report

Merging #1886 (78342fc) into develop (ee70ad1) will decrease coverage by 0.42%.
The diff coverage is 96.33%.

@@             Coverage Diff             @@
##           develop    #1886      +/-   ##
===========================================
- Coverage    85.41%   84.98%   -0.43%     
===========================================
  Files          434      428       -6     
  Lines        22091    21511     -580     
===========================================
- Hits         18868    18282     -586     
- Misses        3223     3229       +6     

Copy link
Contributor

@joeljfischer joeljfischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional missing pieces:

  1. SDLMenuCellSpec, SDLChoiceCellSpec, SDLPreloadChoicesOperationSpec updates

SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLChoiceCell.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLMenuCell.h Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLMenuCell.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLMenuCell.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
@@ -160,41 +162,39 @@ - (void)setMenuConfiguration:(SDLMenuConfiguration *)menuConfiguration {
}

- (void)setMenuCells:(NSArray<SDLMenuCell *> *)menuCells {
NSArray<SDLMenuCell *> *cells = menuCells;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary? It doesn't seem to be doing anything.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be brought up again. I don't think this is doing anything in the current scheme. All references to cells below seem that they can be changed to menuCells.

SmartDeviceLink/private/SDLMenuManager.m Show resolved Hide resolved
SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLChoiceSet.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLChoiceSet.m Outdated Show resolved Hide resolved
@@ -160,41 +162,39 @@ - (void)setMenuConfiguration:(SDLMenuConfiguration *)menuConfiguration {
}

- (void)setMenuCells:(NSArray<SDLMenuCell *> *)menuCells {
NSArray<SDLMenuCell *> *cells = menuCells;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be brought up again. I don't think this is doing anything in the current scheme. All references to cells below seem that they can be changed to menuCells.

SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLChoiceSet.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLChoiceSet.m Outdated Show resolved Hide resolved
…eCell-uniqueness

# Conflicts:
#	SmartDeviceLink-iOS.xcodeproj/project.pbxproj
#	SmartDeviceLink/private/SDLMenuManager.m
#	SmartDeviceLink/public/SDLMenuCell.h
#	SmartDeviceLinkTests/DevAPISpecs/SDLMenuCellSpec.m
#	SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m
Copy link
Contributor

@joeljfischer joeljfischer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not check unit tests or core tests due to the changes needed here.

SmartDeviceLink/private/NSArray+NSObject.h Outdated Show resolved Hide resolved
SmartDeviceLink/private/NSArray+NSObject.h Outdated Show resolved Hide resolved
SmartDeviceLink/private/NSArray+NSObject.h Outdated Show resolved Hide resolved
SmartDeviceLink/private/NSArray+NSObject.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/NSArray+NSObject.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLChoiceSet.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLChoiceSet.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLMenuCell.h Outdated Show resolved Hide resolved
FrankElias77 and others added 8 commits February 19, 2021 16:19
* Simplify dynamicHash method
* Move the menuCells documentation to the SDLScreenManager.h file
* Fix menu manager using instance properties to handle recursive searches, instead passing pointers
* Add tests for dynamicHash method
* Fix test for duplicate cells
* Choice sets with identical items would sometimes present out of order because they preloaded out of order
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a couple of edge case issues.

SmartDeviceLinkTests/DevAPISpecs/SDLChoiceSetManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/DevAPISpecs/SDLChoiceSetManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/public/SDLChoiceSet.m Show resolved Hide resolved
…eCell-uniqueness

# Conflicts:
#	SmartDeviceLink-iOS.xcodeproj/project.pbxproj
#	SmartDeviceLink/private/SDLMenuManager.m
#	SmartDeviceLinkTests/DevAPISpecs/SDLChoiceSetManagerSpec.m
#	SmartDeviceLinkTests/DevAPISpecs/SDLMenuManagerSpec.m
@joeljfischer joeljfischer merged commit 19055a8 into develop Mar 4, 2021
@joeljfischer joeljfischer deleted the feature/issue-1024-sdl-0180-broaden-choiceCell-uniqueness branch March 4, 2021 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
manager-screen Relating to the manager layer - screen managers proposal Accepted SDL Evolution Proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SDL 0180] Broaden Choice Uniqueness
3 participants