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

[basicui] Use sitemap input hint #1787

Closed
wants to merge 23 commits into from

Conversation

mherwege
Copy link
Contributor

@mherwege mherwege commented Mar 10, 2023

This PR builds on #1729 and requires openhab/openhab-core#3418.

The PR will be rebased once the final version of #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:

  • number: a QuantityType will be represented as a number field with a non-changeable unit field. Updates will be assumed to be in the unit of the old value.
  • date: a datepicker is provided. This datepicker uses the standard html input type=date parameter. Therefore behaviour will be browser specific. Older browsers may revert to a text field. Also the representation is dependent on system or browser (for Chrome) locale.
  • time: similarly a time picker is provided.
  • datetime: uses the html input type=datetime-local, provided a combined date and time picker, will also be browser dependent.

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 mherwege requested a review from a team as a code owner March 10, 2023 23:34
@lolodomo lolodomo added enhancement New feature or request basic ui Basic UI labels Mar 11, 2023
@mherwege mherwege changed the title [basicui] WIP - Use sitemap input hint [basicui] Use sitemap input hint Mar 15, 2023
@mherwege
Copy link
Contributor Author

mherwege commented Mar 15, 2023

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
Copy link
Contributor Author

mherwege commented Mar 15, 2023

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
Copy link
Contributor Author

mherwege commented Mar 15, 2023

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
Copy link
Contributor Author

mherwege commented Mar 15, 2023

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

image

@mherwege
Copy link
Contributor Author

mherwege commented Mar 15, 2023

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
Copy link
Contributor Author

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

@lolodomo
Copy link
Contributor

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

florian-h05 pushed a commit that referenced this pull request Apr 5, 2023
This adds support for configuring the inputHint parameter of the Input sitemap element in the sitemap editor of MainUI.
See openhab/openhab-core#3418 and the BasicUI implementation #1787.

Also-by: Florian Hotze <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
@lolodomo
Copy link
Contributor

lolodomo commented Apr 8, 2023

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

@mherwege
Copy link
Contributor Author

mherwege commented Apr 10, 2023

@lolodomo Rebase done. I struggled a bit as things had diverged a bit.
I did not include your last suggestion on #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
Copy link
Contributor

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

@mherwege
Copy link
Contributor Author

mherwege commented May 1, 2023

@lolodomo Rebase done.

@mherwege
Copy link
Contributor Author

mherwege commented May 1, 2023

@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).

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review

} else if ("time".equals(inputHint)) {
dataState = ((DateTimeType) state).format("%1$tT");
} else if ("datetime".equals(inputHint)) {
dataState = ((DateTimeType) state).format("%1$tY-%1$tm-%1$tdT%1$tT");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using a different format than when no hint is provided ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTML5 input types date, time and datetime-local expect a ISO formatted datetime string. This will not be visible to the user when these input types are used. If the inputHint is text (or none), not HTML5 input type will be used, only a plain text field. The pattern in that case removes the "T" separator.
In the Android app, we ultimately forced an input type when the itemType is DateTime, not leaving the option for a text input. For consistency, we could take this approach here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have now forced a datetime inputHint for DateTime items, but still need to keep some of the code as older browsers may not support the input types and default to text.

@lolodomo
Copy link
Contributor

lolodomo commented May 6, 2023

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
Copy link
Contributor

lolodomo commented May 6, 2023

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
Copy link
Contributor

lolodomo commented May 6, 2023

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
Copy link
Contributor Author

mherwege commented May 6, 2023

@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
Copy link
Contributor Author

mherwege commented May 6, 2023

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
Copy link
Contributor Author

mherwege commented May 6, 2023

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

But not when the servlet is initially created.

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]>
Signed-off-by: Mark Herwege <[email protected]>
Signed-off-by: Mark Herwege <[email protected]>
@mherwege
Copy link
Contributor Author

mherwege commented Jul 2, 2023

As the relevant core PR openhab/openhab-core#3644 has now been merge, I also updated that one with the review feedback in this PR. It may be easier to check out that one instead. I didn't test yet, but I believe the issue with difference between UI config and file config impact would not appear there, as it is covered in core.

@lolodomo
Copy link
Contributor

lolodomo commented Jul 2, 2023

Do you mean this PR should be closed in favor of #1923 ?

@mherwege
Copy link
Contributor Author

mherwege commented Jul 2, 2023

Do you mean this PR should be closed in favor of #1923 ?

Yes, I think so. #1923 contains everything in this one, but simplified as core offers better support.

@lolodomo
Copy link
Contributor

lolodomo commented Jul 2, 2023

So I let you close it.
I will switch to the other PR.
My code review was almost finished.
I will test the other PR with a fresh new server starting tomorrow.

@mherwege
Copy link
Contributor Author

mherwege commented Jul 2, 2023

Replaced by #1923.

@mherwege mherwege closed this Jul 2, 2023
lolodomo pushed a commit that referenced this pull request Jul 14, 2023
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]>
@mherwege mherwege deleted the sitemap_input_hint branch July 28, 2023 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
basic ui Basic UI enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants