openhab / openhab-android

openHAB client for Android
https://play.google.com/store/apps/details?id=org.openhab.habdroid
Eclipse Public License 2.0
599 stars 317 forks source link

door and contact icons not working as expected with Contact items with OH2 #193

Closed lolodomo closed 7 years ago

lolodomo commented 8 years ago

"door" and "contact" icons are not correctly displayed with contact items when using OH 2 beta 3. Note that it works well with Classic UI so it is not a problem with my sitemap/items/icons. The displayed door icon is always an opened dioor. The displayed contact icon is always a closed window.

And note that it is working well with OH 1.8.3. So the problem is only with OH 2.

lolodomo commented 8 years ago

Additional information. The problem occurs with such defined items:

Contact PorteEntree "Porte entrée [MAP(fr.map):%s]" <door> (Entree,GSecurite,GPorteFenetre,GPowerMax) {powermax="zone_status:1"}
Contact FenetreCuisine "Fenêtre cuisine [MAP(fr.map):%s]" <contact> (Cuisine,GSecurite,GPorteFenetre,GPowerMax) {powermax="zone_status:9"}

If I suppress the MAP function in these 2 items, then the icons are correctly updated by HABdroid.

One time again, please note that HABdroid is working with these items and a 1.8.3 server. Just it does not work with 2.0 beta 3 server.

hazzeh commented 8 years ago

It might be related to #145, which implemented a new way of fetching the icons. I'll see if I can reproduce the error. You can check what the restapi returns for the item: openhaburl:port/rest/items/PorteEntree

hazzeh commented 8 years ago

I noticed that the restapi returns the value from the map, without preserving the "original" state. Which might be a problem. Also that as you say the webui (basic ui) uses the correct state when fetching the icon from the smarthome icon servlet.

lolodomo commented 8 years ago

Yes, the rest API returns as state value the result of the map transformation . Yes the icon is correctly displayed by Class UI and by Basic UI.

hazzeh commented 8 years ago

I don't know how the web ui works, if it uses rest api or something else for fetching items. Their icon urls are getting the correct state (prior translation). Maybe @kaikreuzer have some ideas how to fetch the untranslated state value?

kaikreuzer commented 8 years ago

I just tried to track this down. The problem is right here: https://github.com/eclipse/smarthome/blob/master/bundles/io/org.eclipse.smarthome.io.rest.core/src/main/java/org/eclipse/smarthome/io/rest/core/item/EnrichedItemDTOMapper.java#L46

This means that the REST API always returns the transformed state within sitemap responses and thus the raw state cannot be passed to the icon servlet. In openHAB 1 this was working afaik, because the state was not transformed within the item (and the transformed version was part of the label that the UI then had to parse).

What would you suggest how to solve this best? I actually think that the item within the widget response should show the raw version - but this will be a breaking change for the UIs, so we will need to adapt them. In order to still have the transformed version as well, we could add an additional field to the widget in the response.

lolodomo commented 8 years ago

@kaikreuzer: how do you explain that it is already working well in basic UI and Classic UI ?

kaikreuzer commented 8 years ago

Simple: They do not use the REST API for accessing sitemaps, but they have their own server side construction of HTML within a servlet.

lolodomo commented 8 years ago

So your suggestion is certainly excellent.

hazzeh commented 8 years ago

My initial thought is that the widget needs yet another field "raw state". If the "old" state start showing the untranslated value, it will might break other usage of the restapi, but it can be handled by first checking apiversion of the restapi etc.

It would not be that much of work to handle the new field in habdroid, and also update the method which fetches the icons to use the new field value. If you think that the original state value should show the raw value instead, I don't think much will break in habdroid, for example: some labels in the gui will show the untranslated value. I don't have any strong opinions of which state field that should contain the raw value. But since OH2 is still in beta it might not be the whole world if there was a breaking change, if you think that is the better solution.

kaikreuzer commented 8 years ago

@digitaldan & @belovictor: Question: Do we actually NEED the transformed state within the item that is associated with the widget? Since the "label" of the widget still contains the transformed state in square brackets, I assume that the apps anyhow use this information and do not require the transformed state in the item. So would it be ok to simply change it to the raw state?

digitaldan commented 8 years ago

To be honest, I have not looked at this, been slammed this week. I'm hoping to get some time over the weekend.

lolodomo commented 8 years ago

Any progess on that issue ?

digitaldan commented 8 years ago

I have not looked at this, but it sounds like we need an additional type added to the sitemap rest api in ESH?

kaikreuzer commented 8 years ago

@digitaldan See my question in https://github.com/openhab/openhab.android/issues/193#issuecomment-227486492 - if the transformed state is not needed in the item element, I would change it to be the raw state, so no "additions" would be needed in the REST API.

digitaldan commented 8 years ago

What would you suggest how to solve this best? I actually think that the item within the widget response should show the raw version - but this will be a breaking change for the UIs, so we will need to adapt them.

@kaikreuzer I'm a little confused/surprised on how this is just breaking images, the mobile clients afaik do not treat oh1 and oh2 item state any differently, so while displaying label values might not be different, I would think that trying to send a command based on a transformed value would cause other issues....maybe we have be (un)lucky so far?

In openHAB 1 this was working afaik, because the state was not transformed within the item (and the transformed version was part of the label that the UI then had to parse).

Was there a strong reason to transform the item state in oh2?

I will check the IOS side, but I don't think your suggestion will break anything for android.

kaikreuzer commented 8 years ago

I would think that trying to send a command based on a transformed value would cause other issues....maybe we have be (un)lucky so far?

Are there situations where you send commands based on the item state that you received? Not really, right?

Was there a strong reason to transform the item state in oh2?

Not that I am aware of, this rather came in by accident. That's why I suggested to get rid off it again.

digitaldan commented 8 years ago

Are there situations where you send commands based on the item state that you received? Not really, right?

For setpoints we display the label value, but when the +/- buttons are pressed we retrieve the item's state, increase or decrease it, then send that as a command to the server. I don't think there are a lot of other places where that happens.

lolodomo commented 7 years ago

Any progress ? This bug is annoying.

lolodomo commented 7 years ago

Any progress?

digitaldan commented 7 years ago

@kaikreuzer did we decide to make the item-> state a raw value in ESH ? If so would you like me to fix that? Would adding an additional item -> transformedState field to that object break clients? I think it would be nice to have both. I would hope most clients would ignore unknown fields

lolodomo commented 7 years ago

@kaikreuzer : as the timing is short for the final release, can you please answer to @digitaldan 's proposal ?

lolodomo commented 7 years ago

@kaikreuzer : sorry for insisting but could you answer please?

kaikreuzer commented 7 years ago

This is related to https://github.com/eclipse/smarthome/issues/2788.

I cannot really assess the implications if we now suddenly remove the mapping from the state. Although this sounds like the right solution to me, it also appears risky. So instead of breaking stuff, I would rather live with this as a known bug for the 2.0 release and come up with a well-conceived solution afterwards.

lolodomo commented 7 years ago

I have submitted PR https://github.com/eclipse/smarthome/pull/3135 that will allow to close this issue as soon as merged.

@digitaldan considering what you previously wrote, it could break setpoint handling in HABdroid.

lolodomo commented 7 years ago

ESH PR was merged and this issue will become irrelevant. @digitaldan : do I close the issue or do you need to adjust code for particular cases ?

lolodomo commented 7 years ago

With snapshot 849, the ESH fix is now available and working. I close this issue.

kaikreuzer commented 7 years ago

Glad to hear that!

lolodomo commented 7 years ago

Almost 10 months for something that was finally easy to fix ;) But I am very happy today to have right icons in HABdroid.