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-0196] Support Static Icon in Screen Manager #1106

Merged

Conversation

joeljfischer
Copy link
Contributor

@joeljfischer joeljfischer commented Oct 16, 2018

Fixes #1062

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

Unit tests and smoke tests

Summary

SDLArtwork and screen manager updated to support static icons and the SDLStaticIconName enum.

Changelog

Enhancements
  • SDLArtwork can now be created with a static icon. Such artworks can be passed to the SDLScreenManager for use in various image slots such as soft buttons and choices.
Bug Fixes
  • Fixed failed SetDisplayLayout causing SDLScreenManager to break.

Tasks Remaining:

  • Unit Tests

CLA

@joeljfischer joeljfischer added enhancement proposal Accepted SDL Evolution Proposal labels Oct 16, 2018
@joeljfischer joeljfischer added this to the 6.2.0 milestone Oct 16, 2018
@joeljfischer joeljfischer self-assigned this Oct 16, 2018
@joeljfischer joeljfischer force-pushed the feature/issue_1062_static_icon_sdlartwork_support branch from 928e9b5 to e8fc233 Compare October 16, 2018 18:14
@NicoleYarroch
Copy link
Contributor

There are two failing test cases in SDLPreloadChoicesOperationSpec

@@ -293,7 +293,7 @@ - (void)sdl_uploadOtherStateImages {
for (SDLSoftButtonObject *object in self.softButtonObjects) {
for (SDLSoftButtonState *state in object.states) {
if ([state.name isEqualToString:object.currentState.name]) { continue; }
if (state.artwork != nil && ![self.fileManager hasUploadedFile:state.artwork]) {
if (![self sdl_artworkNeedsUpload:object.currentState.artwork]) {
[otherStatesToBeUploaded addObject:state.artwork];
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[otherStatesToBeUploaded addObject:state.artwork];
[otherStatesToBeUploaded addObject:object.currentState.artwork];

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise the app crashes when inserting a nil state.artwork into the array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's incorrect logic that will cause other state artwork to never get uploaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

All I know is that my app kept crashing at that point when I uploaded a soft button with a static icon image

@NicoleYarroch
Copy link
Contributor

The SDLMenuManager isn't set up to handle sending static artworks. Currently no artworks are sent if one of the menu items has a static icon.

@NicoleYarroch
Copy link
Contributor

Maybe you should add static icons to the PICS in the example apps so we have an example of how to use static icons?

…_support

# Conflicts:
#	SmartDeviceLink/SDLChoiceSetManager.m
#	SmartDeviceLink/SDLSoftButtonManager.m
#	SmartDeviceLink/SDLTextAndGraphicManager.m
* Many fixes to artwork uploads
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.

There are 2 broken test cases. The Swift example app needs to be updated as well.

@joeljfischer joeljfischer merged commit 2bf35ff into develop Nov 29, 2018
@joeljfischer joeljfischer deleted the feature/issue_1062_static_icon_sdlartwork_support branch November 29, 2018 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal Accepted SDL Evolution Proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants