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

Provide meaningful list of units in item form #2312

Merged
merged 24 commits into from
Mar 1, 2024

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Feb 6, 2024

Closes openhab/openhab-core#4082

This PR adds:

  1. A curated list of units to show as a drop-down list when creating a Number item with dimension.
  2. The possibility to use a different default unit on item creation than the system default unit.
  3. The ability to change unit and state description for items.

By default, the system default unit (for the configured measurement system) will be shown when editing or creating an item. units.js contains a number of frequently used units by dimension and measurement system. These will be available in a autosuggest dropdown list. It is still possible to not select from the list and use any other string as a unit. units.js now only contains a relatively small list, but can easily be extended with other frequently used units.
All units for the dimension in units.js will be in the dropdown list, but they will be sorted by measurement system. If the measurement system is set to US, imperial units will appear higher in the list. Units that have not explicitely been listed as SI or US will always appear higher.
When typing a unit that is not in the curated list, a longer list will be used for autocompletion that considers allowed prefixes to base units and constructs all combinations.

units.js also contains a field to set a different default unit on item creation than the system default unit.

With openhab/openhab-core#4079, the REST API of channel types would provide a unit hint if defined in the binding channel types.

If such information is available, this PR adds support for this in the UI. When creating an item from a thing channel, it will suggest to use the unit from the unit hint if available. If not available, it will suggest the webui defined default from units.js, if not available, the system default unit for the dimension.

For dimensions with different units in SI or US measurement systems, it is possible to provide 2 units separated by comma in binding channel types. The first one will be SI, the second US. The suggested one will depend on the measurement system setting.

See also:

@mherwege mherwege requested a review from a team as a code owner February 6, 2024 12:57
Copy link

relativeci bot commented Feb 6, 2024

Job #1750: Bundle Size — 11.13MiB (+0.11%).

1e05472(current) vs 148c463 main#1745(baseline)

Warning

Bundle contains 19 duplicate packages – View duplicate packages

Bundle metrics  Change 3 changes Regression 1 regression
                 Current
Job #1750
     Baseline
Job #1745
Regression  Initial JS 1.84MiB(~+0.01%) 1.84MiB
No change  Initial CSS 607.88KiB 607.88KiB
Change  Cache Invalidation 17.12% 16.79%
No change  Chunks 220 220
No change  Assets 243 243
Change  Modules 3091(+0.03%) 3090
No change  Duplicate Modules 164 164
No change  Duplicate Code 1.75% 1.75%
No change  Packages 151 151
No change  Duplicate Packages 18 18
Bundle size by type  Change 1 change Regression 1 regression
                 Current
Job #1750
     Baseline
Job #1745
Regression  JS 9.31MiB (+0.13%) 9.3MiB
Not changed  CSS 889.34KiB 889.34KiB
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.24KiB 1.24KiB
Not changed  Other 871B 871B

View job #1750 reportView mherwege:unit-hint branch activityView project dashboard

@florian-h05 florian-h05 added awaiting other PR Depends on another PR enhancement New feature or request main ui Main UI labels Feb 6, 2024
@andrewfg
Copy link

andrewfg commented Feb 6, 2024

@mherwege I can’t really comment on the code; but I will make a build and test how it looks on screen.

@andrewfg
Copy link

andrewfg commented Feb 8, 2024

I will make a build and test how it looks on screen

Ok. I did a full build and found three problems..

  1. Item Creation: Initially when I first tried to create an item attached to a channel with unitHint, the 'Unit' and 'State Description Pattern' fields could not be selected, and thus could not be edited i.e. they were read only. But after some time playing around with creating and deleting items, when I tried to create an item again, the fields became editable once more. I was using the Chrome browser so I wonder if it may have been related to the recently posted browser cache issue -- but perhaps not. I tried it also in Edge and did not see the same issue.

image

  1. Item Editing: When editing the (already created) item the respective input page does not display the 'Unit' or 'State Description Pattern' fields. So to change an already existing item, the only solution is to delete and recreate the item.

image

  1. For the respective channel item shown in the screenshots above, I had set unitHint="dB" in the respective binding, however the input screen in the browser showed 'one'..

@andrewfg
Copy link

andrewfg commented Feb 8, 2024

^
Apropos point 3. above, I can see the "unitHint=dB" in the REST API JSON provided by OH core.
So in other words, the issue seems to be in openhab-webui rather than openhab-core..

  {
    "parameters": [],
    "parameterGroups": [],
    "description": "Quality of Service: percentage of configured devices currently connected to the RF mesh network",
    "label": "Mesh Network QoS",
    "itemType": "Number:Dimensionless",
    "unitHint": "dB",
    "kind": "STATE",
    "stateDescription": {
      "pattern": "%.0f %%",
      "readOnly": true,
      "options": []
    },
    "tags": [],
    "UID": "neohub:meshNetworkQoS",
    "advanced": false
  },

@mherwege
Copy link
Contributor Author

mherwege commented Feb 8, 2024

@andrewfg Thanks for testing. Issues are noted and I will look into them.

I am currently working on integrating a currated list of units in this PR as well. Let's come back on this and test again when I have done that.

@mherwege mherwege marked this pull request as draft February 8, 2024 12:35
@mherwege mherwege changed the title Suggest unit on item creation based on channel unitHint if available Provide meaningful list of units in item form Feb 9, 2024
@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

@andrewfg @jimtng I have updated the scope of this PR and changed the original description.

@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

@andrewfg If you want to have another go at testing this, that would be very much appreciated.

@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

@florian-h05 While this PR now will benefit of openhab/openhab-core#4079 and provide extra functionality, it is not strictly required for this PR and will work without.
There are more ways of providing more meaningful unit suggestions in the PR now.

@jimtng
Copy link
Contributor

jimtng commented Feb 9, 2024

For Time, is it wk or week, and mo is not listed on https://www.openhab.org/docs/concepts/units-of-measurement.html I've just checked, wk == week and mo seems fine.

For Temperature, can someone in SI still pick °F?
An idea for consideration: instead of excluding the "other" units, perhaps put them after the preferred units, so for people using SI, they'll get SI units first, followed by US units, and vice versa...?

@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

For Temperature, can someone in SI still pick °F?

It won't be in the suggestion list, but it is always possible to put it in manually.

@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

An idea for consideration: instead of excluding the "other" units, perhaps put them after the preferred units, so for people using SI, they'll get SI units first, followed by US units, and vice versa...?

Possible, but from a perspective of a European, I don't want to see the Imperial units. I don't deal with them at all. That perspective could be different for users in the US though.

@jimtng
Copy link
Contributor

jimtng commented Feb 9, 2024

Possible, but from a perspective of a European, I don't want to see the Imperial units. I don't deal with them at all.

I agree with this.

That perspective could be different for users in the US though.

They might need to use metric sometimes, and at that time, having the hints would probably be quite helpful. I'm happy either way.

From usability perspective, when it's not there, some people might think it's not possible.

@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

From usability perspective, when it's not there, some people might think it's not possible.

Agree, but I don't see a good way to overcome it. The issue is there are too many combinations possible for units and their multipliers. Showing them all does not make sense. And if you don't show all possible combinations, the proposed list should be relevant with the possiblity to put something else if needed. That's what I try to achieve.

@rkoshak Your opinion on this would be valued as well.

@andrewfg
Copy link

andrewfg commented Feb 9, 2024

If you want to have another go at testing this, that would be very much appreciated.

Ok. I just started building it now..

@andrewfg
Copy link

andrewfg commented Feb 9, 2024

They might need to use metric sometimes, and at that time, having the hints would probably be quite helpful. I'm happy either way

Please just remember the poor old UK ..

.. unlike the US which uses 'Imperial' (I always find it odd that the US refers to its units by reference its one-time imperial over-lords) .. the UK has mostly adopted SI and dropped 'Imperial' (Brexit notwithstanding) .. EXCEPT in a couple of peculiar special cases -- namely as follows..

  1. On the road: where distance and speed are measured in miles, fuel is sold in litres, and consumption is measured in miles-per-gallon (sic) .. and note that UK 'Imperial' gallons are different from US 'Imperial' gallons .. go figure ..
  2. In the pub: where beer is sold in pints..

=> Ergo for UK users, presenting both units, would probably make sense..

@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

  1. UK 'Imperial' gallons are different from US 'Imperial' gallons

Does that mean standard conversion to a metric unit does not work, as different for US and UK?

@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

Please just remember the poor old UK ..

So, what does a UK user set as measurement system, metric or imperial?

@andrewfg
Copy link

andrewfg commented Feb 9, 2024

what does a UK user set as measurement system, metric or imperial

Metric .. except when in a car or a pub. ;)

@mherwege
Copy link
Contributor Author

mherwege commented Feb 9, 2024

OK, so it sounds like it may make sense to show both, but sequence switched depending on measurement system.

mherwege added 9 commits March 1, 2024 11:06
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Mar 1, 2024

@mherwege I have created mherwege#2 which addresses some of my comments (I will mark them as resolved) and fixes a few issues.

I merged your PR in my branch on github, but couldn't get it to my local anymore afterwards. So I reapplied your changes and force pushed. All should be in there.

I went through your other comments, and I believe I have tackled all of them.

@florian-h05
Copy link
Contributor

So I reapplied your changes and force pushed. All should be in there.

I have checked the commits, looks good 👍

I went through your other comments, and I believe I have tackled all of them.

I am currently re-reviewing, I already found a few minor issue, I think I will fix them myself (just push a new commit to your branch).

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

I have applied a few fixes and JSDoc improvements, you can have a look at my recent commits (they are all divided into separate commits).

As this PR is a major improvement to the configuration of UoM Items even without the unit hint being not yet available , I will merge this PR.
Regarding the units.js asset, I am fine with having this file in the UI. In the past, we also had the semantic tags hard-coded and translated inside the UI, which are now loaded from REST.

@florian-h05 florian-h05 removed the awaiting other PR Depends on another PR label Mar 1, 2024
@florian-h05 florian-h05 added this to the 4.2 milestone Mar 1, 2024
@florian-h05
Copy link
Contributor

And with regards to the error message of the unit autocompletion: As the autocompletion works anyway, I would propose to merge this PR and then try to fix that error message later. No need to make this PR even bigger ;-)

@florian-h05 florian-h05 merged commit 2f17183 into openhab:main Mar 1, 2024
6 checks passed
@mherwege mherwege deleted the unit-hint branch March 1, 2024 12:03
@mherwege
Copy link
Contributor Author

mherwege commented Mar 1, 2024

Thank you for the review and merge.

florian-h05 added a commit to florian-h05/openhab-webui that referenced this pull request Jul 26, 2024
florian-h05 added a commit that referenced this pull request Jul 26, 2024
Regression from #2312.
Fixes #2669.

Signed-off-by: Florian Hotze <[email protected]>
florian-h05 added a commit that referenced this pull request Jul 26, 2024
Regression from #2312.
Fixes #2669.

Signed-off-by: Florian Hotze <[email protected]>
(cherry picked from commit fa85c07)
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.

Maintain curated list of units for a given dimension
6 participants