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

Whitespace fields in show command being rejected #5

Closed
mikeburke106 opened this issue Oct 29, 2014 · 14 comments
Closed

Whitespace fields in show command being rejected #5

mikeburke106 opened this issue Oct 29, 2014 · 14 comments
Labels

Comments

@mikeburke106
Copy link

In the Livio SDL Tester application, a Show message is automatically sent once the HMI is updated to HMI_FULL to display the initial text the user will see when the app is connected. This used to work fine in previous versions of the emulator, but the same JSON is now being rejected in the most recent version on github (3.8?).

Here's the JSON:

{
  "request":{
    "correlationID":102,
    "parameters":{
      "mainField3":" ",
      "alignment":"LEFT_ALIGNED",
      "mainField1":"Livio SDL Tester",
      "graphic":{
        "value":"App Icon",
        "imageType":"DYNAMIC"
      },
      "mainField2":"Send SDL Commands"
    },
    "name":"Show"
  }
}

Anybody see anything wrong in the JSON string? I'll keep experimenting and see if I can find what data is invalid.

Also, see attached screenshots:

screenshot_2014-10-29-13-57-02
screenshot_2014-10-29-13-57-08

@mrapitis
Copy link
Contributor

The value for graphic needs to be a valid graphic (as opposed to “App Icon”) that was previously sent with putfile since the imagetype is dynamic.

@mikeburke106
Copy link
Author

An image named "App Icon" was sent through PutFile and received PutFileResponse with success=true prior to sending this message. The image shows up correctly via SetAppIcon command, and if I just do a Show with the same graphic, it works correctly. So, I don't think the image is the problem.

@mikeburke106
Copy link
Author

Aha, I just tried testing with mainField3 = " " and got INVALID_DATA, so I think we found the culprit.

{
  "request":{
    "correlationID":102,
    "parameters":{
      "mainField3":" "
    },
    "name":"Show"
  }
}

In order to clear out mainField3, I am sending a " ". In previous versions of core, this command cleared out that particular field; however, now the message is being rejected due to this.

I believe this is a bug because now there is currently no way to clear Show fields...

@mrapitis
Copy link
Contributor

Not sure if it’s a bug, but show with double quotes and an empty string will clear the field. "mainField3":""

@mikeburke106
Copy link
Author

Ok, I confirmed that an empty string ("") does clear the field.

Do you know if there's a reason whitespace (" ") used to work in previous versions of core, but the message is rejected now? Was it a conscious decision to make it INVALID_DATA, or perhaps a bug was introduced in the most recent changes?

Whitespace seems like a logical string an app might send, so I would still consider this a bug (from an app-developer point of view), because I would expect whitespace to clear out the string in this case. Thanks for the info.

@mrapitis
Copy link
Contributor

It sounds like a bug since gen 1.1 will clear the field when using (“ “) or (“”). We would need to confirm with the module team to make sure the change was not intentional though (unlikely that it was).

@mikeburke106 mikeburke106 changed the title Show command is being rejected Whitespace fields in show command being rejected Oct 29, 2014
@jmarchwi-ford
Copy link

@mikeburke106 It's likely a bug -- whitespace should be rejected. However, two quotes with no empty whitespace should be accepted to clear the field. Understand your point about whitespace being a valid way to clear a field, but conceptually this just becomes a field with whitespace. In order to eliminate strange behaviors or undesired behaviors of fields and HMI, we simply eliminated whitespace characters \t, \n, etc. to avoid any issues.

SDL has been getting more strict in recent releases, so I think this is just improvements to the software over time.

@mikeburke106
Copy link
Author

Just to clarify, whitespace should be rejected? The current version of core on GitHub is rejecting whitespace, so if that's the design intent this bug should be closed.

I still see this as a bug, however, because spaces are valid string characters, so I think it will be confusing for developers when they send a valid string and get INVALID_DATA back... maybe the mobile libraries should handle this - if the string only contains whitespace, set it to ""?

@jmarchwi-ford
Copy link

Yeah, that's an appropriate correction to make -- strings made up of "only whitespace" would be rejected. Strings with control characters like tabs and newlines would also be rejected. Normal strings with spaces should be accepted.

I'm not sure if a string like, "This is a test" or " This is a test " would be valid or just trimmed.

@stefanek22
Copy link

By design (including legacy design), strings with whitespace only or with leading whitespace are rejected. The intention has been to avoid an app trying to format on-screen text to fit a given platform.

Of course the need to clear fields is necessary, hence the blank ("") strings are accepted.

While we have avoided adding logic in the proxy to reformat parameters (like your proposed trimming of whitespace in some cases). However, that can still be considered since it aids the developer (and still avoids invalid data being sent).

@mikeburke106
Copy link
Author

I tested the following strings:

  • "Test"
  • " Test"
  • "Test "
  • "       Test"

And can confirm that core displayed "Test" for each one of these inputs. So, if there is more than whitespace in the text, the leading/trailing whitespace is trimmed off on the core-side - which is what I would expect in this case.

If leading whitespace is supposed to be rejected, we still have a bug in this case because all of the above inputs were accepted and formatted correctly.

Again, just to be clear, everyone agrees that this could be solved on the proxy-side if we trim leading/trailing whitespace from the user inputs prior to sending?

@stefanek22
Copy link

Then SDL is already employing some of the trimming actions to avoid forced formatting.

The solution was done core-side previously to keep the proxy thinner and to avoid malicious behavior.

@mikeburke106
Copy link
Author

Lol, ok - sounds good. Do we consider not rejecting strings with leading whitespace a bug? I personally think it works better as it is - in which case everything meets design intent and this issue can be closed. Agreed?

@stefanek22
Copy link

Yes, agreed! 👍

dev-gh pushed a commit to dev-gh/sdl_core that referenced this issue Apr 15, 2016
…nager_assert

Fixes assert on policy update status changes
dev-gh pushed a commit to dev-gh/sdl_core that referenced this issue Jun 7, 2016
…nager_assert

Fixes assert on policy update status changes
Related-issues: APPLINK-22713, APPLINK-22716
VVeremjova pushed a commit to VVeremjova/sdl_core that referenced this issue Sep 7, 2016
SetGlobalProperties
SetMediaClock
ShowConstantTBT
Show

Related issue: APPLINK-25918
VVeremjova pushed a commit to VVeremjova/sdl_core that referenced this issue Sep 8, 2016
SetGlobalProperties
SetMediaClock
ShowConstantTBT
Show

Related issue: APPLINK-25918
anton-hrytsevich pushed a commit to anton-hrytsevich/sdl_core that referenced this issue Nov 17, 2016
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants