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

Add numericState and unit to StateDTO #4123

Merged
merged 2 commits into from Mar 1, 2024
Merged

Conversation

jimtng
Copy link
Contributor

@jimtng jimtng commented Mar 1, 2024

This would avoid having to use parseFloat for item state by making the numericState readily available.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng jimtng requested a review from a team as a code owner March 1, 2024 06:23
@jimtng
Copy link
Contributor Author

jimtng commented Mar 1, 2024

@rkoshak

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Mar 1, 2024

What if it's an integer? Will this still return a float? If so could you make it an integer?
What about websockets? Do you think we should add it there, too?
Just do double check - will this affect the ItemStateChangedEvent, too?

@jimtng
Copy link
Contributor Author

jimtng commented Mar 1, 2024

What if it's an integer? Will this still return a float? If so could you make it an integer?

There seems to be no distinction in json
https://json-schema.org/understanding-json-schema/reference/numeric

What about websockets? Do you think we should add it there, too?

Where is this used?

@jimtng
Copy link
Contributor Author

jimtng commented Mar 1, 2024

Just do double check - will this affect the ItemStateChangedEvent, too?

This is purely for the MainUI SSE.

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Mar 1, 2024

My concern is not the json schema description but rather the returned value.
But wouldn't decimalState.floatValue(); always return a float value even if it's an integer?
E.g. openHAB state 1 would be 1.0 in the event.
Because even though the schema concerns it as an integer the json parser still returns a float.

This is purely for the MainUI SSE.

Is this different from the sse subscription HABApp uses?

Where is this used?

It's still on my never ending todo list to switch from SSE to websockets.
It just requires a huge refactor in how HABApp handles all connections and events so it takes way longer than anticipated.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 1, 2024

decimalstate is never really just an integer. It is stored as BigDecimal.

I tested this with a Battery level, and I got 100 (without decimal points) in the UI.

Is this different from the sse subscription HABApp uses?

I'm not sure what HabApp uses. This is alongside the existing items[xxx].displayState, whereever that may be used. It is not in the standard ItemStateChangedEvent etc.

@ghys and @florian-h05 understand this better than I.

It's still on my never ending todo list to switch from SSE to websockets.

I've seen somewhere that MainUI is also going to switch to websockets, eventually

@spacemanspiff2007
Copy link
Contributor

HABApp subscribes to /rest/events so I guess it should be similar but I am not sure

This is the event how I get it at the moment

{
    "topic": "openhab/items/Length_event_test/stateupdated",
    "payload": {
        "type": "Quantity",
        "value": "222 m"
    },
    "type": "ItemStateUpdatedEvent"
}

As you can see currently an integer is returned (222) and not a floating point number (222.0).
So I would expect the numericState field to behave similarly.

Now that I look at the events the names are different (values vs state) so it seems that these are indeed different events and ways how they are emitted.

It would be very helpful nonetheless if it would be like this so I'd love to see a PR that changes these events, too.

{
    "topic": "openhab/items/Length_event_test/stateupdated",
    "payload": {
        "type": "Quantity",
        "value": "222 m"
        "numericValue": "222",
        "unit": "m",
    },
    "type": "ItemStateUpdatedEvent"
}

Because currently I have to transform 222 m into 222 programmatically.

@florian-h05
Copy link
Contributor

This PR only changes the Item States SSE stream, which is located at /rest/events/states and is used by the UI to keep track of the Item states.

/rest/events is the generic event stream that provides read access to the event bus, this is also used by the UI, e.g. on the rule edit page to track the rule status.
If the Item events /rest/events should provide additional information, the ItemEventPayloadBean and probably other ItemEvent***Beans as well as the ItemEventFactory need to be updated. But IMO this is something for another PR.

@spacemanspiff2007
Copy link
Contributor

Thank you for the clarification!

Copy link
Contributor

@florian-h05 florian-h05 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

After this PR has been merged, please update the docs accordingly: https://next.openhab.org/docs/ui/widget-expressions-variables.html#expressions-overview.

@rkoshak
Copy link

rkoshak commented Mar 1, 2024

I'm not sure if I have anything to add here. It seems like a good idea to me and will make expressions in widgets at least a bit shorter if not easier to manage.

@jimtng
Copy link
Contributor Author

jimtng commented Mar 1, 2024

I'm not sure if I have anything to add here. It seems like a good idea to me and will make expressions in widgets at least a bit shorter if not easier to manage.

Your approval means a great deal!

Copy link
Member

@J-N-K J-N-K left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Mar 1, 2024
@J-N-K J-N-K added this to the 4.2 milestone Mar 1, 2024
@J-N-K J-N-K merged commit 97d64a1 into openhab:main Mar 1, 2024
3 checks passed
@lolodomo
Copy link
Contributor

lolodomo commented Mar 2, 2024

Is it backward compatible? Only new fields added ?
If I am not wrong, the remote openhab binding relies on this API.

Edit: looking at the code change, I have my answer.

@jimtng jimtng deleted the rest-numeric-state branch March 2, 2024 09:17
florian-h05 pushed a commit to openhab/openhab-webui that referenced this pull request Mar 2, 2024
@openhab-bot
Copy link
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/floor-map-iconrotation-from-item-state-does-not-work/155240/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants