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

Set unit metadata & state description pattern when creating UoM Item #2126

Merged
merged 10 commits into from
Dec 5, 2023

Conversation

florian-h05
Copy link
Contributor

@florian-h05 florian-h05 commented Oct 9, 2023

Related to openhab/openhab-core#3481.
Closes #2108.

  • Ensures that the internal unit is set when an Item is created, so that in case the system unit changes (i.e. measurement system changes) persisted data does not get corrupted.
    By default, the unit metadata is set to the system default unit, however the user can easily change that on Item creation.

    The unit can also be changed later as it is just normal metadata, however this can corrupt persisted data.

  • Adds the ability to set state description pattern when creating a UoM Item.

  • Shows the group type for groups, e.g. Group (Number:Temperature) instead of just the Item type.

This applies to both the Items and the model page.

This ensures that the internal unit is fixed, so that in case the system unit changes (measurement system change) persistence data does not get corrupted.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05 florian-h05 requested a review from a team as a code owner October 9, 2023 22:17
@florian-h05
Copy link
Contributor Author

This was actually discussed somewhere, but I cannot find this discussion now.
Maybe @J-N-K finds that discussion?

@relativeci
Copy link

relativeci bot commented Oct 9, 2023

Job #1218: Bundle Size — 15.8MiB (+0.23%).

16c6c02(current) vs b7270d2 main#1158(baseline)

Warning

Bundle contains 16 duplicate packages – View duplicate packages

Warning

Bundle introduced 13 new packages: @jsep-plugin/regex, @jsep-plugin/arrow, @jsep-plugin/object and 10 more – View changed packages

Bundle metrics  Change 10 changes Regression 5 regressions Improvement 1 improvement
                 Current
Job #1218
     Baseline
Job #1158
Regression  Initial JS 1.85MiB(+11%) 1.67MiB
Regression  Initial CSS 608.95KiB(+0.01%) 608.89KiB
Change  Cache Invalidation 93.81% 93.95%
Change  Chunks 217(-0.91%) 219
Change  Assets 683(-0.87%) 689
Change  Modules 2987(+75.71%) 1700
Regression  Duplicate Modules 173(+92.22%) 90
Improvement  Duplicate Code 1.61%(-17.44%) 1.95%
Regression  Packages 152(+10.14%) 138
Regression  Duplicate Packages 16(+6.67%) 15
Bundle size by type  Change 3 changes Regression 3 regressions
                 Current
Job #1218
     Baseline
Job #1158
Regression  JS 9.28MiB (+0.28%) 9.25MiB
Regression  Other 4.74MiB (+0.21%) 4.73MiB
Regression  CSS 860.19KiB (+0.08%) 859.49KiB
Not changed  Fonts 526.1KiB 526.1KiB
Not changed  Media 295.6KiB 295.6KiB
Not changed  IMG 140.74KiB 140.74KiB
Not changed  HTML 1.23KiB 1.23KiB

View job #1218 reportView florian-h05:item-fix-unit branch activity

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@J-N-K
Copy link
Member

J-N-K commented Oct 10, 2023

This has been discussed several times. I'm not sure if fixing the unit is a good idea, while I agree that fixing the dimension is. The difference is: if you create an item, you have an idea what you want to use it for. This fixes the dimension, but not necessarily the unit.

IMO the unit should be editable, but there should be a warning that this might be a bad idea when persistence is involved.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

florian-h05 commented Oct 10, 2023

IMO the unit should be editable, but there should be a warning that this might be a bad idea when persistence is involved.

The unit metadata is still editable through metadata edit, where a big orange warning is shown that this can corrupt persisted data. This PR only sets the unit metadata to the system default when creating an Item, so that there is unit metadata.

This is how it looks when creating an Item:

image

And this is how it looks when you want to change unit metadata later:

image

@rkoshak
Copy link

rkoshak commented Oct 10, 2023

Does it make sense to merge this with #2108? Those are other places where Items are created and giving the users the option to set the unit before the Item is created right there in those respective forms (i.e. Model and add equipment to model) seems like more of the same as what's being implemented here.

@ghys
Copy link
Member

ghys commented Oct 12, 2023

This is how it looks when creating an Item:

I'm sorry to say it, but this description is absolutely preposterous; it's 4 lines long which is probably 2 too many and seriously:

All processed values are internally normalized to the specified unit. The normalized value is used to propagate the value to external integrations (e.g. persistence, REST API, WebSocket) so these values will always have the specified unit and scale.

You couldn't be as cryptic and obscure if you tried.

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

@ghys I have updated the description, I am open for suggestions!

@rkoshak I have tried to address #2108, please have a look at the PR description.

@florian-h05 florian-h05 changed the title Set unit metadata when creating Item Set unit metadata & state description pattern when creating UoM Item Oct 14, 2023
@spacemanspiff2007
Copy link

How about:

"Used internally, for persistence and external systems. It is independent from the state visualization in the GUI which is defined through the state description".

@rkoshak
Copy link

rkoshak commented Oct 16, 2023

I'm not good at translating this code to what it will look like but based on "This applies to both the Items and the model page" it's not clear this also impacts "create equipment from thing" and "add points to model".

image

If I missed that I'd say it looks good. If It didn't then that's an important place to make suer we can set the unit too.

@martingrassl

This comment was marked as duplicate.

@florian-h05 florian-h05 added enhancement New feature or request main ui Main UI labels Oct 22, 2023
…ems from Thing" functionality

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
@florian-h05
Copy link
Contributor Author

I'm not good at translating this code to what it will look like but based on "This applies to both the Items and the model page" it's not clear this also impacts "create equipment from thing" and "add points to model".

Those fields were displayed on both pages, but not pre-filled. I have fixed this.

"Used internally, for persistence and external systems. It is independent from the state visualization in the GUI which is defined through the state description".

Thanks for the suggestion!

@florian-h05
Copy link
Contributor Author

@J-N-K @spacemanspiff2007 @rkoshak:
Should this PR be merged in the current state or is there anything left to be changed?

@rkoshak
Copy link

rkoshak commented Dec 5, 2023

Based on my understanding the unit can be set by the end user in all the places and ways an Item can be created in MainUI so I'm happy to merge it.

@florian-h05
Copy link
Contributor Author

@ghys Can you please review?

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.

Yep sorry I had this one on the backburner for some time.
Let's say I'm not happy with the added complexity but I'm happy with the solution and implementation.

A big thank you for the work involved!

@ghys ghys added this to the 4.1 milestone Dec 5, 2023
@ghys ghys merged commit 617f70b into openhab:main Dec 5, 2023
6 checks passed
@florian-h05 florian-h05 deleted the item-fix-unit branch December 5, 2023 16:08
florian-h05 added a commit to florian-h05/openhab-webui that referenced this pull request Dec 5, 2023
Addresses openhab#2126 (review).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
florian-h05 added a commit that referenced this pull request Dec 5, 2023
Addresses
#2126 (review).

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
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.

[MainUI] Add option to set unit to Model and "Add equipment to model"
6 participants