openhab / openhab-webui

Web UIs of openHAB
Eclipse Public License 2.0
213 stars 233 forks source link

[basicui] Use sitemap input hint #1787

Closed mherwege closed 11 months ago

mherwege commented 1 year ago

This PR builds on https://github.com/openhab/openhab-webui/pull/1729 and requires https://github.com/openhab/openhab-core/pull/3418.

The PR will be rebased once the final version of https://github.com/openhab/openhab-webui/pull/1729 has been merged. While it is feature complete from my perspective, testing and feedback are much appreciated.

This PR uses the sitemap Input widget's inputHint parameter to adjust the characteristics of the input field. The inputHint parameter will have the following impact:

The date and time pickers use standard html input type parameters. That restricts the formatting and will result in a browser dependent representation. This however, is the solution that can be achieved with least effort and least code. I also did not find good date/time pickers for Material Design Lite, that could easily be brought into the code.

mherwege commented 1 year ago

Testing with following sitemap:

sitemap thuis label="Input Test" {
 Frame {
  Input icon=text     item=Test             label="String input text [%s]"                                  valuecolor=["blue"]
  Text  icon=text     item=Test             label="String test [%s]"
  Input icon=niveau   item=NumberTest       label="Number input text [%.1f]"                                valuecolor=[>10="red","blue"] labelcolor=[>10="red","blue"] 
  Input icon=niveau   item=NumberTest       label="Number input number [%.1f]"          inputHint="number"  valuecolor=[>10="red","blue"] labelcolor=[>10="red","blue"]
  Text  icon=niveau   item=NumberTest       label="Number test [%.3f]"
  Input icon=energy   item=QuantityTypeTest label="Quantity input text [%.0f %unit%]"
  Input icon=energy   item=QuantityTypeTest label="Quantity input number [%.0f %unit%]" inputHint="number"
  Text  icon=energy   item=QuantityTypeTest label="Quantity test [%.3f %unit%]"
  Input icon=calendar item=Datum            label="Date input text"                                          valuecolor=["blue"]
  Input icon=calendar item=Datum            label="Date input text formatted [%1$td/%1$tm/%1$tY]"
  Input icon=calendar item=Datum            label="Date input date"                     inputHint="date"     valuecolor=["blue"]
  Input icon=time     item=Datum            label="Time input time"                     inputHint="time"     valuecolor=["green"]
  Input icon=calendar item=Datum            label="Date input datetime"                 inputHint="datetime" valuecolor=["orange"]
  Text  icon=calendar item=Datum            label="Date test [%1$td/%1$tm/%1$tY %1$tR]"
 }
}

Shows at start of OH (using Chrome), no persisted data:

image
mherwege commented 1 year ago

Filling out a few fields:

image

Notice the comma in the Number input text field, vs the dot in the Number input number field. The input number fields respects the browser setting, which I have set to English GB, while the input text field uses the value from OH as is. My regional settings use a comma.

After entering 10 in the Quantity input text field:

image

The unit is taken from the default, and then also set in the Quantity input number field. It cannot be overriden there.

mherwege commented 1 year ago

Entering a date in the Date input date field:

image

Results in:

image

A few notes on this: date, time and datetime will revert to text fields when the browser doesn't support this (very old browsers). The only supported text field date input format is YYYY-MM-DD hh:mm:ss or a subset thereof (only date or only time). When using a formatted text field for a date, it will show in the different format, but cannot be entered as such, so is not adviced (see Date input text formatted field).

mherwege commented 1 year ago

The Date input datetime field also shows a time picker (as would the Time input time) field:

image
mherwege commented 1 year ago

I have tested this on Chrome and Firefox on Windows, and to a lesser extend on Edge. Firefox is a bit specific with different controls and a clear button on the date, time and datetime input fields I cannot hide. Using the clear button will however always reset to the previous value. Firefox also does not display a popup time picker, rather does inline time editing. I don't have a Mac to test on Safari.

Here is an image on Firefox:

image

And for Edge (with different browser locale settings):

image

It also should look fine in dark theme and condensed mode.

I did not test on mobile browsers so far (not sure how to easily do that when running from a development environment), but I do expect the controls will actually be more specific to the use.

mherwege commented 1 year ago

@lolodomo It is up to you how you want to proceed with this review. This PR already includes https://github.com/openhab/openhab-webui/pull/1729, with all review adjustments from the PR. There is some extra refactoring in this one to properly accomodate support for the inputHint parameter, https://github.com/openhab/openhab-core/pull/3418. You could first continue the review on https://github.com/openhab/openhab-webui/pull/1729, and after merging that one look at this one, or I can also close https://github.com/openhab/openhab-webui/pull/1729 and we continue here (after merging https://github.com/openhab/openhab-core/pull/3418). Let me know what you prefer.

lolodomo commented 1 year ago

Please keep #1729 , we are going to first finalize and merge #1729.

lolodomo commented 1 year ago

@mherwege : I let you rebase (the other PR is now merged) and then I will test and review this PR.

mherwege commented 1 year ago

@lolodomo Rebase done. I struggled a bit as things had diverged a bit. I did not include your last suggestion on https://github.com/openhab/openhab-webui/pull/1729 I included in that PR. I already had a working correction for this in this PR, and kept that. I did try if it still works correctly for the case the correction was meant for. Thank you for all your effort so far on reviewing this. This one should now also be ready for review.

lolodomo commented 1 year ago

@mherwege : can you please rebase it again, there is a conflict in _theming.scss. I will then start the tests and the review.

mherwege commented 1 year ago

@lolodomo Rebase done.

mherwege commented 1 year ago

@lolodomo In the review of the Android PR, a change in behaviour was made for dateTime type items.

  1. If the item is of type dateTime, set a default inputHint to datetime (avoids text input for dateTime items)
  2. If only date is set through input element, append time value from previous
  3. If only time is set through input element, set the date part to the previous value

I have not made a change to this PR (yet), but I am unsure if, and how, I can.

1: I need text input for datetime anyway to allows for browsers not supporting the HTML5 input types. 2 and 3: The Android app has direct access to the item state. Here I only have access to the formatted state at the moment of doing a change (in the js onChange function).

lolodomo commented 1 year ago

2 and 3: The Android app has direct access to the item state. Here I only have access to the formatted state at the moment of doing a change (in the js onChange function).

Just because you removed the code that was providing this information. You have this information each time an item state is updated through the function _t.setValuePrivate. In the current version (before your changes), the item state is saved in a variable named lastItemState that can be then used in function onChange.

lolodomo commented 1 year ago

First tests.

For any kind of item, if I enter a value while the current state is NULL, the item state is updated in the server but "empty/NULL" values are restored in UI.

For number with dimension, whatever input hint (unset, set to text or set to number), if I enter a number, I see the command correctly send, the item state updated in server but the value is restored to the previous value in UI. Edit: seems to work now ! Very strange.

For number item without dimension with a state pattern set to [%f], I can see that the ending 0 are removed when input hint is set to number while not when set to text or unset. As an example, if I enter value 123.45, the display value is finally 123.45 with hint set to number while 123,450000 when not. Looks like the state pattern is not respected ?

For number item without dimension with a state pattern set to [] and input hint set number, no unit is displayed. I would expect to see the unit from the item state (which is °C in my use case).

For date time item and input hint set to date, selecting a new date is correctly sent to the server but then the old date is restored.

For date time item and input hint set to time, if I enter the hour, I see just after the hour disappearing and remains only 2 fields.

More generally, all stuff with date time seems to not work at all in Firefox, I was not able to fill a proper date & time.

lolodomo commented 1 year ago

I guess the biggest problem should be in function setValuePrivate as it is the function that updates the UI when an item state is received. It is mostly not working. I will have a look to the JavaScript and try to find where is the problem.

Edit: date seems to work better now, I am a little lost. Maybe the problem is when the initial item state is NULL.

mherwege commented 1 year ago

@lolodomo Not behind my computer at the moment, but for many of your tests, it looks like the SSE connection was not working properly. Values should be restored if the SSE connection does not send the final update. And it looks like that is what is happening.

mherwege commented 1 year ago

For number item without dimension with a state pattern set to [] and input hint set number, no unit is displayed. I would expect to see the unit from the item state (which is °C in my use case).

This is expected. The state pattern is not a unit, it could be anything. When inputHint is number, only number or number with dimension is allowed. For number with dimension, the unit is shown. Any state description beyond number formatting is ignored. I don’t see an easy way around this. State descriptions were meant for visualization, never for input patterns.

mherwege commented 1 year ago

You have this information each time an item state is updated through the function _t.setValuePrivate.

But not when the servlet is initially created.

lolodomo commented 1 year ago

You have this information each time an item state is updated through the function _t.setValuePrivate.

But not when the servlet is initially created.

That is true but if necessary you can pass it from Java code to client code in a data field when the page is created.

lolodomo commented 1 year ago

For number item without dimension with a state pattern set to [] and input hint set number, no unit is displayed. I would expect to see the unit from the item state (which is °C in my use case).

This is expected. The state pattern is not a unit, it could be anything. When inputHint is number, only number or number with dimension is allowed. For number with dimension, the unit is shown. Any state description beyond number formatting is ignored. I don’t see an easy way around this. State descriptions were meant for visualization, never for input patterns.

You did not understand my use case. I have disabled the state pattern and I am expecting to see the unit of the item state. Look at one of my review comments in the Java file, you apparently used the wrong variable and it explains this wrong result. Edit: my original review commert was not about unit display. I added a new comment to fix this use case.

lolodomo commented 1 year ago

but for many of your tests, it looks like the SSE connection was not working properly. Values should be restored if the SSE connection does not send the final update. And it looks like that is what is happening.

Yes, it could explain what I saw. What is strange is that I never encountered any problem with SSE and I used Ctrl+F5 to clear my cache. I will try again.

lolodomo commented 1 year ago

No, it was not SSE as it was working for one item.

lolodomo commented 1 year ago

Just to show the two little bugs that I can see when there is not state description: Second frame at right, missing value: image The corresponding sitemap:

        Frame label="Input number hint number" {
                Input item=TestNumber inputHint="number" labelcolor=["green"] valuecolor=["red"]
                Input item=TestNumber label="Test number no state descr []" inputHint="number"
        }

Second frame at right, missing value and unit: image The corresponding sitemap:

        Frame label="Input number:dimension hint number" {
                Input item=CurrentTemp inputHint="number" labelcolor=["green"] valuecolor=["red"]
                Input item=CurrentTemp label="Current Temp no state descr []" inputHint="number"
        }

It is just after opening the page.

The good point is that I know how to fix that, it is in the review comment from yesterday. I am going to test the fix.

lolodomo commented 1 year ago

With such item:

Number TestNumber "Test number [%.3f]" (GTest1)

and this sitemap:

        Frame label="Input number no hint" {
                Input item=TestNumber labelcolor=["green"] valuecolor=["red"]
                Input item=TestNumber label="Test number no state descr []"
        }
        Frame label="Input number hint number" {
                Input item=TestNumber inputHint="number" labelcolor=["green"] valuecolor=["red"]
                Input item=TestNumber label="Test number no state descr []" inputHint="number"
        }
        Frame label="Input number hint text" {
                Input item=TestNumber inputHint="text" labelcolor=["green"] valuecolor=["red"]
                Input item=TestNumber label="Test number no state descr []" inputHint="text"
        }

When I update the state to value 234.5 with console (so outside UI):

11:47:18.476 [INFO ] [openhab.event.ItemCommandEvent       ] - Item 'TestNumber' received command 234.5
11:47:18.492 [INFO ] [openhab.event.ItemStateChangedEvent  ] - Item 'TestNumber' changed from 123.45 to 234.5

the display of value does not respect the state pattern while input hint is set to number. It is ok when no input hint or when input hint is set to text. See second frame value at left: image All values at right are OK as there is no state description, the value from item is considered as expected.

mherwege commented 1 year ago

@lolodomo With the TestNumber example you provided above, I also tried creating a Text widget:

Text item=TestNumber label="Test number no state descr []"

FWIW, the number also does not show in this case. Your suggestions do make sense though. I am working through them.

lolodomo commented 1 year ago

FWIW, the number also does not show in this case.

Yes, you're right and that is expected. But for an input field, I believe we should have the current state to be able to update it? But now I have a doubt.... By the way, it should be consistent whatever the input hint.

lolodomo commented 1 year ago

Your suggestions do make sense though. I am working through them.

So I do not start updating your code and I am waiting for your next push. I hope you will be a little available during next week as I would like to merge this PR next weekend to have it included in milestone 3.

mherwege commented 1 year ago

the display of value does not respect the state pattern while input hint is set to number. It is ok when no input hint or when input hint is set to text.

I think I know what is causing this. The reason is the state pattern %.3f. It will interpret something like "2,450" as 2450, as it contains 3 digits after the comma and therefore settles on English number format in the parseNumber method (java) and function (js). That's the limit of trying to be intelligent in interpreting the numbers. If you change the pattern to %.2f I believe it shows correctly. I don't see any easy way out, apart from trying to compare the displayState and item state. When using the itemState, all formatting is lost. The displayState delivers a format that may be in comma format (depending on international settings) and needs to be parsed. But I don't know what the locale format is. And even if I get it on the Java side, the browser may use a different locale, so it may still be out of sync. I don't see an easy solution yet.

mherwege commented 1 year ago

@lolodomo Ready for a next review.

2 issues you may want to look into, and I would appreciate your view on.

  1. Number parsing for %.3f formats
  2. When starting BasicUI with a Numer:Dimension item with state NULL or UNDEF, the unit field is not initialized, and therefore, setting a value leads to a js exception when initializing the item. A refresh solves it. I didn't find a why to figure out if a non-initialized item is of type Number:Dimension, and therefore I should create an empty placeholder for the unit in the Java code. If I create the placeholder by default, it takes up space and breaks the allignment for all number items without units.
lolodomo commented 1 year ago

I have run new tests with the latest version. When the items have a state, globally this is working well. There are more problems for date/time when starting from UNDEF state. The state update from UNDEF to non UNDEF for date is also not working very well. Sometimes, for hour/minute/second, only 2 fields are displayed and I can't understand how I can enter the 3 fields in that case.

First, I will report and try to fix the few problems when the page is opened and item states are updated. Second, I will report the few problems when filling inputs while the page was opened with item states already set. Almost no problem. And finally, I will report all the problems when filling inputs while the page was opened with UNDEF items. Concerns mainly dates.

mherwege commented 1 year ago

Sometimes, for hour/minute/second, only 2 fields are displayed and I can't understand how I can enter the 3 fields in that case.

That is a constraint of the HTML5 time input type. They only allow setting hour and minute, not seconds. How relevant is this in the context of OH?

lolodomo commented 1 year ago

Sometimes, for hour/minute/second, only 2 fields are displayed and I can't understand how I can enter the 3 fields in that case.

That is a constraint of the HTML5 time input type. They only allow setting hour and minute, not seconds.

But when seconds are defined in the state, I was able to display and edit them with success.

How relevant is this in the context of OH?

Not important I believe. Would it possible and a good idea to not display them ?

mherwege commented 1 year ago

But when seconds are defined in the state, I was able to display and edit them with success.

How relevant is this in the context of OH?

Not important I believe. Would it possible and a good idea to not display them ?

OK, I see the same thing. It should be possible to cut after minutes and not display seconds. I will look into it.

mherwege commented 1 year ago

Not important I believe. Would it possible and a good idea to not display them ?

Last commit puts in logic to remove the seconds from visualisation and input.

mherwege commented 1 year ago

There are more problems for date/time when starting from UNDEF state. The state update from UNDEF to non UNDEF for date is also not working very well.

This should now be fixed in the last commit.

lolodomo commented 1 year ago

For me, the version before the last one was OK for live update of dates.

For text and number items, if the item state switches to UNDEF, I see "UNDEF" displayed when,there is no state description (right column). image But if you open the page while the state is UNDEF, "UNDEF" is then not displayed. image It is not consistent. I think a blank would be OK in that case.

lolodomo commented 1 year ago

I think you are not handling the color of the new "unit" field: image Should it be colored when the item state is NULL/UNDEF ?

For the date, the widget gets the defined color even when the item state is NULL/UNDEF. image Is it what you would like ? It looks to me not fully consistent.

mherwege commented 1 year ago

It is not consistent. I think a blank would be OK in that case.

fixed

lolodomo commented 1 year ago

Does no more compile with your last change:

[INFO] --- frontend-maven-plugin:1.9.0:npm (gulp build) @ org.openhab.ui.basic ---
[INFO] Running 'npm run build' in D:\dev\openhab3\git\openhab-webui\bundles\org.openhab.ui.basic
[INFO]
[INFO] > org.openhab.ui.basic@4.0.0 build
[INFO] > gulp default
[INFO]
[INFO] [15:20:19] Using gulpfile D:\dev\openhab3\git\openhab-webui\bundles\org.openhab.ui.basic\gulpfile.js
[INFO] [15:20:19] Starting 'default'...
[INFO] [15:20:19] Starting 'css'...
[INFO] [15:20:19] Starting 'copyFontLibs'...
[INFO] [15:20:19] Starting 'eslint'...
[INFO] [15:20:19]
[INFO] D:\dev\openhab3\git\openhab-webui\bundles\org.openhab.ui.basic\web-src\smarthome.js
[INFO]   1662:21  error  Expected '===' and instead saw '=='  eqeqeq
[INFO]   1694:18  error  Expected '!==' and instead saw '!='  eqeqeq
[INFO]
[INFO] ✖ 2 problems (2 errors, 0 warnings)
[INFO]
[INFO] [15:20:19] 'eslint' errored after 519 ms
[INFO] [15:20:19] ESLintError in plugin "gulp-eslint"
[INFO] Message:
[INFO]     Failed with 2 errors
[INFO] Details:
[INFO]     domainEmitter: [object Object]
[INFO]     domainThrown: false
[INFO]
[INFO] [15:20:19] 'default' errored after 535 ms
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  14.756 s
[INFO] Finished at: 2023-05-08T15:20:19+02:00
mherwege commented 1 year ago

@lolodomo Sorry for the build failure. I added color handling for undef values. It was not necessary before as the color of the input widget label is fixed, but unit and datetime placeholders are not put there but in the value fields. So it needs handling.

lolodomo commented 1 year ago

It is better after each commit ;)

You still have to handle the colors when opening the page and item state is NULL/UNDEF. It is OK when state switches to UNDEF while the page is already opened.

Regarding rendering of states and live updates of items, remains only one problem: number item with NULL/UNDEF state, input hint number and no state description. When the item state is updated, the field remains blank. I believe you mentioned yesterday that you have no idea how to handle this case. image It is OK for number item without dimension: image

Regarding date inputs, I like that the format is the French format.

Regarding number input with hint number, I see that the value is displayed with a comma as decimal separator. It uses the French format. Good. image But let see what happens if the item state is 56 and 15 °C: image Strangely it displays "56.00" and "15.0 °C" instead of "56,00" and "15,0 °C".

lolodomo commented 1 year ago

Regarding filling input field for date & time, it works now perfectly. Just one question: I see that the command is sent each time you update one element like the month for example. So if you want to set a new date & date, in fact this will trigger several commands, including wrong (intermediate) values. I was asking myself if the command should not be sent only when the user pushes the ENTER key in the field ? Is it at least possible ?

lolodomo commented 1 year ago

And regarding time filling, there is no time picker available ?

PS: all these tests were run in Firefox. I will test later with Chrome.

lolodomo commented 1 year ago

And regarding time filling, there is no time picker available ?

Ok, I see there is one with Chrome ;) For firefox, is there something missing or does Firefox not support itme picking ?

mherwege commented 1 year ago

And regarding time filling, there is no time picker available ?

PS: all these tests were run in Firefox. I will test later with Chrome.

Chrome shows a time picker, Firefox does not.

lolodomo commented 1 year ago

Chrome shows a time picker, Firefox does not.

Yes and it is better ! Tests with date & time also OK using Chrome on Windows.

mherwege commented 1 year ago

Regarding filling input field for date & time, it works now perfectly. Just one question: I see that the command is sent each time you update one element like the month for example. So if you want to set a new date & date, in fact this will trigger several commands, including wrong (intermediate) values. I was asking myself if the command should not be sent only when the user pushes the ENTER key in the field ? Is it at least possible ?

I don't know if there is a way to control this. However, I don't think it sends multiple updates when you use the picker, only when you edit the field directly. This is the disadvantage of using the standard HTML5 input types. While relatively easy to use and available in all modern browsers, it doesn't allow much control.

mherwege commented 1 year ago

You still have to handle the colors when opening the page and item state is NULL/UNDEF. It is OK when state switches to UNDEF while the page is already opened.

That is done now.

lolodomo commented 1 year ago

Chrome shows a time picker, Firefox does not.

Yes and it is better ! Tests with date & time also OK using Chrome on Windows.

Date & time picker of Microsoft Edge is stricly identical to the Chrome one ! So works well.

lolodomo commented 1 year ago

You still have to handle the colors when opening the page and item state is NULL/UNDEF. It is OK when state switches to UNDEF while the page is already opened.

That is done now.

Does not compile ;)