On Wed, 15 Nov 2023 09:37:23 GMT, brunesto <d...@openjdk.org> wrote:
>> The fix prevents the DatePicker from losing focus if the date is not
>> parsable.
>
> brunesto has updated the pull request incrementally with one additional
> commit since the last revision:
>
> inlined
Looks good to me, just some minor things.
I also dug into the code and found out how this worked in JavaFX 8:
...
try {
value = c.fromString(text);
} catch (Exception ex) {
// Most likely a parsing error, such as DateTimeParseException
}
...
So this approach looks correct to me!
I just wonder if we want to catch `Exception` here as well, as it was done in
the old code.
The contract then would be: If a converter throws any exception, we cancel the
edit.
@andy-goryachev-oracle any objections here from your side?
modules/javafx.controls/src/test/java/test/javafx/scene/control/DatePickerTest.java
line 746:
> 744: @Test
> 745: public void testFocusLostWithTypo() {
> 746: //datePicker.setEditable(true); // other test cases have it .is
> this necessary?
Looks like you can remove this
modules/javafx.controls/src/test/java/test/javafx/scene/control/DatePickerTest.java
line 753:
> 751: // initial value
> 752: datePicker.setValue(LocalDate.of(2015, 03, 25));
> 753: assertEquals("3/25/2015",datePicker.getEditor().getText());
Minor: A space after comma is missing
modules/javafx.controls/src/test/java/test/javafx/scene/control/DatePickerTest.java
line 756:
> 754:
> 755: // set misformatted text
> 756: //stageLoader.getStage().requestFocus(); // other test cases
> have it . is this necessary?
same here, remove as it works without :)
modules/javafx.controls/src/test/java/test/javafx/scene/control/DatePickerTest.java
line 764:
> 762:
> 763: // check that value remains unchanged, and text is reverted
> 764: assertEquals(LocalDate.of(2015, 03, 25),datePicker.getValue());
Minor: A space after comma is missing
modules/javafx.controls/src/test/java/test/javafx/scene/control/DatePickerTest.java
line 765:
> 763: // check that value remains unchanged, and text is reverted
> 764: assertEquals(LocalDate.of(2015, 03, 25),datePicker.getValue());
> 765: assertEquals("3/25/2015",datePicker.getEditor().getText());
Minor: A space after comma is missing
-------------
PR Review: https://git.openjdk.org/jfx/pull/1274#pullrequestreview-1732643564
PR Review Comment: https://git.openjdk.org/jfx/pull/1274#discussion_r1394584158
PR Review Comment: https://git.openjdk.org/jfx/pull/1274#discussion_r1394585258
PR Review Comment: https://git.openjdk.org/jfx/pull/1274#discussion_r1394584665
PR Review Comment: https://git.openjdk.org/jfx/pull/1274#discussion_r1394585403
PR Review Comment: https://git.openjdk.org/jfx/pull/1274#discussion_r1394585502
