Refactor Choice Set Manager to Fix Several Bugs #2019

joeljfischer · 2021-07-06T20:05:41Z

Fixes #2018, #2020, #2025

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 tests updated and rewritten

Core Tests

Tested sending a presentation, then sending another presentation when the first has a button pressed.
Tested sending a presentation on various systems in both swift and obj-c

Core version / branch / commit hash / module tested against: Manticore (SDL v7.1.1), Sync Gen 3.4
HMI name / version / branch / commit hash / module tested against: Manticore (Generic HMI v0.10.0), Sync Gen 3.4

Summary

This PR rewrites the choice set manager to use a full queuing system rather than the partial queueing system of the past.
This PR rewrites the preload / present operations into one operation to solve some queueing problems. The operation will now figure out what it has to do based on how it is initialized.
This PR changes cancelation behavior. Before if you sent a present, then another present, the manager would try to cancel previous ones. Now they will be queued and the dev can cancel previous ones if desired.
This PR fixes strange deletion behavior. Before if a delete came in while a preload / present was happening, the manager would try to delete choices from the upload. Now it will wait until the delete operation comes around and delete at that time. This allows proper behavior around preload -> present -> delete situations.

Changelog

Bug Fixes

Fixed unpredictable choice set behavior when quickly loading / presenting / deleting choices. They now will happen in the order that you call the methods.

Tasks Remaining:

Done

CLA

I have signed the CLA

* Pending presentations are now enqueued and canceled as needed * When a presentation fails, the request itself will now be printed

* Fix tests

* Fix updating cells unique names based on what's already on the head unit

* Fixed Choice cell not printing unique text correctly * Fixed uniqueness of uploading cells

* Add tests

JulianKast · 2021-08-26T17:46:39Z

SmartDeviceLink/private/SDLPreloadPresentChoicesOperation.m

+        if (![windowCapability hasImageFieldOfName:SDLImageFieldNameChoiceImage]) {
+            cell.artwork = nil;
+        }
+        if (![windowCapability hasTextFieldOfName:SDLTextFieldNameSecondaryText]) {


Should you use methods like sdl_shouldSendChoiceSecondaryTextBasedOnWindowCapability here instead of checking the windowCapability directly since it does the same thing?

joeljfischer · 2021-08-26T19:04:46Z

This needs to fix rollovers of choice ids, check based on existing choice ids, and error if there's no more ids

* Remove unused parameter from method

SmartDeviceLink/private/SDLChoiceSetManager.m

SmartDeviceLink/private/SDLPreloadPresentChoicesOperation.m

* Fixes a bug with pre-fall 2021 HMIs (smartdevicelink/generic_hmi@b292fbb)

* Don't attempt to upload duplicate artworks

SmartDeviceLink/private/SDLPreloadPresentChoicesOperation.m

Update choice set manager to handle pending presentations better

794eeb3

* Pending presentations are now enqueued and canceled as needed * When a presentation fails, the request itself will now be printed

joeljfischer linked an issue Jul 6, 2021 that may be closed by this pull request

Back-to-Back Choice Sets Can Fail #2018

Closed

joeljfischer added 25 commits July 8, 2021 15:52

Start rewriting how deletions work

73eb0eb

In progress operation changes

ea88a48

Lots of updates for new completion handlers

b3a3a77

Continue updating operations

925e5c1

Working on tests

2a7e311

More unit tests

3bd9d31

Check VR Optional re-do completion handler

cd5c7e9

When CSM is shutdown, error delegate should be called

1b2cc0f

Fixing tests

29d91ec

Fixing unit tests

e2ff279

More test fixes

89053a1

Preload and present test fixes

0078b2f

Remove unneeded synchronization

66c51a2

* Fix tests

More test fixes

36723cc

Move choice uniqueness code to preload op

2d8ea0d

Moving broaden choice tests to preload

7fadc8a

In-progress combine preload / present ops

fb0abfc

Finish first pass combining preload / present ops

5806c03

Fix last commit

da199be

In progress test updates

d61766b

More unit test updates

7f8ab4c

Rename spec file

fa6ba33

Preload/Present Op keep internal state to fix cancel stuff

a1720e3

Preload present op now handles delegate callback to simplify manager

58c6823

Preload choices fix

d1678e1

* Fix updating cells unique names based on what's already on the head unit

joeljfischer changed the title ~~Update choice set manager to handle pending presentations better~~ Refactor Choice Set Manager to Fix Several Bugs Jul 22, 2021

joeljfischer self-assigned this Jul 22, 2021

joeljfischer added the bug A defect in the library label Jul 22, 2021

joeljfischer added 5 commits August 18, 2021 13:39

Remove TODO

daed634

Fixes

7ec6842

* Fixed Choice cell not printing unique text correctly * Fixed uniqueness of uploading cells

Add complex test for choice cell upload uniqueness

e3a2e94

Fix some casting bugs

658d3d8

* Add tests

Fix focused test

bc29a7a

FrankElias77 previously approved these changes Aug 24, 2021

View reviewed changes

JulianKast reviewed Aug 26, 2021

View reviewed changes

In-progress fixes for choice set ids

8da784d

joeljfischer dismissed FrankElias77’s stale review via 8da784d August 30, 2021 13:53

joeljfischer added 3 commits August 30, 2021 16:04

In-progress choice set id changes and tests

fe3b547

Add additional choice set id tests

4efd8c6

Update choice set tests

71a5242

* Remove unused parameter from method

joeljfischer commented Sep 3, 2021

View reviewed changes

SmartDeviceLink/private/SDLChoiceSetManager.m Outdated Show resolved Hide resolved

joeljfischer commented Sep 3, 2021

View reviewed changes

SmartDeviceLink/private/SDLPreloadPresentChoicesOperation.m Show resolved Hide resolved

Fix minor issues

4678c18

FrankElias77 previously approved these changes Sep 8, 2021

View reviewed changes

Change choice ID start from 0 to 1

52c1e0f

* Fixes a bug with pre-fall 2021 HMIs (smartdevicelink/generic_hmi@b292fbb)

joeljfischer dismissed FrankElias77’s stale review via 52c1e0f September 13, 2021 13:10

FrankElias77 previously approved these changes Sep 14, 2021

View reviewed changes

Update choice set artwork upload

7242e32

* Don't attempt to upload duplicate artworks

joeljfischer dismissed FrankElias77’s stale review via 7242e32 September 14, 2021 17:59

FrankElias77 requested changes Sep 14, 2021

View reviewed changes

SmartDeviceLink/private/SDLPreloadPresentChoicesOperation.m Show resolved Hide resolved

Fix comments

8c5f3b0

FrankElias77 previously approved these changes Sep 15, 2021

View reviewed changes

Formatting fix

1b6772a

joeljfischer dismissed FrankElias77’s stale review via 1b6772a September 16, 2021 16:54

joeljfischer merged commit ca6c31f into develop Sep 17, 2021

joeljfischer deleted the bugfix/issue-2018-choice-sets-fail branch September 17, 2021 18:50

joeljfischer mentioned this pull request Sep 21, 2022

Fix overwriting alert view images not being shown #2110

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Choice Set Manager to Fix Several Bugs #2019

Refactor Choice Set Manager to Fix Several Bugs #2019

joeljfischer commented Jul 6, 2021 •

edited

Loading

JulianKast Aug 26, 2021

joeljfischer commented Aug 26, 2021

Refactor Choice Set Manager to Fix Several Bugs #2019

Refactor Choice Set Manager to Fix Several Bugs #2019

Conversation

joeljfischer commented Jul 6, 2021 • edited Loading

Risk

Testing Plan

Unit Tests

Core Tests

Summary

Changelog

Bug Fixes

Tasks Remaining:

CLA

JulianKast Aug 26, 2021

Choose a reason for hiding this comment

joeljfischer commented Aug 26, 2021

joeljfischer commented Jul 6, 2021 •

edited

Loading