vaadin / vaadin-time-picker-flow

Vaadin Flow Java API for vaadin/vaadin-time-picker Web Component
https://vaadin.com/components/vaadin-time-picker
Other
6 stars 11 forks source link

Problem with .setValue(LocalTime.now()); - Invalid regular expression #15

Closed masbaehr closed 5 years ago

masbaehr commented 5 years ago

There seems to be a problem when using

TimePicker tp = new TimePicker();
tp.setValue(LocalTime.now());

image

svipal commented 5 years ago

Hey, I'm working on this and I can't reproduce it.

masbaehr commented 5 years ago

Did you check the JS Console (Chrome) and are you using Vaadin 13.0.1?

Additional information:

image

svipal commented 5 years ago

I cloned the repo and launched the demo with this card :

private void bugDemo2() {
        Div message = createMessageDiv("time-picker-bug-demo2");
        // begin-source-example
        // source-example-heading: Bug
        TimePicker timePicker = new TimePicker();
        timePicker.setValue(LocalTime.now());
        // end-source-example
        addCard("BugDemo2", timePicker, message);
    }

I'm using Firefox; there's nothing wrong in the console. Will retry with Chrome.

svipal commented 5 years ago

image Works fine for me in both bowsers with the current version.

masbaehr commented 5 years ago

Can you try to make a breakpoint at the spot i showed, and post the content of the text variable? I have the suspicion that this is something related to the system locale. As i'm not in the US!

image

svipal commented 5 years ago

I don't really know how to do that from the Vaadin time picker flow side. Adding console.log calls in vaadin-time-picker.html in the vaadin-time-picker-1.2.0.jar used by the demo does nothing sadly.

I'm in France btw.

masbaehr commented 5 years ago

What result do you get if you do the following on your system:

System.out.println(LocalTime.now());

I'm getting 13:35:02.325198200

svipal commented 5 years ago

I get 13:40:14.936 Are you using the latest version of vaadin-time-picker-flow ? Well, I thought it might have been that from your example earlier. Maybe your milliseconds are too many and that's why the \d{1,3} regex fails If that is the case, we can just truncate the milliseconds in the regex instead.

masbaehr commented 5 years ago

Hm - it would be interesting to know why i'm getting a different precision then. Whats your JVM? I'm using Oracle Java JVM 11.0.2 on Win X64

I'm using vaadin 13.0.1 which bundles time-picker-flow 1.1.0

svipal commented 5 years ago

Good point. Openjdk8 on Solus 3.9999

can you update your vaadin-time-picker-html with this and test ?

parseTime: text => {
                    // Parsing with RegExp to ensure correct format
                    const MATCH_HOURS = '(\\d|[0-1]\\d|2[0-3])';
                    const MATCH_MINUTES = '(\\d|[0-5]\\d)';
                    const MATCH_SECONDS = MATCH_MINUTES;
                    const MATCH_MILLISECONDS = '(\\d+)';//changed the regex to allow for any number of digits
                    const re = new RegExp(`^${MATCH_HOURS}(?::${MATCH_MINUTES}(?::${MATCH_SECONDS}(?:\\.${MATCH_MILLISECONDS})?)?)?$`);
                    const parts = re.exec(text);
                    if (parts) {
                      // Allows setting the milliseconds with hundreds and tens precision
                      if (parts[4]) {
                        //truncate the string.
                        parts[4] = parts[4].substring(0,3);
                        while (parts[4].length < 3) {
                          parts[4] += '0';
                        }
                      }
                      return {hours: parts[1], minutes: parts[2], seconds: parts[3], milliseconds: parts[4]};
                    }
                  },
svipal commented 5 years ago

just so we're clear on it, could you also try using the git master version of vaadin-time-picker-flow ?

masbaehr commented 5 years ago

https://stackoverflow.com/questions/52029920/localdatetime-now-has-different-levels-of-precision-on-windows-and-mac-machine

This is probably the cause then. so we should make sure the precision is truncated correctly before applying the regex.

the master version should be the latest which is used in v 13.0.1.

Maybe it would be even better to do this in the java-code of the component

svipal commented 5 years ago

Good job finding that. On it.

svipal commented 5 years ago
@Override
public void setValue(LocalTime value) {
        LocalTime truncated_value = value.truncatedTo(ChronoUnit.MILLIS);
    super.setValue(truncated_value);
}

Tested it and it works, I will make a PR. Send me your email address btw so we can share the bounty.

pleku commented 5 years ago

Closed with #28