[rest] Add widget state pattern and default unit to ItemUIRegistry #3644

mherwege · 2023-06-05T16:06:56Z

Resolves #3624

When developing the Input widget type for BasicUI and the Android app, I ran into a number of constraints to properly support an input widget in all cases. Having better access to the defined pattern and default unit would make this a lot simpler for BasicUI (using the methods of ItemUIRegistry) and make things possible that are not possible at this time in the Android app.

The fundamental problem is that format patterns are meant to be used for visualisation, but not as an input mask. Still, also in the input widget, one wants to use the formatted value (otherwise, you end up with the wrong unit, or digits beyond what is needed). Also, if a value is undefined, it would still be required to show the unit from the state pattern if available (or the default unit if not).

This PR proposes to extend ItemUIRegistry methods by providing a getFormatPattern method. This method will get the format pattern from the widget label, resp. item label, resp. item state description.

The sitemap REST response has following additional fields:

pattern: widget format pattern (using the value from the getFormatPattern listed above
unit: unit of the widget, also available when the value is NULL or UNDEF

In addition, the item REST part of the sitemap response contains an extra field, unitSymbol with the default unit or defined unit from meta data, so this can be shown for NULL or UNDEF value if no format or no state description is provided.

All existing tests for ItemUIRegistry run through fine. A modified version of BasicUI is also a lot simpler with these changes added and has been tested to work. I will amend openhab/openhab-webui#1787 and push a version making use of the functionality in this PR if this PR would be accepted.

While this will simplify things in the UI's, this is not an absolute must for BasicUI. For the Android app, a user would need to make sure to provide a simplified format in the label to be able to use the input field consistently. This PR would allow to work around this. It does provide all info in the REST API to avoid ambiguity.

lolodomo · 2023-06-09T07:10:59Z

All my OH free time is currently focussed on implementing the mew semantic tag registry but I hope I can review that PR during the weekend.

lolodomo

Note that you are updating a very sensible (and complex) part.
I hope this will break nothing.

Note also that your change in convertState will break the slider widget in Basic UI as Basic UI does not expect a unit in that case. A fix in Basic UI should be prepared.

lolodomo · 2023-06-10T17:25:22Z

...org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.java

- label = label.substring(0, label.indexOf("[") + 1) + formatPattern + "]";
+ int index = label.indexOf("[");
+ if (index >= 0) {
+ label = label.substring(0, label.indexOf("[") + 1) + formatPattern + "]";


Please use index

lolodomo · 2023-06-10T17:31:59Z

...org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.java

+ }
+
+ } catch (ItemNotFoundException e) {
+ logger.error("Cannot retrieve item '{}' for widget {}", itemName, w.eClass().getInstanceTypeName());


Looks like a too much high log level. WARN level should be sufficient.

This wasn't consistent across the class and I just copied one with error level. I will make it warn everywhere.

lolodomo · 2023-06-10T17:37:43Z

...ore.io.rest.core/src/main/java/org/openhab/core/io/rest/core/item/EnrichedItemDTOMapper.java

@@ -93,6 +94,10 @@ private static EnrichedItemDTO map(Item item, ItemDTO itemDTO, boolean drillDown

 EnrichedItemDTO enrichedItemDTO;

+ String unitSymbol = null;
+ if (item instanceof NumberItem numberItem) {
+ unitSymbol = numberItem.getUnitSymbol();


This is the item internal unit ?

lolodomo · 2023-06-10T17:44:06Z

@J-N-K or @kaikreuzer : as this PR is updating a very sensible part of sitemap UIs, please do an attentive review. I have the feeling it is OK. Please also check the pattern strings, I am not very familiar with their syntax.

As already mentioned, the change at line 694 will break slider in Basic UI for numbers with dimension. So a fix must be prepared for Basic UI and merges should be consecutive.

mherwege · 2023-06-11T14:06:28Z

As already mentioned, the change at line 694 will break slider in Basic UI for numbers with dimension. So a fix must be prepared for Basic UI and merges should be consecutive.

I will look into this. The change in line 694 is based on @J-N-K suggestion here: #3624 (comment). Keen to know if there are other consequences. This case is not covered by unit tests.

lolodomo · 2023-06-12T11:01:05Z

In file smarthome.js (BasicUI), line 1698 has to be fixed and comment at line 1683 has to be removed. Line 1659 has to be checked too (I am not sure of the behaviour of parseInt) .
Setpoint widget is properly handled, so code can be looked at line 973.

lolodomo · 2023-06-12T11:06:13Z

@mherwege : can you prepare a small PR for Basic UI without changing everything but just fixing the issue introduced by this PR?

mherwege · 2023-06-12T14:59:21Z

@lolodomo I create the PR , but have one outstanding question on there.

spacemanspiff2007 · 2023-06-13T04:58:34Z

Just for my understanding:
When I asked to include the metadata in the item events it was always rejected because it might be not available when the event is issued (or some implementation using a custom metadata provider).
How is this reflected with in this PR? Will the item unit then be wrongly not set?

I think it would be nice to have the item unit as a dedicated field in the normal item response, too ...

lolodomo · 2023-06-13T06:12:06Z

@lolodomo I create the PR , but have one outstanding question on there.

Your BasicUI PR is fine and ready to be merged.
So we are now waiting for core maintainers review of the current PR.

I believe there is a good chance of fast merge so you can start adjusting your PR for new input widget (BasicUI).

lolodomo · 2023-06-13T06:18:24Z

I believe there is a good chance of fast merge so you can start adjusting your PR for new input widget (BasicUI).

Maybe create a separate PR in case this PR is not merged fast.
In any case, my intention is to merge your Basic UI stuff related to input widget whatever the solution, before OH4 is released.

lolodomo · 2023-06-13T06:31:00Z

To avoid a new infinite debate about unit management in OH, I would suggest to cut this PR in two, one for REST item (to be debated) , and one for REST sitemap. Idea is to not delay the REST sitemap part which is waited.

mherwege · 2023-06-13T06:47:30Z

Maybe create a separate PR in case this PR is not merged fast.
In any case, my intention is to merge your Basic UI stuff related to input widget whatever the solution, before OH4 is released.

PR is here openhab/openhab-webui#1923.

mherwege · 2023-06-13T07:15:40Z

To avoid a new infinite debate about unit management in OH, I would suggest to cut this PR in two, one for REST item (to be debated) , and one for REST sitemap. Idea is to not delay the REST sitemap part which is waited.

I removed the changes to the item REST part and will do that in a separate PR. However, I am not sure this is solving the potential problem. The unit is retrieved in ItemUIRegistryImpl, and the changes there are necessary for supporting BasicUI. If it is available there, it will be in the REST api, or am I missing something? If it is not available, it will also not work in BasicUI.

lolodomo · 2023-06-14T06:45:20Z

However, I am not sure this is solving the potential problem. The unit is retrieved in ItemUIRegistryImpl, and the changes there are necessary for supporting BasicUI. If it is available there, it will be in the REST api, or am I missing something? If it is not available, it will also not work in BasicUI.

Basic UI uses code in ItemUIRegistryImpl + REST sitemap API but does not rely on REST item API.

mherwege · 2023-06-14T06:57:30Z

Basic UI uses code in ItemUIRegistryImpl + REST sitemap API but does not rely on REST item API.

It uses exactly the same code to get the unitSymbol in ItemUIRegistryImpl and the proposed change to the REST item API:

        if (item instanceof NumberItem numberItem) {
            unitSymbol = numberItem.getUnitSymbol();
        }

Do we have a stronger guarantee that getUnitSymbol returns the unit from the unit metadata when called from ItemUIRegistryImpl, and not the default unit if the metadata is not available yet? I don't know. If that's not the case, I don't see a difference. And it suggests that overriding the default unit should actually not be stored in the metadata, but should be a field in NumberItem to avoid this kind of situation. But that's far beyond the scope of this PR, and is independent of it.

Adjust BasicUI slider unit handling. So far, the unit was not passed on to the slider. With proposed changes in core openhab/openhab-core#3644, unit would be passed and needs to stripped for the slider to work properly. Signed-off-by: Mark Herwege <[email protected]>

lolodomo · 2023-06-14T09:56:30Z

Basic UI is now ready for this change (PR is merged).

lolodomo · 2023-06-16T06:28:21Z

@openhab/core-maintainers : please tell us if there is an intention or not to review & merge this PR before OH4 is released. I need this answer to review & merge the right Basic UI PR from @mherwege (input field).

J-N-K

Otherwise LGTM, even though iterating over the regex seems to be expensive.

@lolodomo Did you try if that works?

J-N-K · 2023-06-18T08:19:20Z

...org.openhab.core.ui/src/main/java/org/openhab/core/ui/internal/items/ItemUIRegistryImpl.java

+ }
+ pattern = pattern.substring(0, matcherEnd) + (!unit.isBlank() ? " " + unit : "");
+ }
+


remove empty line

lolodomo · 2023-06-18T11:39:34Z

@lolodomo Did you try if that works?

No, I did not try it myself.
But I hope @mherwege did try it.

mherwege · 2023-06-18T14:18:38Z

But I hope @mherwege did try it.

Yes, I did try.

Signed-off-by: Mark Herwege <[email protected]>

mherwege · 2023-06-19T07:30:49Z

@J-N-K I removed the regex iteration. As this check is only done for number items, there should only be one % format string, so I can just take the first one and declare invalid what comes after. I tested with various patterns (valid and invalid) and it still does what I expect it to do.

Signed-off-by: Mark Herwege <[email protected]>

lolodomo · 2023-07-02T06:42:35Z

@J-N-K : any chance to merge this PR before new milestone is generated ?

J-N-K

LGTM, thanks.

Fix openhab/openhab-webui#1951 Signed-off-by: Laurent Garnier <[email protected]>

…penhab#3644) * expose widget format pattern to sitemap REST * Add unit fields to sitemap REST Signed-off-by: Mark Herwege <[email protected]> GitOrigin-RevId: 5ceaa64

Fix openhab/openhab-webui#1951 Signed-off-by: Laurent Garnier <[email protected]> GitOrigin-RevId: b2934ad

This PR is a continuation of #1787, using the extra functionality provided by core if openhab/openhab-core#3644 gets merged in core. It would be an alternative to #1787 with the same functionality, but reduced complexity. Requires openhab/openhab-core#3644. --------- Signed-off-by: Mark Herwege <[email protected]>

Fix openhab/openhab-webui#1953 Fix a regression introduced by PR openhab#3644 Signed-off-by: Laurent Garnier <[email protected]>

…3708) Fix openhab/openhab-webui#1953 Fix a regression introduced by PR #3644 Signed-off-by: Laurent Garnier <[email protected]>

mherwege requested a review from a team as a code owner June 5, 2023 16:06

lolodomo suggested changes Jun 10, 2023

View reviewed changes

mherwege force-pushed the ui_pattern branch from 4f73aab to cc05b9c Compare June 12, 2023 06:57

mherwege mentioned this pull request Jun 12, 2023

[BasicUI] Adjust slider unit handling openhab/openhab-webui#1922

Merged

mherwege mentioned this pull request Jun 13, 2023

[basicui] Use sitemap input hint v2 openhab/openhab-webui#1923

Merged

This was referenced Jun 13, 2023

[rest] Add unit to item response #3654

Merged

[basicui] Use sitemap input hint openhab/openhab-webui#1787

Closed

J-N-K reviewed Jun 18, 2023

View reviewed changes

mherwege added 3 commits June 19, 2023 09:10

expose widget format pattern to sitemap REST

99ec7db

Signed-off-by: Mark Herwege <[email protected]>

Add unit fields to sitemap REST

8fb8ab5

Signed-off-by: Mark Herwege <[email protected]>

Java 17 syntax improvement

8cfac2c

Signed-off-by: Mark Herwege <[email protected]>

mherwege added 3 commits June 19, 2023 09:11

review feedback

f5d319f

Signed-off-by: Mark Herwege <[email protected]>

revert item REST changes

2d6585a

Signed-off-by: Mark Herwege <[email protected]>

review feedback

ca7fff2

Signed-off-by: Mark Herwege <[email protected]>

mherwege force-pushed the ui_pattern branch from 076b35a to 6cf1d6e Compare June 19, 2023 07:28

no regex iteration

d7ce903

Signed-off-by: Mark Herwege <[email protected]>

mherwege force-pushed the ui_pattern branch from 6cf1d6e to d7ce903 Compare June 19, 2023 07:45

J-N-K approved these changes Jul 2, 2023

View reviewed changes

J-N-K added enhancement An enhancement or new feature of the Core REST/SSE labels Jul 2, 2023

J-N-K changed the title ~~Add widget state pattern and default unit to ItemUIRegistry interface and REST API response~~ [rest] Add widget state pattern and default unit to ItemUIRegistry Jul 2, 2023

J-N-K merged commit 5ceaa64 into openhab:main Jul 2, 2023

lolodomo mentioned this pull request Jul 5, 2023

[Basic UI] Frames generate errors with null items openhab/openhab-webui#1951

Closed

4 tasks

lolodomo added a commit to lolodomo/openhab-core that referenced this pull request Jul 5, 2023

Avoid warning log introduced by PR openhab#3644

ff2c494

Fix openhab/openhab-webui#1951 Signed-off-by: Laurent Garnier <[email protected]>

J-N-K pushed a commit that referenced this pull request Jul 5, 2023

Avoid warning log introduced by PR #3644 (#3686)

b2934ad

Fix openhab/openhab-webui#1951 Signed-off-by: Laurent Garnier <[email protected]>

lolodomo mentioned this pull request Jul 12, 2023

[Sitemap UI] Extra space for item state with Number: items openhab/openhab-webui#1953

Closed

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023

Avoid warning log introduced by PR openhab#3644 (openhab#3686)

6eee3a4

Fix openhab/openhab-webui#1951 Signed-off-by: Laurent Garnier <[email protected]> GitOrigin-RevId: b2934ad

wborn added this to the 4.0 milestone Jul 15, 2023

lolodomo mentioned this pull request Jul 17, 2023

[Sitemap UI] Do not break format defined in label with extra spaces #3708

Merged

J-N-K pushed a commit that referenced this pull request Jul 17, 2023

[Sitemap UI] Do not break format defined in label with extra spaces (#…

f711373

…3708) Fix openhab/openhab-webui#1953 Fix a regression introduced by PR #3644 Signed-off-by: Laurent Garnier <[email protected]>

jlaur mentioned this pull request Jul 21, 2023

[Sitemap] Space added when formatting Number:Time with pattern %1$tH #3722

Closed

mherwege deleted the ui_pattern branch July 28, 2023 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rest] Add widget state pattern and default unit to ItemUIRegistry #3644

[rest] Add widget state pattern and default unit to ItemUIRegistry #3644

mherwege commented Jun 5, 2023

lolodomo commented Jun 9, 2023

lolodomo left a comment

lolodomo Jun 10, 2023

lolodomo Jun 10, 2023

mherwege Jun 11, 2023

lolodomo Jun 10, 2023

J-N-K Jun 10, 2023

lolodomo commented Jun 10, 2023

mherwege commented Jun 11, 2023

lolodomo commented Jun 12, 2023 •

edited

Loading

lolodomo commented Jun 12, 2023 •

edited

Loading

mherwege commented Jun 12, 2023

spacemanspiff2007 commented Jun 13, 2023

lolodomo commented Jun 13, 2023 •

edited

Loading

lolodomo commented Jun 13, 2023

lolodomo commented Jun 13, 2023

mherwege commented Jun 13, 2023

mherwege commented Jun 13, 2023

lolodomo commented Jun 14, 2023

mherwege commented Jun 14, 2023

lolodomo commented Jun 14, 2023

lolodomo commented Jun 16, 2023

J-N-K left a comment

J-N-K Jun 18, 2023

mherwege Jun 18, 2023

lolodomo commented Jun 18, 2023 •

edited

Loading

mherwege commented Jun 18, 2023

mherwege commented Jun 19, 2023 •

edited

Loading

lolodomo commented Jul 2, 2023

J-N-K left a comment

[rest] Add widget state pattern and default unit to ItemUIRegistry #3644

[rest] Add widget state pattern and default unit to ItemUIRegistry #3644

Conversation

mherwege commented Jun 5, 2023

lolodomo commented Jun 9, 2023

lolodomo left a comment

Choose a reason for hiding this comment

lolodomo Jun 10, 2023

Choose a reason for hiding this comment

lolodomo Jun 10, 2023

Choose a reason for hiding this comment

mherwege Jun 11, 2023

Choose a reason for hiding this comment

lolodomo Jun 10, 2023

Choose a reason for hiding this comment

J-N-K Jun 10, 2023

Choose a reason for hiding this comment

lolodomo commented Jun 10, 2023

mherwege commented Jun 11, 2023

lolodomo commented Jun 12, 2023 • edited Loading

lolodomo commented Jun 12, 2023 • edited Loading

mherwege commented Jun 12, 2023

spacemanspiff2007 commented Jun 13, 2023

lolodomo commented Jun 13, 2023 • edited Loading

lolodomo commented Jun 13, 2023

lolodomo commented Jun 13, 2023

mherwege commented Jun 13, 2023

mherwege commented Jun 13, 2023

lolodomo commented Jun 14, 2023

mherwege commented Jun 14, 2023

lolodomo commented Jun 14, 2023

lolodomo commented Jun 16, 2023

J-N-K left a comment

Choose a reason for hiding this comment

J-N-K Jun 18, 2023

Choose a reason for hiding this comment

mherwege Jun 18, 2023

Choose a reason for hiding this comment

lolodomo commented Jun 18, 2023 • edited Loading

mherwege commented Jun 18, 2023

mherwege commented Jun 19, 2023 • edited Loading

lolodomo commented Jul 2, 2023

J-N-K left a comment

Choose a reason for hiding this comment

lolodomo commented Jun 12, 2023 •

edited

Loading

lolodomo commented Jun 12, 2023 •

edited

Loading

lolodomo commented Jun 13, 2023 •

edited

Loading

lolodomo commented Jun 18, 2023 •

edited

Loading

mherwege commented Jun 19, 2023 •

edited

Loading