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

DateRangePicker.clear() throws NPE #54

Closed
Dudeplayz opened this issue Dec 10, 2020 · 4 comments
Closed

DateRangePicker.clear() throws NPE #54

Dudeplayz opened this issue Dec 10, 2020 · 4 comments
Assignees
Labels
bug Something isn't working

Comments

@Dudeplayz
Copy link

The default implementation of clear in HasValue uses the default getEmptyValue, which is null. I am not sure what should happen if clear is called, but a NPE should never occur. I think it should jump back to the defaultModel.

java.lang.NullPointerException: null
	at software.xdev.vaadin.daterange_picker.ui.DateRangePicker.tryFixInvalidModel(DateRangePicker.java:355) ~[vaadin-date-range-picker-2.0.0.jar:2.0.0]
	at software.xdev.vaadin.daterange_picker.ui.DateRangePicker.updateFromModel(DateRangePicker.java:333) ~[vaadin-date-range-picker-2.0.0.jar:2.0.0]
	at software.xdev.vaadin.daterange_picker.ui.DateRangePicker.setValue(DateRangePicker.java:501) ~[vaadin-date-range-picker-2.0.0.jar:2.0.0]
	at software.xdev.vaadin.daterange_picker.ui.DateRangePicker.setValue(DateRangePicker.java:63) ~[vaadin-date-range-picker-2.0.0.jar:2.0.0]
	at com.vaadin.flow.component.HasValue.clear(HasValue.java:179) ~[flow-server-2.4.2.jar:2.4.2]
@AB-xdev AB-xdev added the bug Something isn't working label Dec 10, 2020
@AB-xdev AB-xdev self-assigned this Mar 25, 2021
AB-xdev added a commit that referenced this issue Mar 25, 2021
* Throwing now on clear()
* Implemented getEmptyValue() and isEmpty()
* Refactored some code
* Improved docs
@AB-xdev
Copy link
Member

AB-xdev commented Mar 25, 2021

This should be fixed with a2854ff

When calling clear() an UnsupportedOperationException is thrown.

@AB-xdev AB-xdev closed this as completed Mar 25, 2021
@AB-xdev AB-xdev mentioned this issue Mar 25, 2021
@Dudeplayz
Copy link
Author

Why not using the default model for clear?

@AB-xdev
Copy link
Member

AB-xdev commented Apr 13, 2021

2 reasons:

  1. Technical:
    There are constructors that use the defaultModel (maybe we should better rename it to initialModel) directly - without a supplier.
    Due to this the defaultModel is used directly by the component (changes are written immediately to it), which means, that even when you call clear() nothing will happen, because the object would be the same.
  2. Logically:
    The clear() method shouldn't even be available, because when you clear a date (or date-range) what is its default value?
    It could be null - which is not a date -
    or a fixed LocalDate (e.g. LocalDate.MIN) - which doesn't "clears" a field.
    So there's a bit of a dilemma here 😐

@Dudeplayz
Copy link
Author

That's true. I understand the problem now. I would suggest two additional ideas:

  1. Allowing to pass a default value to the constructor, which is set, when clear is called. This would cause the problem, which default value should be used if none is supplied.
  2. Store the relevant state information from the initialModel and recover/set these on clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants