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

Add widgets API updates #185

Merged

Conversation

mked-luxoft
Copy link
Contributor

Adds updates to API according #2918

MOBILE_API.xml Outdated
<element name="templateTitle" since="6.0">
<description>The title of the new template that will be displayed; applies to "Show"</description>
</element>

Copy link
Contributor

Choose a reason for hiding this comment

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

This PR should only include widget related changes. Please remove this element from the enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 086a52f

MOBILE_API.xml Outdated
@@ -871,11 +897,11 @@
<element name="locationImage" since="4.0">
<description>The optional image of a destination / location</description>
</element>

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this delta is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in 086a52f

MOBILE_API.xml Outdated
<element name="alertIcon" since="6.0">
<description>The image field for Alert</description>>
</element>

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this delta is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed in 086a52f

@kshala-ford
Copy link
Contributor

This PR implements #164 SDL 0216 Widget Support. @mked-luxoft I reviewed the file. There seems to be an element from another proposal implementation. Please have a look at the review and let me know if you have any questions.

@Jack-Byrne
Copy link
Collaborator

@mked-luxoft There is a difference in the number of line changes between this PR and the mobile_api.xml changes in core.

Can you please verify that all fields are correct? I went through and see that <struct name="DisplayCapabilities" deprecated="true" since="6.0"> is missing from this PR.

The pr for these changes should try to be an exact copy of the changes in the core pr (or vice versa).

Copy link
Collaborator

@Jack-Byrne Jack-Byrne left a comment

Choose a reason for hiding this comment

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

Core's mobile_api.xml changes should align with this pr for the widgets implementation.

@theresalech
Copy link
Contributor

@mked-luxoft can you please let us know if you need any more information from us in order to make these changes?

@theresalech
Copy link
Contributor

@mked-luxoft following up here. Please advise when this is ready for re-review. Thanks!

@mked-luxoft
Copy link
Contributor Author

@mked-luxoft There is a difference in the number of line changes between this PR and the mobile_api.xml changes in core.

Can you please verify that all fields are correct? I went through and see that <struct name="DisplayCapabilities" deprecated="true" since="6.0"> is missing from this PR.

The pr for these changes should try to be an exact copy of the changes in the core pr (or vice versa).

@JackLivio added and updated in 8daa157

@mked-luxoft
Copy link
Contributor Author

Core's mobile_api.xml changes should align with this pr for the widgets implementation.

@JackLivio i checked the changes and they seem to be the same as in core PR. the difference in line changes might be explained by different versions of file the PRs are based on.

@mked-luxoft
Copy link
Contributor Author

@mked-luxoft following up here. Please advise when this is ready for re-review. Thanks!

@theresalech sorry for delay, this pr is ready for review

MOBILE_API.xml Outdated
@@ -3243,6 +3342,7 @@

<struct name="RadioControlCapabilities" since="4.5">
<description>Contains information about a radio control module's capabilities.</description>
<!-- need an ID in the future -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this

MOBILE_API.xml Outdated
@@ -3328,6 +3428,7 @@

<struct name="ClimateControlCapabilities" since="4.5">
<description>Contains information about a climate control module's capabilities.</description>
<!-- need an ID in the future -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed on a8b4b19

MOBILE_API.xml Outdated
@@ -2881,6 +2968,7 @@
<element name="REMOTE_CONTROL"/>
<element name="APP_SERVICES" since="5.1"/>
<element name="SEAT_LOCATION" since="6.0"/>
<element name="DISPLAYS" since="6.0"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please Fix Spacing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in a8b4b19

@theresalech
Copy link
Contributor

@mked-luxoft please advise once you've addressed the PM's feedback, and this PR is ready for re-review. Thank you!

@theresalech
Copy link
Contributor

@mked-luxoft, can you please let us know once this feedback has been addressed? We would like to have this resolved before we share the Core 6.0 Release Candidate with the Steering Committee for review on 2019-10-04.

@mked-luxoft
Copy link
Contributor Author

@theresalech PR is ready for re review

Copy link
Contributor

@theresalech theresalech left a comment

Choose a reason for hiding this comment

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

@JackLivio can you please take a look at the questions I've left in my comments and advise?

Additionally, believe the following changes are still needed, per the proposal. Please let me know if these were included and I missed them, or if they are not needed.

  • The notification OnHMIStatus should be extended to address a specific window.
  • Add windowID and templateConfiguration params to Show

Please request changes from the author if needed. Let me know if you need any more information from me, thanks!

MOBILE_API.xml Show resolved Hide resolved
MOBILE_API.xml Outdated
<param name="presetBankCapabilities" type="PresetBankCapabilities" mandatory="false" deprecated="true" since="6.0">
<description>
If returned, the platform supports custom on-screen Presets; see PresetBankCapabilities.
This parameter is deprecated and replaced by SystemCapability using DISPLAYS.
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
This parameter is deprecated and replaced by SystemCapability using DISPLAYS.
This parameter is deprecated and replaced by SystemCapability using DISPLAYS.

please remove extra space

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

MOBILE_API.xml Show resolved Hide resolved
MOBILE_API.xml Show resolved Hide resolved
@Jack-Byrne
Copy link
Collaborator

@theresalech

The notification OnHMIStatus should be extended to address a specific window.

I believe that parameter is added "windowID".

Add windowID and templateConfiguration params to Show

Those were added here

Copy link
Contributor

@theresalech theresalech left a comment

Choose a reason for hiding this comment

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

@mked-luxoft I've left a couple minor comments for you, regarding spacing and history tags. Please let me know if you have any questions or concerns with addressing. Thanks!

@mked-luxoft
Copy link
Contributor Author

@theresalech comments have been addressed in 6aa7613, pr is ready for re-review

@joeygrover joeygrover merged commit 6aa7613 into smartdevicelink:version/6_0_0 Oct 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants