Skip to content
This repository has been archived by the owner on May 7, 2020. It is now read-only.

[Basic UI] Element Type Default for item of type Location or "How to render a PointType in a sitemap" #6110

Closed
cweitkamp opened this issue Aug 24, 2018 · 19 comments

Comments

@cweitkamp
Copy link
Contributor

cweitkamp commented Aug 24, 2018

How does it work? Element Type Default for item of type Location: "How to render a PointType in a sitemap" for Basic UI. This is what I have set up.

demo.items:

Location owmCurrentStationLocation { channel="openweathermap:weather:api:local:station#location" }

demo.sitemap:

sitemap test label="Local weather" {
    Frame label="OpenWeatherMap - Station" {
        Default item=owmCurrentStationLocation
        Text item=owmCurrentStationLocation
        Text item=owmCurrentStationLocation label="Location [%s]"
        Mapview item=owmCurrentStationLocation height=5
    }
}

What is working - the Mapview - but nothing else (state is 49.26,-123.19):

wetter

{
  "link": "http://localhost:8080/rest/items/owmCurrentStationLocation",
  "state": "49.26,-123.19",
  "stateDescription": {
    "readOnly": true,
    "options": []
  },
  "editable": false,
  "type": "Location",
  "name": "owmCurrentStationLocation",
  "tags": [],
  "groupNames": []
}
@lolodomo
Copy link
Contributor

@lolodomo
Copy link
Contributor

Maybe the channel should have a pattern with %s.

By the way, you show another problem because when you use a label with %s, the value is not correctly displayed. Maybe a problem with an unexpected character in the value that breaks the Basic UI code ?

@cweitkamp
Copy link
Contributor Author

cweitkamp commented Aug 24, 2018

By the way, you show another problem because when you use a label with %s, the value is not correctly displayed. Maybe a problem with an unexpected character in the value that breaks the Basic UI code ?

What do you mean exactly? I am afraid I did a mistake in the first post. My original sitemap contains label="Ort [%s]" instead of label="Location [%s]". I translated it while typing.

Maybe the channel should have a pattern with %s.

The result of a new test is not better. But I figured a pattern for the label which seems to work: label="Ort [%2$s°N,%3$s°W, %1$sm]". A quick Google search results in an interesting link to a international standard (ISO 6709) for a representation of latitude, longitude and altitude for geographic point locations.

demo.items:

Location owmCurrentStationLocation "Ort [%s]" { channel="openweathermap:weather:api:local:station#location" }

weather

@lolodomo
Copy link
Contributor

So it could be solved bu adding the right pattern definition to all channels associated to a Location item ?

@cweitkamp
Copy link
Contributor Author

In general, yes. But from my point of view we should reach an agreement on the "%s" pattern for Text elements. Resulting in an "0" is not an option. Is it?

And lets add some documentation about this specific pattern somewhere in the docs.

@lolodomo
Copy link
Contributor

You're right. There is a place where a default pattern is set if not defined. I did that for String and Number items if I correctly remember. We should add one for Location item.
I will search where it is defined...

@lolodomo
Copy link
Contributor

@lolodomo
Copy link
Contributor

The PR that introduced these default patterns is #3822.

@lolodomo
Copy link
Contributor

Note that it could make sense to replace the "Default" element from a Text widget to a Mapview widget in case of a Location item ?

@cweitkamp
Copy link
Contributor Author

cweitkamp commented Aug 25, 2018

I am not sure if we should define a global default pattern. I just want to make sure that we get a valid result for the pattern "%s". May both.

Does every available UI support the Mapview element? If so replacing the Default could be an option.

I use the system.location channel type in the OWM binding. A default pattern should be added to it.

https://github.com/eclipse/smarthome/blob/5ef015187d8c768312124d418e74feb282e9b69d/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/DefaultSystemChannelTypeProvider.java#L139-L146

@lolodomo
Copy link
Contributor

Ok, you're right, maybe not all UIs are already supporting mapview. So Default = Text looks reasonable.
Adding a pattern to the system channel is a good idea.
We should not have a lot of existing bindings defining channels loked to Location item.

@lolodomo
Copy link
Contributor

I found 7 OH2 bindings defining such channels. Only 3 of them are defining a pattern (air quality, sensebox and netatmo). Others are defining no pattern.

@cweitkamp
Copy link
Contributor Author

I created PR #6123 to address the pattern.

@lolodomo
Copy link
Contributor

I think we could define the same default pattern for the bindings that have not set a pattern in their Location channels, the same way we already have for String and Number channels.

@cweitkamp
Copy link
Contributor Author

Why not change the bindings to use the default system channel type for location instead?

I guess I have spotted why we see a "0" for the pattern "%s". Have a look at these lines of code:
https://github.com/eclipse/smarthome/blob/79d5ec5cebadafbdbe0fa2b76d7f3986116a96c9/bundles/core/org.eclipse.smarthome.core/src/main/java/org/eclipse/smarthome/core/library/types/PointType.java#L146-L158

We pass an array to the String.format() method which contains a sorted list of the constituents - first one is the altitude. The altitude in my example is zero.

If we want to have a framework-wide default pattern for the PointType we should implement it here - similar to the DateTimeType.

Just as a side not: The pattern parameter could be null. We have a missing Nullable annotation there.

@htreu
Copy link
Contributor

htreu commented Sep 4, 2018

In the code listed by @cweitkamp above we should fall back to the default pattern of the system channel in case the given pattern is null.
For %s as a pattern there are two valid options:

  1. Leave it as it is right now, only addressing the first element in the array. Users then have to know about the data format and provide the correct pattern by themselves.
  2. Replace %s with the default pattern because we think the user wanted to format the whole PointType. As a result users would have to use %1$s to address the altitude.

Nr 2 is my preference. Its the least surprising output to the user.

wdyt?

@cweitkamp
Copy link
Contributor Author

Sounds good. I prefer No. 2 as well.

Just as a side not: The pattern parameter could be null. We have a missing Nullable annotation there.

I have to revoke this: as mentioned in #5901 (comment) there is no specific reason for the annotation in this case and we should either remove it or add it on interface level -
combined with NPE safe-guard.

@cweitkamp
Copy link
Contributor Author

I submitted a PR #6190.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants