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

Fix ScreenManager not sending updates in certain cases that it should #1575

Merged

Conversation

joeljfischer
Copy link
Contributor

@joeljfischer joeljfischer commented Mar 3, 2020

Fixes #1536

This PR is ready for review.

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

  • Update unit tests to pass properly
  • Add new unit tests for sub-managers to ensure they send properly if displayCapabilities is null.

Core Tests

  • Tested the screen managers to ensure continued functionality.

Core version / branch / commit hash / module tested against: Sync 3.0 (17276_DEVTEST)
HMI name / version / branch / commit hash / module tested against: Sync 3.0 (17276_DEVTEST)

Summary

This PR fixes a possible case where the head unit sends back a nil displayCapabilities or displayCapabilities.textFields / imageFields would cause the screen managers to not send data. Instead, we will now send all possible data and leave it to the head unit to reject or show what it determines is appropriate.

Changelog

Enhancements
  • Added a few RPC struct initializers.
Bug Fixes
  • Fix a possible case where the head unit sends back a nil displayCapabilities or displayCapabilities.textFields / imageFields would cause the screen managers to not send data. Instead, we will now send all possible data and leave it to the head unit to reject or show what it determines is appropriate.
  • Fixed the choice set manager sending data when the head unit sends textFields without the primary choice text.
  • Fixed the soft button manager attempting to send soft buttons when no soft button capabilities are present.

CLA

* Fixes screen manager sub-managers checking windowCapability nullability, but it's always non-null. Instead, check the image and text fields for nullability
@joeljfischer joeljfischer added bug A defect in the library manager-screen Relating to the manager layer - screen managers labels Mar 3, 2020
@joeljfischer joeljfischer self-assigned this Mar 3, 2020
@joeljfischer joeljfischer added this to In progress in v6.6.0 via automation Mar 3, 2020
* Add test for setting media track and title in TextAndGraphicManager
* Fix initializer with incorrect parameter types causing a crash
* If we don't have soft button capabilities, suspend the transaction queue
* ChoiceSetManager now suspends and starts its transaction queue in a consistent manner based on the hmi level, system context, and TextFieldName MenuName (Choice primary text) availability.
* Updated ChoiceSetManager tests to account for these updates
* If displayCapabilities is nil, allow the transaction queue to run and attempt to show choice sets
@NicoleYarroch NicoleYarroch self-requested a review March 5, 2020 18:22
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.

Left some comments

SmartDeviceLink/SDLDisplayCapability.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLChoiceSetManager.m Show resolved Hide resolved
SmartDeviceLink/SDLChoiceSetManager.m Show resolved Hide resolved
SmartDeviceLink/SDLMenuManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSoftButtonManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSoftButtonManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSoftButtonManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLTextAndGraphicManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLTextField.h Outdated Show resolved Hide resolved
v6.6.0 automation moved this from In progress to Waiting for Review Mar 10, 2020
… of https://github.com/smartdevicelink/sdl_ios into bugfix/issue-1536-empty-displaycapabilities-workaround

# Conflicts:
#	SmartDeviceLink/SDLSoftButtonManager.m
* Update how the soft button manager updates its capabilities
  * Only send a replace operation if the capabilities changed
  * Break from the loop early
* Fix `hasData` not returning true if media text or title were available
* Separate methods for checking media text / title text fields available
* Fix the choice set manager starting in HMI NONE
* Add a huge number of logs
* Add documentation
* Don't suspend transaction queue based on system context
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.

Left some comments.

SmartDeviceLink/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLChoiceSetManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSoftButtonManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLChoiceSetManager.m Show resolved Hide resolved
@NicoleYarroch NicoleYarroch self-requested a review March 17, 2020 21:00
v6.6.0 automation moved this from Waiting for Review to Reviewer approved Mar 17, 2020
@joeljfischer joeljfischer merged commit b2660aa into develop Mar 17, 2020
v6.6.0 automation moved this from Reviewer approved to Done Mar 17, 2020
@joeljfischer joeljfischer deleted the bugfix/issue-1536-empty-displaycapabilities-workaround branch March 17, 2020 21:15
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
No open projects
v6.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

Empty displayCapabilities in setDisplayLayout response prevents display update
2 participants