smartdevicelink / sdl_core

SmartDeviceLink In-Vehicle Software and Sample HMI
BSD 3-Clause "New" or "Revised" License
241 stars 244 forks source link

Prevent resumption of default keyboard properties after reset #3633

Closed ychernysheva closed 3 years ago

ychernysheva commented 3 years ago

Fixes #3610

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Covered by ATF test script

Summary

There is an issue when after reset of keyboard properties previously saved values are still resumed instead of default ones. Root cause of this situation is ignoring of keyboard properties cache during reset of keyboard properties. Previously this cache wasn't emptied during reset, so it continued to store outdated application defined data. This cache is also used as data source for resumption, that's why we need to empty this cache after reset, because we don't need to re-send requests with default keyboard properties.

CLA

ychernysheva commented 3 years ago

@JackLivio This PR is ready for review. Thank you!

Jack-Byrne commented 3 years ago

@ychernysheva We are including a similar fix in this pr: https://github.com/smartdevicelink/sdl_core/pull/3635/files

ychernysheva commented 3 years ago

@JackLivio Thank you for your response! It seems that after proposed changes from https://github.com/smartdevicelink/sdl_core/pull/3635/files this defect is still actual (I've checked it locally). Maybe it's possible to replace https://github.com/smartdevicelink/sdl_core/pull/3635/files#diff-59bdfbd55b32b2dae488584440e0d7a594ca6176c26d15fbf479deecc8a7f999L410 with https://github.com/smartdevicelink/sdl_core/pull/3633/files#diff-59bdfbd55b32b2dae488584440e0d7a594ca6176c26d15fbf479deecc8a7f999L410 ?

Jack-Byrne commented 3 years ago

@ychernysheva Thank you, reconsidering this pr. Could maybe explain why clearing the map doesnt work the same as setting the value to null?

ychernysheva commented 3 years ago

@JackLivio If we clear this map, we will have following parameter in UI request during resumption: "keyboardProperties":{}. I guess that it is caused by the next code inside of resumption_data_db.cc file: https://github.com/smartdevicelink/sdl_core/blob/develop/src/components/application_manager/src/resumption/resumption_data_db.cc#L2449

Jack-Byrne commented 3 years ago

@ychernysheva Thank you for pointing that out.

Jack-Byrne commented 3 years ago

I confirmed the empty object issue caused by our previous fix, but im not sure this fix includes all necessary changes for the problem.

  1. I had a test app configured to use the AZERTY keyboard.
  2. Sent a resetGlobalProperties, here is the msg received by hmi 👍 Screen Shot 2021-02-18 at 1 46 57 PM
  3. I reset and resume the app with a valid hashid and this is the setGlobalProperties message received by the hmi
    Screen Shot 2021-02-18 at 1 47 10 PM

Should the messages in screenshot's 2&3 be the same?

However, I can see why being same should not be the case, reason being that Core does not send these default SGP parameters on an initial connect. Thoughts? @ychernysheva

edit: I decided I approve this behavior.