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

Use displayState in oh-input #1350

Merged
merged 3 commits into from
Apr 30, 2022
Merged

Use displayState in oh-input #1350

merged 3 commits into from
Apr 30, 2022

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Apr 8, 2022

Signed-off-by: Mark Herwege mark.herwege@telenet.be

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege mherwege requested a review from a team as a code owner April 8, 2022 11:56
@relativeci
Copy link

relativeci bot commented Apr 8, 2022

Job #388: Bundle Size — 10.7MB (~+0.01%).

6323e4e vs 40054c4

Changed metrics (2/10)
Metric Current Baseline
Initial JS 1.67MB(+0.01%) 1.67MB
Cache Invalidation 15.77% 7.49%
Changed assets by type (1/7)
            Current     Baseline
JS 8.65MB (~+0.01%) 8.65MB

View Job #388 report on app.relative-ci.com

@ghys
Copy link
Member

ghys commented Apr 13, 2022

Thanks, but there's no context provided so not sure what to make of this. What's the rationale here? It could be perfectly reasonable to think an input widget would rather be "fed" the actual state and not its displayState even if it exists (you could have another widget to show the displayState alongside the input).
At best this should be made an option - and kept as-is as the default to avoid breaking changes.

@mherwege
Copy link
Contributor Author

@ghys Thank you for your feedback.

I mostly use oh-input for entering things like meter readings. An example: I have an official, non smart, meter for my solar production. I need to manually read the value and communicate the reading to the authorities to receive subsidies for my solar production. Every 1000kWh produced will give me a certificate.
I built some rules that calculate an estimated meter reading continuously to inform me when I approach the 1000kWh value. The calculation is a bit off from the real meter, so I need to adjust it regularly with the real value.
Now the oh-input will show me the calculated value as a float with a lot of digits. This just looks ugly and is not relevant. Using displayState would solve that. It makes using the default widget easy as well. I have strong doubts the full float value is relevant in any use case.
It also allows me to show it in the most appropriate UOM. Input in other UOM remains possible.

See screenshot:
image

You are right this is a breaking change. But I still have to find a use case where displayState would not give me a better value than the raw state. Using a formatting on a free input beyond simple number formatting always will be tricky. E.g. I also use it to do text input to be able to name playlists. Such text does not have any formatting. A simple map transformation on a free form input value is also not likely, as it would be difficult to interpret all possible input values.

I can investigate making this a parameter, but in my view, this adds complexity and the current proposed solution just makes things consistent across the UI. I expected to get the displayState when someting is shown, even in an input field.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege
Copy link
Contributor Author

@ghys I made displayState optional. While this would break backward compatibility, I would argue the inverse parameter 'forceRawState' would actually make more sense and be more consistent overall. But I am OK with the this solution as well.

@ghys
Copy link
Member

ghys commented Apr 19, 2022

You are right this is a breaking change. But I still have to find a use case where displayState would not give me a better value than the raw state.

You can always find some. Imagine a scenario where you have a Number item for a scene and a state description mapping like 1=Morning, 2=Evening, 3=Night etc. In such a case for a state of "1" the displayState would be "Morning".
Now someone might choose to have an input to select the scene from 1, 2 or 3. With this change they would have to change the "Morning" value to the new raw state i.e. "2", which is probably not what they want.

@mherwege
Copy link
Contributor Author

Number item for a scene and a state description mapping like 1=Morning, 2=Evening, 3=Night etc.

I know and understand, but wouldn’t a list item be a much more appropriate input widget in this case? I personally only use an input type widget where there is no better way possible. After all, it is not the most practical widget type on a touch display.

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's a better example:

image

(regular DateTime item with a state description pattern: %1$tA, %1$td.%1$tm.%1$tY)
Default standalone widget:

value: oh-input-card
config:
  type: date
  footer: =items.TestDate
  sendButton: true

In this case the displayState would probably not be parsed if set as the <input type="date"> value (depends on the pattern), whereas the state always will.

Below are some wording suggestions for the new option.

Signed-off-by: Mark Herwege <mark.herwege@telenet.be>
@mherwege
Copy link
Contributor Author

@ghys Thank you for the example and the feedback. I have made the suggested adjustments.

Copy link
Member

@ghys ghys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@ghys ghys merged commit da8621c into openhab:main Apr 30, 2022
@ghys ghys added main ui Main UI enhancement New feature or request labels Apr 30, 2022
@ghys ghys added this to the 3.3 milestone Apr 30, 2022
@mherwege mherwege deleted the input-displayState branch May 20, 2022 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request main ui Main UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants