eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
863 stars 782 forks source link

Switch Dynamic Icons should represent Item State #4445

Open Dracrius opened 7 years ago

Dracrius commented 7 years ago

It was brought to my attention while testing a problem on androids openHAB app that Basic UI only pays attention to the switch state of an item when a switch is used in a sitemap regardless of the item type.

This is fine and the simple way to do things but say you have a Dimmer item with a switch and a slider in your sitemap. The slider will show say the 50% dynamic icon because it's set to 50% but the switch right beside it would show 100% or I am assuming more accurately the ON icon. This seems like a very quick and dirty solution. Both the Switch and the Slider should have the same dynamic icon as they both represent the same item just different ways of control it.

I post this as an issue because until I brought up androids in ability to show color item icons dynamically #408 android actually fetched the item's state so any switch I had for a light would show an icon that represented more then just ON or OFF this was beautiful and made a simple switch more informative. The person helping me found how Basic UI handles this and has reverted the android app to the simpler approach. This is highly disappointing as that feels like a step backwards simply because of a standard most likely unintentionally set by Basic UI!

htreu commented 7 years ago

@lolodomo could you have a look into this?

lolodomo commented 7 years ago

You have an icon for each item state, that means one item for slide 0%, one for slide 10%, ... one for slider 100%, and you have an icon for ON state and for OFF state. So if the current state is 50%, you will get the icon for 50% for the dimmer control and the icon for ON for the switch control. That makes sense for me.

Dracrius commented 7 years ago

But this is an overly simplified way to look at it. It may work but its kind of cheating. If an item has two controls a slider and a switch a user may put them together. Even if not there is no reason to slack here! An icon is supposed to represent the items current state not just the sitemap elements state. Also if your going to have dynamic icons shouldnt they all be equaly dynamic? The information is there so why not use it. I'm simply asking for the change from checking a visual element when deciding which icon to use to using the items actual state! This would result in any switch used with a dimmer or color item having the same icons as its corisponding slider rather then a slightly jaring 100% icon for lights only set to 50% right beside a slider for the same light showing a diffrent icon! This is tandamount to autoupdate=off keeping sliders from being updated to 100% because an ON command was sent.

Dracrius commented 7 years ago

It boils down to the idea that i think i should be able to tell from my light switches page which lights are set high or low when ON based on their icons. Its a simple thing but it makes all the diffrence in feeling like a more polished product vs just what works. Dynamic icons are there to present visual information why not provide all the information you can!

lolodomo commented 7 years ago

The icon management is currently attached to the sitemap element, not the item type. I understand what you request but that would be a major change to implement. Maybe @kaikreuzer @maggu2810 @sjka may give their opinion.

mueller-ma commented 7 years ago

I agree with @Dracrius

Dracrius commented 6 years ago

I'd appreciate seeing this looked into even though it may not be a bug it's quite a short cumming and makes the name dynamic icon kind of an oxy moron! Icon's should represent ITEMs states if they have the ability to be dynamic not simply represent the sitemap elements state as this leaves the entire interface disjointed!! Two controls for the same device should show the same icon/state, just because they are separate controls doesn't mean that item is in two sates at once. They control the same item so they should represent a single state!

Leaving this as is just because it's work to change will only make it harder to change later as you have people like mueller-ma making changes to the android app to align it with how basic UI works and it sounds like it only works this way because it was easier and no bodies bothered to change it now! This will result in a standard set by laziness not a good sign for the advancement of the project.

lolodomo commented 6 years ago

In fact, this is something that was decided at the origin of openHAB, probably when the concept of sitemap was imagined. It was first implemented in Classic UI and only later in Basic UI when OH2 started. People who invented the sitemap concept could probably explain their choice. @kaikreuzer: you were probably already there at this time ?

On my side, I think the change request is acceptable. It would just require a change in all UIs and probably not a basic change.

lolodomo commented 6 years ago

And it would very probably require a change in the sitemap REST API to provide 2 item states, one to be used for the icon (raw state) and one for the sitemap element control.

lolodomo commented 6 years ago

Something not clear for me: for color item, the state is a HSB value. How do you link this value to an icon ? What would be the icon naming convention ?

Dracrius commented 6 years ago

As of the moment i expected them to only represent a colored lights brightness value. I have a set of icons i made for sliders that I expected to also be shown by my switches as brightness changed. The icons show the bulbs as a light purple as this is the color/tone philips uses for colored bulb packaging.

I can understand it would be one heck of an undertaking to attempt to show a live color in the icons. Although I had thought personally it could be possible if you had a set of icons with hue color ranges. Ie one for greens, oranges, blues etc i can't remember the number ranges off the top of my head though. This would still be a large icon group including all the brightness values but to be honest I'd make the icon set if you thought it was worth doing since we'd be doing the large change anyways! Maybe something like colorbulb-280-360-10.png for color range 280-360 at 10% brightness.

lolodomo commented 6 years ago

I will try to explain the current implementations and my proposal of enhancement:

My proposal is to add the necessary information in the sitemap REST API to avoid that each UI implements its own logic to convert the item state to a state adapted to the widget. For this, I propose to add two new string fields in the WidgetDTO (sitemap REST API) and SitemapWidgetEvent (SSE event) objects:

To optimize and reduce data exchange, I can propose to have these fields set to null when their value is identical to the raw item state.

Here are the changes to be done:

For dynamic icons, we have to consider what to do for certain item types like color item for example.

Here are examples how would look the message depending on item type and widget type.

For a switch item set to ON and a switch widget:

For a dimmer item set to 60 and a slider widget:

For a dimmer item set to 70 and a switch widget:

For a color item set to 200,45,50 and a color widget:

For a color item set to 200,45,50 and a slider widget:

For a color item set to 200,45,50 and a switch widget:

So for color items, we consider only the brightness for the dynamic icon.

For an image item set to data:xxxxxxx and an image widget:

For a datetime item set to 2017-10-30T20:40:00.0000 and a text widget:

So for certain items, we set the iconstate to the empty string to avoid using the real state when searching the dynamic icon.

All this implies a lot of changes but it is not too much difficult for me (because I know eactly what to change - except in the Android app).

I am waiting for a feedback and I will do nothing until I got a GO from mainteners.

lolodomo commented 6 years ago

I forgot to mention that with this solution, UIs that will not be updated will of course continue to work but with the current behaviour. Only UIs using SSE would be impacted. At my knowledge, Basic UI is probably the only UI using SSE and I plan to fix it.

And the advantage of this new solution is that the main logic is on the server side and no more in each UI.

lolodomo commented 6 years ago

The optimization with null is important mainly for image items to not have the image data twice in the message.

lolodomo commented 6 years ago

No feedback ? Really ?

As a reminder, the purpose of the requested change is to have the dynamic icon based on the item type rather than the widget type. My proposal allows this even if a little larger.

Dracrius commented 6 years ago

I really appreciate this personally! I really wish we were getting more feed back on this as I think its a great and needed change/improvement. I held off on replying as there is not much more I can add to this but that sound like it will do the job quite well.

I apologize I didn't realize how complicated things are behind the scene as I still don't understand why a single reference to an items state isn't used. Is this just easier then having each UI's code itself be able to interpret states? I can understand this would cut a lot of code from the UI's but is it enough to cause a major performance difference or is it more just simplifying whatever possible, and congregating logic to one place?

lolodomo commented 6 years ago

It is only few lines of code on the server side. Using these new data in REST API will have no negative impact on performance. That will be a kind of helper for each UI. The UI will use it or not. Using it will help to have a common behaviour amongst all UIs.

digitaldan commented 6 years ago

HI @lolodomo ,

I will try to explain the current implementations and my proposal of enhancement:

Thanks for the very clear explanation, was very helpful.

state : it will contain the item state adjusted to the current widget computed by the server iconstate : it will contain the state to be used to search for the dynamic icon

I'll let others debate having a separate state field as I would need to think about that more, but I'm not sure that iconstate makes sense to me. I understand what you are trying to achieve, but at first glance this would not be clear to me what that field is used for. Is there a better name maybe? Its more then the state of a icon, correct?

To optimize and reduce data exchange, I can propose to have these fields set to null when their value is identical to the raw item state.

My issue with this is that this implies logic within the API itself, a endpoint cannot simply use the values it is given, it has to have knowledge that "if this is null, use another value", where is that documented? How would a user know to do this? Since this can not be communicated at all in a simple REST api, I would not go that route.

mueller-ma commented 6 years ago

The optimization with null is important mainly for image items to not have the image data twice in the message.

Maybe it would be better if it is only for image items to reduce the complexity.

lolodomo commented 6 years ago

When you define in your item file icon for an item, a dynamic icon is built by searching the file light_.. My idea with iconstate is to provide the string corresponding to .

Regarding the optimization for the new state field, I can hear your good arguments but please note that this optimization is already implemented in the same message with the field transformedState (set to null when there is no transformation applied to the state). Another solution will be to set the new state field with state.toFullString() and update the current field to set it with state.toString() rather than state.toFullString(). Of course, this change will impact any sitemap REST API consumer but only for the handling of image item. That is a good alternative.

lolodomo commented 6 years ago

I will provide again the examples with the alternative solution.

Dracrius commented 6 years ago

@lolodomo Could we get those examples? I was very excited by finally seeing some movement on this and would be overjoyed to see it picked back up again!

lolodomo commented 6 years ago

Here are my updated examples without the optimisation and with iconstate renamed into iconDynamic.

For a switch item set to ON and a switch widget:

For a dimmer item set to 60 and a slider widget:

For a dimmer item set to 70 and a switch widget:

For a color item set to 200,45,50 and a color widget:

For a color item set to 200,45,50 and a slider widget:

For a color item set to 200,45,50 and a switch widget:

So for color items, we consider only the brightness for the dynamic icon.

For an image item set to data:xxxxxxx and an image widget (not satisfying as the big raw data is duplicated):

For a datetime item set to 2017-10-30T20:40:00.0000 and a text widget:

So for certain items, we set the dynamic icon to the empty string to avoid using the real state when searching the dynamic icon.

lolodomo commented 6 years ago

I will try to progress step by step. First I opened a new issue to discuss the issue with SSE (4670). Then I will open a new issue to discuss the advantage of the new "widget state" in the REST API. And so here we could continue discussing about your real subject, that is the fact that you would like the dynamic icon linked to the item rather than the widget itself. You probably noticed like me that this subject is not interesting a lot of people ;)

lolodomo commented 6 years ago

For information, all APIs that are used by UIs (sitemap REST API and sitemap SSE) are now providing the item raw state + the widget state if different from the item state.

So we could now come back to the original feature request which is to consider the item state rather than the widget state to select the dynamic icon. It is now doable by any UI even without enhancing the API. @Dracrius : you got no feedback from mainteners of Eclipse SmartHome, I have no idea if they share your wish. But what I know is that if we change the icon philosophy like you wish, all users would have to update their items/sitemaps.

Dracrius commented 6 years ago

So are you saying it is again not feasible? Based on your description I don't see why users would have to update their items/sitemaps. Your statement "For information, all APIs that are used by UIs (sitemap REST API and sitemap SSE) are now providing the item raw state + the widget state if different from the item state." makes it sound like the information is now their and shouldn't require any change to items/sitemaps. Or do you simply mean users would have to update their OpenHAB to the current version to take advantage of this feature?

lolodomo commented 6 years ago

No I say the reverse, that is it is now doable. I just say in addition that if done it could require few adjustments of icons in users's existing sitemaps.

To be honest, I have not an absolute opinion if your idea is better than what we have now. But I would be ok with the change if it is the same for most of users.

Dracrius commented 6 years ago

What kind of changes would users have to make is the part I'm confused by as it seemed to me like you were just changing how things work in the background.

On the point of the idea is it really not better having dynamic icons representing the items they are linked to rather then just the widgets state? I hate looking at my site map and seeing all my lights at 100% even though they are at varying brightness's! If this wasn't a free program you can be sure the developers would have gotten complaints about mitch matched icons.

Right now if a switch and a slider for the same item are grouped together it looks like one represents a different light then the other as they don't have matching icons. This makes OpenHAB and the UI feel very cheap and lazy not like the world class tool it is. OpenHAB is 10x's the product that most major companies like Samsung are pushing out but things like this will keep it looking like not much more then a cheap alternative!

lolodomo commented 6 years ago

One time again, it is not really me that has to be convinced. I even tried to "push" with you, I even worked on the prerequisites to implement the change and I even proposed to implement the change in some of the UIs.

Your change request is a major change that will require a change in all official UIs for consistency (consistency between UIs is very important). So it is not something that can be decided only by you and me. We need a kind of consensus. And until now, I have see abolutely no message of guys who are in charge of accepting code changes.

When I say that user sitemaps would have to be updated, I think for example about the case the user selected for a switch widget in his sitemap an icon that has only ON and OFF values. If after the changes, the values are 0, 10, ... 100, the user will have to change its icon. In fact, with your idea, the icons have to be set mainly on the item and no more in the sitemap.

Dracrius commented 6 years ago

Awh I see what you mean about the icons now I'm sorry I hadn't thought of that because I tend to set the icons for item in their definition as I'm a tad bit OCD and it seems more efficient. This is one of the many reason's I stumbled onto this as I had to add ON and OFF icons to my icon sets for it to function properly the way things are now.

Do you see any chance or way of bringing light of this idea and it's benefits to the powers that be?

Dracrius commented 6 years ago

The current setup left me thinking things were broken many times when I was just getting started as the UI has a major disconnect from items actual state!

htreu commented 6 years ago

Hey @Dracrius & @lolodomo, although not in charge of code changes I think the proposed change makes sense from a user pov. Nonetheless breaking users sitemaps is a no-go, so you have to find a way to provide backward compatibility.

mueller-ma commented 6 years ago

Nonetheless breaking users sitemaps is a no-go, so you have to find a way to provide backward compatibility.

It doesn't brake the whole sitemap, just some dynamic icons won't work anymore. When I updated to oh 2.2, my sitemap was also "broken", because of some icons beeing renamed.

lolodomo commented 6 years ago

Yes, that's correct.