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

datepicker doesn't make use of converter during encodeBegin #558

Closed
andbi opened this issue Nov 21, 2016 · 14 comments
Closed

datepicker doesn't make use of converter during encodeBegin #558

andbi opened this issue Nov 21, 2016 · 14 comments
Labels
Milestone

Comments

@andbi
Copy link
Contributor

andbi commented Nov 21, 2016

It looks like it happens during component initialisation when initial value has already been given (long type in my case). Here is the stacktrace:

21-Nov-2016 23:05:41.971 SEVERE [http-nio-8080-exec-750] org.apache.catalina.core.StandardWrapperValve.invoke Servlet.service() for servlet [Faces Servlet] in context with path [/trade-web] threw exception [Value could be either String or java.util.Date, you may want to use a custom converter.] with root cause java.lang.IllegalArgumentException: Value could be either String or java.util.Date, you may want to use a custom converter. at net.bootsfaces.component.datePicker.DatePicker.getDateAsString(DatePicker.java:462) at net.bootsfaces.component.datePicker.DatePicker.encodeHTML(DatePicker.java:353) at net.bootsfaces.component.datePicker.DatePicker.encodeBegin(DatePicker.java:192) at javax.faces.component.UIComponentBase.encodeAll(UIComponentBase.java:527)

@zhedar
Copy link
Collaborator

zhedar commented Nov 21, 2016

Can you provide an example? How did you define the converter?

@zhedar
Copy link
Collaborator

zhedar commented Nov 21, 2016

Oh, wait. You did specify a long as a date? We didn't implement this use case, especially as you can use a Date, long seems a bit indirect for date representation. That's why there's the hint with providing your custom converter.

If you don't want to implement a new converter, would using a Date instead of a long be a quick workaround for you?

@andbi
Copy link
Contributor Author

andbi commented Nov 21, 2016

The stacktrace is self-explanatory, one can see from the flow of the source code that no converter is being employed. Here's the markup
<b:datepicker id="sd" value="#{testBean.startTime}" format="dd.MM.yyyy" ajax="true" update=":f:config"> <f:converter converterId="calendarConverter"/> </b:datepicker>

and the converter class:

`
@FacesConverter(value = "calendarConverter")
public class CalendarConverter implements Converter {

private static final Logger LOGGER = Logger.getLogger(CalendarConverter.class.getName());
private final SimpleDateFormat DATE_FORMAT = new SimpleDateFormat("dd.MM.yyyy");

// and so on
`

the converter is created but neither of its methods is being called.

@zhedar, what do you mean by "we didn't implement this use case"? The whole story about converters is that you actually don't need to worry about different use cases, just tell me what your component expects and it's gonna be my responsibility to use proper converter.

@zhedar
Copy link
Collaborator

zhedar commented Nov 21, 2016

You're right, but I originally thought, that you maybe expected it to handle long values as well (sometimes we get those misconceptions).

This looks like something, that shouldn't happen. I will have a look at this later on and maybe we can get this done this week, so it will be in the next hotfix release.

@andbi
Copy link
Contributor Author

andbi commented Nov 21, 2016

I'll fix the code and create a pull request, in the meantime I will be using snapshot with this issue fixed, cause the project is really great and I can't wait for so long))

@stephanrauh
Copy link
Collaborator

Can you already estimate when you're submitting your pull request? We're currently pondering about to release version 1.0.1, so we'd like to include your PR if you're submitting it early enough. What about next weekend? If that's too early, I guess we're going to schedule a 1.0.2, too :).

@andbi
Copy link
Contributor Author

andbi commented Nov 21, 2016

@stephanrauh I have already fixed the code, the question now is whether the DateTimePicker component should be changed the same way. So we're talking about a day or two tops here.

@stephanrauh
Copy link
Collaborator

Some time ago, we've decided to call b:datePicker deprecated in favor of b:dateTimePicker (because b:datePicker requires jQueryUI, while b:dateTimePicker does not). So it's more important to fix b:dateTimePicker (if it's broken) than b:datePicker. It's good that you thought of both components!

BTW, I'm sure we won't publish a new release before Sunday. So take your time.

@andbi
Copy link
Contributor Author

andbi commented Nov 22, 2016

@stephanrauh I've fixed both components so that they now rely on provided converters. That's all I've done, no errors are now thrown on initialisation. However I had no time to perform full tests plus I was unable to ajaxify any of the two components, which means I'm reverting back to the Primefaces's Calendar or plain text input.

There's another very important point I'd like to make with regard to these two components. Forcing Java(!!!) developers to stick to a date/time format of a third-party JS(!!!) library is a very bad idea, especially when it comes to date/time conversion and Faces converters in particular. How one is expected to perform that conversion?

So I would rewrite the components completely so that they use normal Java's date/time formats which in turn would be used to disassemble component value and internally convert it to whatever format is required for that JS library. Developers are supposed then to specify that Java format (which then doesn't get overridden by locale settings like in DateTimePicker), or use a default one which would depend on locale in use and showDate/showTime settings. The logic of that default format should be clear to developers cause they will need to rely on it (reproduce it) when creating converters.

As for the minor fixes I've made, I will create a pull request, please have a look and test it. That will fix the original issue, but, again, the internals of the two components are not ready for converters, I would even say they are disrespecting general JSF concepts.

@asterd
Copy link
Collaborator

asterd commented Nov 22, 2016

Hi @0swald, first of all, thank you for your contribution.. we will look at your PR asap.
Now, I don't understand what do you mean in your comment: the DateTimePicker component provide the option to specify the value as a Date object and, if you don't specify the "format" attribute, it assume that you use a "moment-like" format. But if you specify your format string, you can use whatever you want. So, where is the problem? There is an internal utility (LocaleUtils) that manage the translation between java Date format and moment.js library format, that, IMHO, is a normal way to provide transformation between this two incompatible formats.

Another point: what do you mean when you say "I was unable to ajaxify any of the two components"?

@andbi
Copy link
Contributor Author

andbi commented Nov 22, 2016

@asterd , the docs are saying (as far as I remember) that the format must comply with the moments one. Now imagine you were to use DateTimePicker in a situation like mine, dates are of type long (could be any type, it's up to me). Then, if DateTimePicker followed the concept "ValueHolder + Converter", the only change you would need to make in your possibly huge code is to create a new FacesConverter. And to create one, you would need a Java library for conversion between moments and your internal date/time type. That's jumping through hoops.

So it shouldn't be "provide the option to use Date objects", instead it should be able to use any types in the first place (given that a converter is provided), that's just what ValueHolder and FacesConverter are about, and that's the concept that is broken. Furthermore, Faces converters are about converting between String and Object back and forth, that's why I offered to stick internally to a String format as a core of the two components.

As for the ajax, the DateTimePicker is not triggering <f:ajax>, adding ajax=true and custom process/update attributes resulted in changes of internal String representation of dates and thus made it impossible to use a FacesConverter. That's where I had to stop experimenting and decided to abandon the component until it starts respecting the ValueHolder interface.

@andbi
Copy link
Contributor Author

andbi commented Nov 22, 2016

@asterd, forgot to mention that "if you specify your format string then you can use whatevet you want" is not true, and I've already mentioned that - the provided format gets changed by locale settings, which, again, makes it impossible to use faces converters.

@asterd
Copy link
Collaborator

asterd commented Nov 22, 2016

@0swald, I understand your point and in this case, I think that your help on this can be a good addition to the project. I'll try to take a look at the component to follow your request.. but if you can help us, it was good!

@andbi
Copy link
Contributor Author

andbi commented Nov 22, 2016

@asterd I will gladly join fixing the component but I can make no commitments as to the dates and releases. Besides, in this particular case, there might be a better approach:

The author(s) of the component create a "test environment" consisting of a markup and two Java classes:

  1. A bean class with a field of an "internal" date/time type, it could be for example MyDateType.java with a single String parameter in a format like "dd|MM|yyyy hh:mm:ss" or six integers.
  2. A FacesConverter for the above mentioned MyDateType.java.

Then they are offered to test the component in three different ViewRoot locales: en_GB, en_US and de_DE. I bet that'd be enough to fix all the major ValueHolder issues, plus it'd take much less time than if I decided to get involved on this stage.

@zhedar zhedar added the bug label Nov 22, 2016
zhedar pushed a commit that referenced this issue Nov 27, 2016
zhedar pushed a commit that referenced this issue Nov 27, 2016
@zhedar zhedar added this to the v1.0.1 milestone Nov 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants