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

Items: Add unit metadata for UoM (Number:) Items #1901

Merged
merged 3 commits into from
May 20, 2023

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented May 15, 2023

Fixes #1890.

This adds the unit metadata from openhab/openhab-core#3481 to the pre-defined metadata namespaces for UoM Items (Item type Number:) and provides a metadata edit page for it.

Once openhab/openhab-core#3611 is merged, the Item create page will be adjusted to set the unit metadata to the system default on Item creation.

@florian-h05 florian-h05 added this to the 4.0 milestone May 15, 2023
@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels May 15, 2023
@relativeci
Copy link

relativeci bot commented May 15, 2023

Job #1009: Bundle Size — 15.77MiB (+0.01%).

50918d2(current) vs 9c95b8d main#1008(baseline)

⚠️ Bundle contains 16 duplicate packages

Metrics (no changes)
                 Current
Job #1009
     Baseline
Job #1008
Initial JS 1.73MiB 1.73MiB
Initial CSS 608.89KiB 608.89KiB
Cache Invalidation 93.95% 93.95%
Chunks 219 219
Assets 689 689
Modules 1720 1720
Duplicate Modules 85 85
Duplicate Code 1.72% 1.72%
Packages 138 138
Duplicate Packages 15 15
Total size by type (2 changes)
                 Current
Job #1009
     Baseline
Job #1008
CSS 858.62KiB 858.62KiB
Fonts 526.1KiB 526.1KiB
HTML 1.23KiB 1.23KiB
IMG 140.74KiB 140.74KiB
JS 9.26MiB (+0.02%) 9.25MiB
Media 295.6KiB 295.6KiB
Other 4.73MiB (+0.01%) 4.73MiB

View job #1009 reportView main branch activity

@J-N-K
Copy link
Member

J-N-K commented May 15, 2023

Looks good. Please note that the metadata unit is optional, if it is not set, the system default is used (state description still is independent).

@florian-h05
Copy link
Contributor Author

There is a notice that system default is used when unit is not set:

image

I think that makes it clear enough that it is optional, WDYT?

@florian-h05
Copy link
Contributor Author

@spacemanspiff2007 wrote on the linked issue:

When creating a UoM item the unit should be mandatory, see core issue.
The system default should be (pre-)selected but the unit should be visible directly on the site with the create button.

I also had a look at the linked core issue openhab/openhab-core#3615.

I will provide a field to set the Item metadata on Item creation and as soon as the core PR for UoM information is merged, we can populate the unit metadata field with the system default.

WDYT?

@spacemanspiff2007
Copy link

spacemanspiff2007 commented May 16, 2023

Sounds great, as long as the field will always be automatically populated so creating an UoM item will always result in the corresponding unit metadata.
It should not be possible to create a UoM item through the GUI without the metadata.
That way it's ensured that future changes (e.g. to system defaults, locale, etc.) will not lead to wrong data/graphs in external systems.


I think that makes it clear enough that it is optional, WDYT?

How about
"All processed values are internally normalized to the specified unit. The normalized value is used to propagate the value to external systems (e.g. persistence, RestAPI, HABApp) so these values will always have the specified unit and scale. Changing the unit will change the values that these systems receive.
Additionally the unit will be used if no unit is specified for the state description.
The state description is used ..."

@lolodomo
Copy link
Contributor

If we don't want to postpone again the milestone 3, the merge of this PR would be greatly appreciated (if it works).

@lolodomo
Copy link
Contributor

lolodomo commented May 18, 2023

Additionally the unit will be used if no unit is specified for the state description.

Additionally the unit will be used in UIs if no unit is specified for the state description.

... to external systems (e.g. persistence, RestAPI, HABApp) so ...

We could maybe avoid mentioning specifically a tool that is not part of the official OH distribution (and not in OH documentation) ?
Just a remark.

@J-N-K
Copy link
Member

J-N-K commented May 18, 2023

I would suggest to use external integrations (e.g. persistence, REST API, WebSocket).

@florian-h05
Copy link
Contributor Author

If we don't want to postpone again the milestone 3, the merge of this PR would be greatly appreciated (if it works).

This is not ready yet and for setting the unit metadata when creating a UoM Item, openhab/openhab-core#3611 has to be merged first.
Unfortunately I’m on school trip until Sunday morning, but I think I can finish this on Sunday.

@lolodomo
Copy link
Contributor

IMHO, if we have this PR allowing to easily add the unit metadata, the change in item creation page could be delayed.

@florian-h05
Copy link
Contributor Author

If we agree to delay the change in Item creation page this doesn’t need the core PR and I should be able to finish this (it’s just about naming and descriptions then) from my iPad.

Addressing review.

Signed-off-by: Florian Hotze <[email protected]>
@florian-h05 florian-h05 marked this pull request as ready for review May 18, 2023 11:26
@florian-h05 florian-h05 requested a review from a team as a code owner May 18, 2023 11:26
@florian-h05
Copy link
Contributor Author

@ghys Can you please review and merge this for M3? Thanks!

@spacemanspiff2007
Copy link

spacemanspiff2007 commented May 18, 2023

We could maybe avoid mentioning specifically a tool that is not part of the official OH distribution (and not in OH documentation) ?
Just a remark.

It's shown in the rule technology overview. Also NodeRed, jRuby, Mycroft AI are mentioned multiple times through the whole docs and these are all not part of the official OH distribution. So the argument "not part of the official OH distribution" does obviously not apply.

@rkoshak
Copy link

rkoshak commented May 18, 2023

Also NodeRed, jRuby, Mycroft AI are mentioned multiple times multiple times through the whole docs and these are all not part of the official OH distribution.

I'm not arguing against the sentiment (I have no problems with mentioning tools outside the OH project in the docs where it make sense). But I wanted to correct this list of examples of external projects.

  • NodeRed is mentioned in only three places I can find through search. In addition to the link you provided, one is in openHABian's docs where it's explained that openHABian installs it and the second is an example for how a third party tool can use an openHAB API Token. The first is very appropriate since it's part of what openHABian does and openHABian is part of official openHAB. The second is arguable whether NodeRed is the best choice but something needs to be picked as an example there and the screenshots from NodeRed are very clear so I would argue it's a good choice.

  • jRuby has an official automation add-on and it's helper library is part of the openHAB project too which put it at the same level as at least Rules DSL and GraalVM JS Scripting (maybe Groovy and Java too?). It is an official part of OH in every meaningful way.

  • Mycroft.AI is a third party system which openHAB officially supports integration with, similar to Alexa, Google Assistant, and HomeKit. And it's only mentioned in the Mycroft.AI pages in the docs, as is appropriate.

All three of these examples are a part of official openHAB.

But again, I'm not arguing that HABApp shouldn't be mentioned in the docs anywhere because it isn't part of the openHAB project. In fact, isn't it an option in openHABian to install? If not it should be. That would make it as "official" as NodeRed at least.

However, because HABApp isn't discussed anywhere in the docs except that one table in Getting Started, it's a poor choice to use as an example in the description of the field built into MainUI. So I agree with @lolodomo in this one case but not as a general rule to apply everywhere.

@spacemanspiff2007
Copy link

Nicely written and I mostly agree with you.
I was just showing that we have parts that are not official OH distribution and thus I think the argument was void. If you or @lolodomo write you think it's not a good fit here that's totally fine and I'll gladly accept it.

In fact, isn't it an option in openHABian to install? If not it should be.

It has been since over two years - option 2B in the menu (I know it by heart now 😉 ).

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 for the description part. Can't comment on the code.

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.

We wouldn't want a milestone release with the core change but no UI support. LGTM, thanks for the implementation!

@ghys ghys merged commit 0b55558 into openhab:main May 20, 2023
@florian-h05 florian-h05 deleted the items-unit-metadata branch May 20, 2023 12:37
florian-h05 added a commit to florian-h05/openhab-webui that referenced this pull request Jun 24, 2023
Fixes openhab#1933.
Follow-up for openhab#1901.

This enables the unit metadata for UoM groups (i.e. groups with a groupType of `Number:`).

Signed-off-by: Florian Hotze <[email protected]>
ghys pushed a commit that referenced this pull request Jun 28, 2023
Fixes #1933.
Follow-up for #1901.

This enables the unit metadata for UoM groups (i.e. groups with a
groupType of `Number:`).

--
Signed-off-by: Florian Hotze <[email protected]>
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.

Unit as pre-defined metadata
6 participants