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

2093 - Soft button manager image upload fix #2094

Merged

Conversation

joeljfischer
Copy link
Contributor

@joeljfischer joeljfischer commented Jun 8, 2022

Fixes #2093

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 added for softButtonCapabilities.imageSupported = YES and displayCapabilities.graphicEnabled = NO, as well as visa-versa.

Core Tests

Tested that existing behavior continues to

Core version / branch / commit hash / module tested against: Sync 3.4 TDK (19353_DEVTEST)
HMI name / version / branch / commit hash / module tested against: Sync 3.4 TDK (19353_DEVTEST)

Summary

This PR works around a bug when connecting to Sync 2.0 head units that causes the app to attempt to upload soft button images that will never work. This wastes bandwidth and time.

The issues happens because Sync 2.0 doesn't support softButtonCapabilities, so the library assumes all capabilities are available. However, Sync 2.0 does report that all graphics are unsupported in RAIR.displayCapabilities.graphicSupported.

Changelog

Bug Fixes
  • Fixes attempted uploads of image data on SDL 2.0

Tasks Remaining:

n/a

CLA

* Soft button manager will now only upload soft button images if the `RAIR.displayCapabilities.graphicSupported` bool is `NO`
* Fix soft button images not working with head units that don't send `displayCapabilities` (intra-PR fix)
@joeljfischer joeljfischer added bug A defect in the library manager-screen Relating to the manager layer - screen managers labels Jun 8, 2022
@joeljfischer joeljfischer requested a review from FrankElias77 June 8, 2022 19:51
@codecov
Copy link

codecov bot commented Jun 8, 2022

Codecov Report

Merging #2094 (f9fc42d) into develop (4580b82) will decrease coverage by 0.02%.
The diff coverage is 75.00%.

❗ Current head f9fc42d differs from pull request most recent head f62311c. Consider uploading reports for the commit f62311c to get more accurate results

@@             Coverage Diff             @@
##           develop    #2094      +/-   ##
===========================================
- Coverage    85.38%   85.35%   -0.03%     
===========================================
  Files          447      447              
  Lines        22598    22646      +48     
===========================================
+ Hits         19295    19330      +35     
- Misses        3303     3316      +13     

* Fix soft button operation not supporting text and static image soft buttons after the dynamic image fix
* Add tests for static only capabilities
Copy link
Contributor

@FrankElias77 FrankElias77 left a comment

Choose a reason for hiding this comment

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

iOS App: SmokeTester
SDL Version: Branch Develop
SYNC Version: 2.0


BUG #1

If we send 2 softButtons, one with a staticImage and the other with a dynamicImage to screenManager.softButtonObjects both buttons will not be displayed on HMI.

STEPS:
1.Send [softButtonWithStaticImageA, softButtonWithDynamicImageA].

Expected Behavior:
Both buttons displayed on HMI.

Observed Behavior:
No buttons displayed.

BUG #2

If we send 1 softButton with a dynamicImage to screenManager.softButtonObjects, button will not be displayed on HMI.

STEPS:
1.Send [softButtonWithDynamicImageA].

Expected Behavior:
Button displayed on HMI.

Observed Behavior:
No button displayed.

BUG #3

If we send 1 softButton that has 2 or more states, where every state contain a staticImage to screenManager.softButtonObjects, button will not be displayed on HMI.

STEPS:
1.Send [softButtonWithStaticImageAnd2States].

Expected Behavior:
Button displayed on HMI.

Observed Behavior:
No button displayed.

Copy link
Contributor

@FrankElias77 FrankElias77 left a comment

Choose a reason for hiding this comment

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

Update var graphicsEnabled -> isDynamicGraphicSupported, to align with the java suite PR.
I suggested the changes where the var is defined, the remaining changes should be noticeable when xcode flags the errors for using an unknown var

SmartDeviceLink/private/SDLSoftButtonManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLSoftButtonReplaceOperation.m Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLSoftButtonReplaceOperation.h Outdated Show resolved Hide resolved
SmartDeviceLink/private/SDLSoftButtonReplaceOperation.m Outdated Show resolved Hide resolved
SmartDeviceLinkTests/SDLSoftButtonReplaceOperationSpec.m Outdated Show resolved Hide resolved
* Change `graphicsEnabled` name to `dynamicGraphicSupported` to match Android lib
@joeljfischer joeljfischer merged commit c177ca8 into develop Jun 21, 2022
@joeljfischer joeljfischer deleted the bugfix/issue-2093-softbuttonmanager-image-upload-sdl-2.0 branch June 21, 2022 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A defect in the library manager-screen Relating to the manager layer - screen managers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ScreenManager Tries to Upload Soft Button Images on Sync 2.0
2 participants