Wrong initial value shown (timespinner control)

[trueqbit]

Hi Boris,

I am having a hard time to implement a time spinner tag control. Apart from having to create my own ui widget because of jquery-ui's dependency on the legacy globalize library, the issue with jsviews boils down to the fact that the underlying value is different from the rendered (displayed) representation: the spinner widget keeps a timestamp internally but formats that timestamp into a localised time string.

The problem is that the input element initially displays the data value instead of the localised string.

Here is a fiddle showcasing the problem. It originates from the initialization sequence of the tag control: callAfterLink() calls and extended version of tag.setValue(), which is a locally derived function. The custom base part just updates the widget, but later the derived part compares the initially retrieved input value with the data value (linkCtx._val !== val). If the values aren't the same then it directly sets the data value on the linked input element (depending on the type of linked element).

I see 3 ways how to solve that:

1. In Tag.onBind() retrieve the raw value from the linked element and store it in linkCtx._val [tag.linkCtx._val = +tag.linkedElem.val()] (which should be done anyway?)
2. In Tag.onBind() retrieve the calculated value from the widget and store it in linkCtx._val [tag.linkCtx._val = this.widget._parse(tag.linkedElem.val())]
3. Rely on custom setValue() to have updated the widget properly

Issues:

1. Depends on the initial input value set properly. If the jsrender template isn't sent by a server placing the value there, it needs to be set by a jsrender API call. Both require additional setup when it's a computed value (like when the timestamp is computed from minutes and hours). Also this method isn't available for the tag-only syntax?
2. Depends on knowing what tag.widget._parse() actually does - the value to be parsed might be different from the outcome, which is cumbersome to handle as well (and proven to be in case of timestamps)
3. Question: why does the derived part set the value after the custom base part?
   1. Could this be swapped?
   2. Does it actually make sense to set the input value directly at all? Since the final rendered value depends on the widget's parsing it makes sense to delegate that

I have a solution for now (#1) but I am trying to think generically: once jquery-ui would use the new globalize library and it's possible to set options again, I wouldn't need a custom widget and tag control anymore, but could reuse the spinner with different options, so the described problem would become apparent later.

Let me know what you think.

[trueqbit]

Hah! I've just discovered that I could simply store the set value in the linked context object:

...
setValue: function(value) {
    this.widget.value((value || 0).toString());
    this.linkCtx._val = +value;
},
....

Is this intended to be used like this?

[BorisMoore]

I need to look more closely and get back to you. I think you may have hit on something which needs tweaking (in which case thanks again - you feedback is very helpful) - but I need to get some time to take a look. Given that you have a workaround, I may take longer to come back on this...

[BorisMoore]

Hi Klaus - getting back to you on this, see https://github.com/BorisMoore/jsviews/issues/360#issuecomment-291613759 for a proposed approach to easily creating a timespinner. Also, no need to set linkCtx._val, with the new update provided there...

[trueqbit]

Hi Boris,

I am currently reviewing the new test jsviews library you linked from #360 (comment). I am not sure but it's probably more appropriate (last but not least due to the topic's length) to continue commenting specifics of the spinner/timespinner on this thread.

Regarding the spinner control:

1. Is there a specific reason you set the convert function conditionally but not convertBack?
2. Additionally I'd like to suggest a 'change' event handler pairing 'spin' (in case (tag.tagCtx.props.trigger === false). This would cover the situation when the user has entered a value, which needs to be interpreted and set back after the input field lost focus.

So spinner's complete 'options' control parameter would look like this:

options: function() {
  var tag = this;
  var opts = {
    spin: function(evt, ui) {
      tag.update(ui.value);
    }
  };

  if (tag.tagCtx.props.trigger === false) {
    // convert and set value entered by user
    opts.change = function(evt, ui) {
      // [note: might trigger another change event]
      tag.setValue(tag.convert(tag.widget.value()));
      tag.update(tag.widget.value());
    };
  }
  return opts;
},

Regarding a timespinner control, I created another github gist, showing a possible implementation I would consider generic (it also shows how to use the newer globalizejs 1.x library).

If you ask me the specific purpose of a time spinner and the practice of actually deriving from a base class justifies its own control, hence it could be worth including it in your collection of jquery-ui tag controls? Then again formatting and parsing are tied to using the globalizejs library... If you like the idea you could at least incorporate it as an example, such that others have a ready-to-use tag control. After all even such a simple requirement as entering a time of day is complex enough.

[BorisMoore]

Hi Klaus,

Just to let you know that my version of spinner, timespinner and datepicker (along with the associated jsviews.js) is evolving, and I will post the latest here soon for you to take a look at if you would like to. It's probably better to wait for that version rather than spending additional time on the version I posted a week ago.

The options.change event handler should not be necessary with the update. That behavior is already provided by the underlying jsviews.js - but there was a bug in an earlier version which meant that the overriding of convert/convertback was preventing trigger happening with onchange...

Conditional setting of convert/convertBack is a bit different in the new version, so I won't go into that until you have the update...

[BorisMoore]

Commit 85, just published, includes changes and improvements to {{datepicker}} and {{spinner}}, and a new {{timespinner}}, along with documentation. See in particular:

• http://www.jsviews.com/#samples/tag-controls/jqui/datepicker/formats
• http://www.jsviews.com/#samples/tag-controls/jqui/spinner/formats
• http://www.jsviews.com/#samples/tag-controls/jqui/timespinner

@trueqbit - let me know if your scenarios are covered by these changes. Thanks!

[trueqbit]

Better late than never I want to let you know that the new timespinner control is working as expected (not to say great!) [using v0.9.90 Beta].

One thing i noticed is the keepDay condition in jsviews-jqueryui-widgets.js, which is if (value && !tag.keepDay) .... Shouldn't it be the other way around? Also, if using a timestamp for the data format, then the call to value.getHours() fails.

[BorisMoore]

Thanks Klaus. If keepDay is true, then we don't want to create a new Date() instance - we stay on the same date, So I think that code is correct. (And indeed, setting {{timespinner ... keepDay=true /}} has the desired behavior.) I don't see value.getHours() failing, either. Can you provide a small sample showing the failure?

[trueqbit]

I must admit that I don't comprehend how value's value comes into being, just by reading the code I had the impression that because you are creating another Date object from tag.value the intention was to 'keep the same day' as tag.value. I actually hit that part only because my data is a computed timestamp, and I thought that specifying keepDay isn't necessary, but the !tag.keepDay branch assumes that value is a date object, which fails in this fiddle (note that you have to set keepDay to true). That said it's working as expected, so I guess everything is fine. Nevertheless it might be interesting for you how others are using the control.

[BorisMoore]

Hi Klaus, My response above was too rapid. In fact you are correct, there are some issues around keepDay=true. I am opening this issue. I have a candidate fix, here: jsviews-jqueryui-widgets.js.txt Can you test whether it works correctly for you, both with keepDay true and keepDay false.

For your fiddle, there is one issue, which is that your displayFormat.parse needs to return a Date:

parse: function parse(value, props) {
  ...
  return new Date(value);
}

With that change it should work correctly with the new version of jsviews-jqueryui-widgets.js. Let me know of any remaining issues.

Thanks again for calling this out.

[trueqbit]

Hi Boris, I updated the parsing function accordingly - it actually originated from extending jqueryui's spinner widget.

The candidate fix works on my side!

[BorisMoore]

Great, thanks. I'll include that fix in the next update...

[BorisMoore]

This has been fixed in release v0.9.91.