Time not updated after .val() #24

Merged
merged 1 commit into from May 28, 2012

Projects

None yet

2 participants

@joliss
Contributor
joliss commented May 14, 2012

Calling .val() on the input field followed by timepicker('getTime') does not return the new time.

I stumbled on this while writing unit tests for my app. I'm attaching a failing test case.

@joliss
Contributor
joliss commented May 14, 2012

Here is some code to reproduce in a JS console:

input = $('<input type="text"></input>').appendTo('body'); input.timepicker().val('1:15 PM'); input.timepicker('getTime')
# => undefined
@wvega wvega merged commit 37694cd into wvega:master May 28, 2012
@wvega
Owner
wvega commented May 28, 2012

hi @joliss, the problem here is that TimePicker stores the selected time in a private variable that gets updated when the setTime API method is updated or the change event is triggered. Calling the val method does not trigger a change event.

If I try to use always the actual value of the input field as the selected time there may be a lost of information, and I'm not sure if that's acceptable. Let me try to explain:

Using the private variable, if you call setTime method to set the time to '11:40:34 PM', that's the value stored in the interval variable; the hour, minutes and seconds are all preserved. Now consider the time format for our example timepicker is set to h p, the value of the input field would be just 11 PM (no minutes and no seconds).

Using the stored value, when you call getTime you get a Date object with all information originally set: 11:40:34 PM. If I start giving priority to the value of the input field, because now we consider the user may have used jQuery.fn.val to change the value to something else, then, the getTime method would return a Date object with just the hour part: 11 PM, because that's all information I can extract from the input field.

That's the loss of information I'm concerned about and some of the tests are failing because of that. I need to decide whether to update the tests to ignore the loss of data, possibly harming users of previous versions or giving up on considering the input field's value and encourage users to use setTime to alter the time of their timepickers.

What do you think about this? Is there a third solution that I'm not considering?

I really appreciate your feedback. Thank you.

@joliss
Contributor
joliss commented Jun 8, 2012

Hey Willington,

Sorry for the long delay.

I see your point about losing information. As a user of timepicker, I wouldn't be surprised if it lost information when the granularity of my field is lower. In other words, if I need seconds, I'd always have the seconds show.

That said, you obviously can't break it for existing users.

Now having to call .change after .val in test code is obviously not a big problem, but the issue I see with relying on change events is that it might not trigger reliably. For example, if you type into the field, it'll only trigger on blur. If we read the value before that, or the browser fires the blur event late or not at all, we'll miss the new value. (You can work around this with input events, which should trigger instantly, but I believe browser support is not universal, and I've seen very weird effects happening with IE9.)

Because of that, my suggestion would be to change it and then bumping the major version to 2.0.0. Following semantic versioning, people should expect backwards-incompatible changes between major versions.

That said, it's up to you obviously. If you think preserving information is a critical feature, then leaving it as it is (and perhaps removing my failing test) would be the best course of action I can think of.

@wvega wvega added a commit that referenced this pull request Jul 28, 2013
@wvega getTime now takes into account values set using $.fn.val() (#24)
AFAIK, I managed to avoid the loss of precision discussed in
#24 (comment).
c1a91be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment