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.

Sign up for GitHub

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

Jump to bottom

Fixed HMI Uncaught SyntaxError after processing SetDisplayLayout #617

Merged
merged 7 commits into from
Sep 22, 2021
Merged

Fixed HMI Uncaught SyntaxError after processing SetDisplayLayout #617

merged 7 commits into from
Sep 22, 2021

Conversation

ValeriiMalkov
Copy link
Contributor

@ValeriiMalkov ValeriiMalkov commented Sep 20, 2021 •
edited by jacobkeeler
Loading

Fixes #609

This PR is ready for review.

Testing Plan

Manual testing

Summary

A DEFAULT available template is removed from the display capabilities because it's not defined on the HMI.
The expected result of the issue-609 must be replaced with an error result code.

CLA

ValeriiMalkov and others added 2 commits September 15, 2021 14:41
@ValeriiMalkov
Fixed issue: HMI Uncaught SyntaxError after processing SetDisplayLayo…
1897d6c 
…ut(displayLayout = DEFAULT) for media and non-media apps
Merge pull request #119 from LuxoftSDL/impl/fix/uncaught_syntax_error…
1dc30c7 
…_set_displaylayout

Fixed issue: HMI Uncaught SyntaxError after processing SetDisplayLayout(displayLayout = DEFAULT) for media and non-media apps
@ValeriiMalkov
Copy link
Contributor Author

Hi @theresalech This is a regression issue and PR is ready for Livio team review

@ShobhitAd
Copy link
Contributor

ShobhitAd commented Sep 20, 2021 •
edited
Loading

@ValeriiMalkov I'm not sure that removing "DEFAULT" from the available templates is the best solution. If displayLayout "DEFAULT" is sent in the SetDisplayLayout request, the HMI should select an appropriate template name based on the App HMI type or default to one (there is already a case for this https://github.com/smartdevicelink/sdl_hmi/blob/release/5.6.0/ffw/UIRPC.js#L517).

The reported error is caused by changes made in #590. https://github.com/smartdevicelink/sdl_hmi/blob/release/5.6.0/app/controller/sdl/Abstract/Controller.js#L1656 gets the template capabilities based on the template name (appModel.templateConfiguration.template). appModel.templateConfiguration.template is set to DEFAULT for this case and since there are no template capabilities for "DEFAULT" the lookup returns undefined.

I think for fix we might have to select the appropriate template name (as done in https://github.com/smartdevicelink/sdl_hmi/blob/release/5.6.0/ffw/UIRPC.js#L517) in the case that appModel.templateConfiguration.template is set to DEFAULT.

ValeriiMalkov and others added 4 commits September 21, 2021 13:16
@ValeriiMalkov
Fixed issue: HMI Uncaught SyntaxError after processing SetDisplayLayo…
45d586c 
…ut(displayLayout = DEFAULT) for media and non-media apps
@ValeriiMalkov
Revert "Fixed issue: HMI Uncaught SyntaxError after processing SetDis…
d61e9bb 
…playLayout(displayLayout = DEFAULT) for media and non-media apps"

This reverts commit 1897d6c.
@ValeriiMalkov
Fixed issue after code review: Uncaught SyntaxError
9a6f009 
after processing SetDisplayLayout(displayLayout = DEFAULT)
for media and non-media apps
Merge pull request #124 from LuxoftSDL/review/uncaught_syntax_error_s…
1c49d10 
…et_displaylayout

Review/uncaught syntax error set displaylayout
ShobhitAd
ShobhitAd requested changes Sep 21, 2021
View reviewed changes
app/model/sdl/Abstract/Model.js Outdated
@@ -628,7 +628,6 @@ SDL.SDLModel = Em.Object.extend({
return model.appType.indexOf('WEB_VIEW') >= 0;
}
case 'NON-MEDIA':
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
case 'NON-MEDIA':
case 'NON-MEDIA':
case 'DEFAULT':

Since getDisplayCapability can now handle the "DEFAULT" template, case DEFAULT should be re-added to the code.

If default is not re-added, the HMI will return a REJECTED response for SetDisplayLayout("DEFAULT")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, my mistake. I forgot to re-add it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes in commit 2048ee3

app/model/sdl/Abstract/data.js Outdated
@@ -1183,7 +1183,7 @@ SDL.SDLModelData = Em.Object.create(
],
"imageTypeSupported": ["STATIC", "DYNAMIC"],
"numCustomPresetsAvailable": 8,
"templatesAvailable": ["MEDIA", "NON-MEDIA", "DEFAULT", "NAV_FULLSCREEN_MAP", 'WEB_VIEW'],
"templatesAvailable": ["MEDIA", "NON-MEDIA", "NAV_FULLSCREEN_MAP", 'WEB_VIEW'],
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
"templatesAvailable": ["MEDIA", "NON-MEDIA", "NAV_FULLSCREEN_MAP", 'WEB_VIEW'],
"templatesAvailable": ["MEDIA", "NON-MEDIA", "DEFAULT", "NAV_FULLSCREEN_MAP", 'WEB_VIEW'],

Since getDisplayCapability can now handle the "DEFAULT" template, "DEFAULT" should be re-added to the available templates.

Also removing items from templateAvaible may impact the mobile libraries (if the managers rely on display capabilities to send SetDisplayLayout requests)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes in commit 2048ee3

ffw/UIRPC.js Outdated
@@ -1087,7 +1087,7 @@ FFW.UI = FFW.RPCObserver.create(
],
'graphicSupported': true,
'imageCapabilities': ['DYNAMIC', 'STATIC'],
'templatesAvailable': ["MEDIA", "NON-MEDIA", "DEFAULT", "NAV_FULLSCREEN_MAP", 'WEB_VIEW'],
'templatesAvailable': ["MEDIA", "NON-MEDIA", "NAV_FULLSCREEN_MAP", 'WEB_VIEW'],
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
'templatesAvailable': ["MEDIA", "NON-MEDIA", "NAV_FULLSCREEN_MAP", 'WEB_VIEW'],
'templatesAvailable': ["MEDIA", "NON-MEDIA", "DEFAULT", "NAV_FULLSCREEN_MAP", 'WEB_VIEW'],

Since getDisplayCapability can now handle the "DEFAULT" template, "DEFAULT should be re-added to the available templates

app/controller/sdl/Abstract/Controller.js Outdated
Comment on lines 1658 to 1661
if(appModel.appType.includes('NAVIGATION')) {
template = 'NAV_FULLSCREEN_MAP';
} else if(appModel.appType.includes('WEB_VIEW')) {
template = 'WEB_VIEW';
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
if(appModel.appType.includes('NAVIGATION')) {
template = 'NAV_FULLSCREEN_MAP';
} else if(appModel.appType.includes('WEB_VIEW')) {
template = 'WEB_VIEW';
if(appModel.appType.includes('WEB_VIEW')) {
template = 'WEB_VIEW';
} else if(appModel.appType.includes('NAVIGATION') || appModel.appType.includes('PROJECTION')) {
template = 'NAV_FULLSCREEN_MAP';

I think that WEB_VIEW should take precedence over NAVIGATION to match the behavior in the generic hmi https://github.com/smartdevicelink/generic_hmi/blob/develop/src/js/Controllers/BCController.js#L51

Also NAV_FULLSCREEN_MAP is used for both NAVIGATION and PROJECTION app types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changes in commit 2048ee3

@ShobhitAd ShobhitAd merged commit 1f52391 into smartdevicelink:release/5.6.0 Sep 22, 2021
@ShobhitAd ShobhitAd mentioned this pull request Sep 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShobhitAd ShobhitAd ShobhitAd approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ValeriiMalkov @ShobhitAd