[Accepted with Revisions] Revise SDL-0305 Homogenize TextFieldName

Question

[Accepted with Revisions] Revise SDL-0305 Homogenize TextFieldName

jordynmackool opened this issue 4 years ago · 19 comments

Hello SDL community,

The review of "Revise SDL-0305 Homogenize TextFieldName" began on January 12, 2021, and continues through January 26, 2021.

This will be a review of proposed revisions to a previously accepted but not yet implemented proposal, SDL 0305.

The pull request outlining the revisions under review is available here:

#1109

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

#1113

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

Is the problem being addressed significant enough to warrant a change to SDL?
Does this proposal fit well with the feel and direction of SDL?
If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Jordyn Mackool

Program Manager - Livio
jordyn@livio.io

Answer 1 · 2021-01-15T11:01:36.000Z

As described navigationText and turnText refer to the same text field, and turnText is the only one of the two that is used in SDL Core, so navigationText is redundant and can be removed.

I'm not sure if this statement is correct neither is the original proposal correct in the statement regarding turnText. The text field turnText does not exist in any of the SDL APIs. It only appears in the HMI_API as a named text field but again, there's no RPC or parameter using this text field according to the API documents. However, I found this in SDL Core https://github.com/smartdevicelink/sdl_core/blob/master/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/update_turn_list_request.cc#L124 which loops through turnList and basically renames navigationText to turnText in Turn elements in the list. I must be honest with you, I don't understand why Core is doing this but the parameter name is not in the SDL APIs. As long as this is Core internal it's fine but the structure should not leave or to the HMI or mobile with this parameter name.

There are only two options to proceed with this proposal:

Option 1: Remove/not add turnText and add/use navigationText in the Turn struct as specified in both HMI and Mobile API. Make sure the text field name navigationText is listed in TextFieldName. Optionally remove turnText related code in Core and use navigationText, basically wipe out turnText from SDL. This option is the least invasive solution to SDL as it won't cause deprecation or anything.

Option 2: Remove navigationText as proposed but to be consistent add turnText as the replacement. This requires the deprecation of the parameter in the mobile API.

Option 1 is the preferred solution because the latter causes an unnecessary deprecation issue to apps and the app, library and Core must deal with backwards compatibility.

Answer 2 · 2021-01-15T16:34:39.000Z

Hello @kshala-ford, thank you for your comment.

The code you linked from SDL Core is the root of my misunderstanding and why this revision was created. It is not obvious, but the UpdateTurnList parameter turnList uses type Turn which has different parameters in either API.

MOBILE_API.xml

<struct name="Turn" since="2.0">
    <param name="navigationText" type="String" maxlength="500" mandatory="false">
        <description>Individual turn text.  Must provide at least text or icon for a given turn.</description>
    </param>
    <param name="turnIcon" type="Image" mandatory="false">
        <description>Individual turn icon.  Must provide at least text or icon for a given turn.</description>
    </param>
</struct>

HMI_API.xml

<struct name="Turn">
  <param name="navigationText" type="Common.TextFieldStruct" mandatory="false">
    <description>Uses navigationText from TextFieldStruct.</description>
  </param>
  <param name="turnIcon" type="Common.Image" mandatory="false">
  </param>
</struct>

Understanding these structures makes it easier to see what Core is actually doing in the code you linked: It is converting navigationText as a string parameter to a TextFieldStruct with TextFieldName turnText. That means turnText textFieldName is currently used in only Core to HMI UpdateTurnList RPC, but this textField may also appear in an HMI's capabilities, which will be shared with mobile. Since this TextFieldName may be sent to mobile as a capability, it should be included in the mobile API as well.

This proposal with this revision would be very similar to your option 2: just add turnText, and leave navigationText out.
Luckily, this proposal only requires additions to the mobile API, no deprecation will be necessary.

Please let me know if you have any questions.

Answer 3 · 2021-01-18T09:32:41.000Z

@iCollin thank you for your comment! I didn't look into the parameter type. I still believe the option 1 is the better way to go because of the naming in the APIs.

turnText doesn't exist in the Mobile API which is why it doesn't make sense adding it in the text field list. Also removing navigationText is a problem as it would introduce inconsistency because this field exists in Turn in the Mobile API. turnText is not part of the HMI API hence Core must not send Turn with this parameter but with navigationText. The HMI implementation is expecting navigationText in Turn and using turnText is not documented.

In the HMI_API a Turn object should refer to an instance of TextFieldStruct where the Turn reference and the field name are navigationText. Using turnText requires an update of the HMI API and technically would cause a breaking change for SDL integrators.

Answer 4 · 2021-01-19T15:27:44.000Z

@kshala-ford I think there is still some misunderstanding with current state of SDL Core. Thank you for your comment.

Currently when Core sends a turn list to the HMI it will be using the TextFieldName turnText. The UpdateTurnList RPC does not anywhere use TextFieldName navigationText. (and nor does any other part of sdl_core or sdl_hmi)

turnText doesn't exist in the Mobile API which is why it doesn't make sense adding it in the text field list

I disagree, for the following reasons:

it does exist in the HMI API
it is used in UpdateTurnList
it can be sent to Mobile as part of capabilities, and for Mobile to understand the TextFieldName, it must be in the Mobile API.

removing navigationText is a problem as it would introduce inconsistency because this field exists in Turn in the Mobile API.

This isn't the case. Currently in the Mobile API Turn does not contain any TextFields. It contains a parameter of type String named navigationText.

turnText is not part of the HMI API hence Core must not send Turn with this parameter but with navigationText. The HMI implementation is expecting navigationText in Turn and using turnText is not documented.

Currently Core is sending Turn with turnText, and the HMI is expecting as such. This TextFieldName has been in the HMI API for a while (presumably at least as long as UpdateTurnList), and you can see Core setting the name here. navigationText is not used anywhere within sdl_core or sdl_hmi as a TextFieldName.

Does this help explain why I am proposing something more close to your Option 2? Please let me know if you have any more questions.

Answer 5 · 2021-01-19T19:34:50.000Z

I think that the best arguments for adding turnText to the rpc_spec instead of adding navigationText are the following:

navigationText is confusing because there's also navigationText1 and navigationText2 and to avoid confusion we should use a better name than the actual parameter name. This has precedent with SubtleAlert's TextFieldNames for example.
This is already in use in HMI and Core as turnText so we should avoid creating a translation layer in Core if possible.

I do think that the description should be something like: Declares whether Turn.navigationText is supported.

Answer 6 · 2021-01-19T19:39:15.000Z

To maybe clear some confusion about usage in core, here is the snippet of turnText used as the TextFieldName while the string parameter is named navigationText.

https://github.com/smartdevicelink/sdl_core/blob/master/src/components/application_manager/rpc_plugins/sdl_rpc_plugin/src/commands/mobile/update_turn_list_request.cc#L125-L134

Answer 7 · 2021-01-20T14:24:30.000Z

I agree that by naming convention Turn.navigationText should have been named Turn.turnText, however the proposal and the revision are not complete to the steps to this change. The consequences of the newly named parameter are deprecations and breaking changes to the HMI API. I already made this clear in my previous comments.

If you believe the new name is worth the effort and pain of deprecation then please make this clear in the proposal. Right now it's not clear in this point and I want to avoid the situation where the SDLC is called again for an emergency poll during development asking if a major version bump is accepted.

Both, Mobile and HMI API do not include turnText anywhere. Adding turnText to the Mobile API requires at least a minor version bump. Same for deprecating navigationText in Turn. @joeljfischer please tell me that your proposal doesn't require a translation layer. FYI, you will need to have both text field names turnText and navigationText in the enum.

Adding turnText in the HMI API to replace navigationText will be a breaking change to OEM integrators which means we, also Ford must pay for a change request in order to upgrade Core in the IVI.

@iCollin @joeljfischer are not considering the impact to the APIs when changing the name. You need to see both APIs and the Core implementation separately. Core's implementation sending Turn.turnText (which contains details to mobile's Turn.navigationText) to the IVI through the HMI API is an API violation. According to the HMI API, Core must name the parameter navigationText containing the text of navigationText. I don't understand why we're having this debate.

I would appreciate if the author and the PM describes the required proposal revisions for the following items:

Respect the Mobile API versioning to introduce turnText. Including adding a new parameter, deprecate the old one and have both fields listed in the enum.
Describe Core behavior processing Turn with respect to navigationText and turnText
In HMI API: Specify Turn to have turnText as the parameter of type TextFieldStruct holding the content of the paramter the mobile sent. Describe the impact for OEMs to integrate this change.

Answer 8 · 2021-01-20T14:25:56.000Z

The Steering Committee voted to keep these proposed revisions in review on 2021-01-19 to allow for conversation to continue on the open items.

Answer 9 · 2021-01-20T14:36:37.000Z

@kshala-ford First, please note that altering HMI_API, even in a breaking manner, does not change the versioning of Core. This has already been decided and agreed upon by the steering committee in previous releases. Second, because this TextFieldName is already turnText in the HMI_API, I don't see how this would require changes to the HMI_API.

Both, Mobile and HMI API do not include turnText anywhere.

This is untrue. Search turnText in the HMI_API.

Answer 10 · 2021-01-20T14:50:01.000Z

@joeljfischer I know the existance of turnText in the enum as I already mentioned many times but this doesn't make it appear in the communication between Core and HMI. The HMI API must contain the parameter in Turn. I can't find this being the case in the API. See below

<struct name="Turn">
  <param name="navigationText" type="Common.TextFieldStruct" mandatory="false">
    <description>Uses navigationText from TextFieldStruct.</description>
  </param>
  <param name="turnIcon" type="Common.Image" mandatory="false">
  </param>
</struct>

Also I never said it changes the versioning of Core. I never mentioned to bump up the Core version. However the HMI API change to rename navigationText to turnText is a breaking change for us when upgrading Core to a version that implements this proposal.

Answer 11 · 2021-01-20T16:02:43.000Z

The HMI does not send turnText or navigationText as supported TextFields, and therefore, neither does Core. Apps do not know which TextFields of these two are supported as neither exist in the RPC Spec.
The developer does not currently check if the TextField exist for the Turn struct because they do not have a way to do so per the previous point. Therefore this will not break their integration and will only enhance it.
Core currently transforms the navigationText param in the Turn struct into a TextFieldStruct (see here) with TextFieldName value of turnText with a param fieldText with value of the navigationText string from the Turn struct before it sends it to the HMI. Therefore, the TextFieldName value navigationText is currently never sent to the HMI regardless of what is initially received by Core from the app.

textFieldStruct: {
  fieldName: "turnText",
  fieldText: Turn.navigationText //string variable
}

This transformation has been part of the Core code base for 5 years now from this commit. It appears to have been a modification from Luxoft on Ford's behalf. It has what appears to be internal issue tracking for Ford so it might be possible to hunt down what those were and understand why this change took place. Because of this I imagine that Ford's HMIs are actually aligned with the changes proposed in this proposal and revisions.
In general, navigationText is confusing when there is also navigationText1 and navigationText2 as TextFieldName values. As per Joel's comments, we have precedent that the TextFieldName does not explicitly have to match the param name in a struct. For this change we simply need to update the descriptions.

Answer 12 · 2021-01-20T17:13:39.000Z

@joeygrover

The HMI does not send turnText or navigationText as supported TextFields, and therefore, neither does Core. Apps do not know which TextFields of these two are supported as neither exist in the RPC Spec.

Can you elaborate which HMI you mean? Adding turnText in the list of supported text fields doesn't make sense because there is no RPC that requires this field to be used. So I bet no HMI sends turnText. Regarding navigationText it is definitely HMI specific and depends if this text field, the RPC and the actual feature (turn list) is implemented. Your statement is only valid if you have access to all SDL supported IVIs on the road.

The developer does not currently check if the TextField exist for the Turn struct because they do not have a way to do so per the previous point. Therefore this will not break their integration and will only enhance it.

How do you know that? Why should the developer do that? Common practice for developers is to fill out the desired parameters in RPCs and send them. The managers in the libraries started the consideration of the capabilities. The developer were never required to listen to the capabilities to know which text fields are supported. This is a thing added far later by the PM. I don't disagree with the additional capability checks. I want to say that apps exist setting navigationText according to the description in the mobile API

        <param name="navigationText" type="String" maxlength="500" mandatory="false">
            <description>Individual turn text.  Must provide at least text or icon for a given turn.</description>
        </param>

Core currently transforms the navigationText param in the Turn struct into a TextFieldStruct (see here) with TextFieldName value of turnText with a param fieldText with value of the navigationText string from the Turn struct before it sends it to the HMI.

I already described this in my comments twice. To be clear, this renaming is undocumented and should not have happened.

Therefore, the TextFieldName value navigationText is currently never sent to the HMI regardless of what is initially received by Core from the app.

Whatever the field is called within the TextFieldStruct, it must be listed in TextFieldName. Core sets it to "turnText" but according to the APIs it should be "navigationText". See HMI API:

  <param name="navigationText" type="Common.TextFieldStruct" mandatory="false">
    <description>Uses navigationText from TextFieldStruct.</description>
  </param>

If your plan is to keep the Struct parameter to be navigationText but only it's content to refer to a turnText then I don't have any concerns except that this should be covered in the integration guidelines.

This transformation has been part of the Core code base for 5 years now from this commit. It appears to have been a modification from Luxoft on Ford's behalf.

I think you have to blame beyond that date because this commit isn't changing the logic of the lines in question. Honestly, I am not looking to blame who wrote this code. I wouldn't make assumptions on how a CR was implemented. Note the XML comment TODO to remove turnText? There are many ways to fix this API mistake. Making decisions based on assumptions was never an option for the PM. At least it wasn't for past proposals. I remember how you fought for a stale API because there could be nonofficial integrations of Core by non SDLC members and their integration could then break. What happened since then?

In general, navigationText is confusing when there is also navigationText1 and navigationText2 as TextFieldName values. As per Joel's comments, we have precedent that the TextFieldName does not explicitly have to match the param name in a struct. For this change we simply need to update the descriptions.

So the alternative is to add turnText to TextFieldName which refers to Turn.navigationText and describe how they refer to each other? How is this less confusing? I don't think this is a good solution for this specific case. The libraries should fully encapsulate Turn before I explain this to app developers or consider a new turnText parameter in Turn deprecating navigationText.

Answer 13 · 2021-01-20T18:27:03.000Z

If your plan is to keep the Struct parameter to be navigationText but only it's content to refer to a turnText then I don't have any concerns except that this should be covered in the integration guidelines.

Yes, that is the idea, we don't intend to change any names. Good point that the behavior should be made clear in the HMI integration guidelines.

The HMI does not send turnText or navigationText as supported TextFields, and therefore, neither does Core. Apps do not know which TextFields of these two are supported as neither exist in the RPC Spec.

Can you elaborate which HMI you mean? Adding turnText in the list of supported text fields doesn't make sense because there is no RPC that requires this field to be used.

turnText is the field name currently used by sdl_hmi and sdl_core for the UpdateTurnList RPC, but since this field does not exist in the Mobile API, an HMI cannot provide that field to mobile within its' capabilities. Even if an HMI were to include this field in their capabilities, Mobile would receive the text field with a value of -1 INVALID_ENUM.

Core sets it to "turnText" but according to the APIs it should be "navigationText"

Core is following the spec and setting the parameter name to navigationText, and then setting navigationText.fieldName to turnText. Here is a log from an HMI receiving an UpdateTurnList RPC on master (7.0), currently core is using fieldName: turnText

SDL -> HMI: {"id":42,"jsonrpc":"2.0","method":"Navigation.UpdateTurnList","params":{"appID":1,"turnList":[{"navigationText":{"fieldName":"turnText","fieldText":"turn left"},"turnIcon":{"imageType":"STATIC","value":"0xCD"}},{"navigationText":{"fieldName":"turnText","fieldText":"turn right"},"turnIcon":{"imageType":"STATIC","value":"0xFF"}}]}}

Answer 14 · 2021-01-21T08:20:28.000Z

turnText is the field name currently used by sdl_hmi and sdl_core for the UpdateTurnList RPC, but since this field does not exist in the Mobile API, an HMI cannot provide that field to mobile within its' capabilities. Even if an HMI were to include this field in their capabilities, Mobile would receive the text field with a value of -1 INVALID_ENUM.

That's absolutely correct. The specific text field capabilities can be missing but permission to the RPC is known. Also API descriptions say that at least one field must be used. Your statement is no evidence that the field is not used by any application. Check out the mobile API parameter description.

Core is following the spec and setting the parameter name to navigationText, and then setting navigationText.fieldName to turnText

Again, I totally get that. But according to the API it shouldn't be the case. Check out the description.

  <param name="navigationText" type="Common.TextFieldStruct" mandatory="false">
    <description>Uses navigationText from TextFieldStruct.</description>
  </param>

The original intention of the proposal is to make sure the APIs match how they should be matching. Now we know the current implementation is different and we must workaround the fact with tricks to overcome versioning and compatibility problems. I don't think the proposal or the revision is transparent enough to understand the real impact to existing code. All your workarounds would work but I don't believe this is a clean and solid work API if we hide turnText in navigationText and bend practice of field naming for any need. It's one thing to use subtleAlertSoftButtonText for naming but another to use a completely different name. Following the naming of previous proposals the text field name in the mobile API should be turnNavigationText.

Answer 15 · 2021-01-21T16:58:10.000Z

Good point, the Turn.navigationText description in the HMI API could be updated as well.

Which part of this proposal are you seeing as a workaround?

I don't believe this is a clean and solid work API if we hide turnText in navigationText and bend practice of field naming for any need.

The current behavior is not ideal, but that behavior is not part of this proposal and has existed in SDL Core for years. This proposal is just to add missing text field names to the Mobile API and then to align the two enums. Is the concern that people have modified SDL Core to use navigationText text field name in UpdateTurnList?

Answer 16 · 2021-01-26T18:57:20.000Z

Since some time has passed and no response has been given on the last comment, the Project Maintainer requests the Steering Committee moves to vote on this proposal as the changes impact the April 14, 2021 releases.

It is requested that the proposed revisions to SDL 0305 are accepted with the below revisions:

1. Keep the Turn struct parameter navigationText using text field name turnText and clarify this behavior in the HMI integration guidelines.

2. Update the HMI API description for Turn.navigationText to mention "turnText".

Answer 17 · 2021-01-27T14:29:49.000Z

The Steering Committee voted on 2021-01-26 to accept this revised proposal with the revisions summarized in this comment.

Answer 18 · 2021-01-27T14:31:17.000Z

@iCollin please advise when a new PR has been entered to update the proposal to reflect the agreed-upon revisions. I'll then merge the PR so the proposal is up to date, and comment on the already created issues for the impacted repositories. Thanks!

Answer 19 · 2021-01-27T21:48:12.000Z

The proposal has been updated based on the Steering Committee's agreed-upon revisions. Comments have been left on implementation issues to reference revisions: