Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#15825 closed defect (fixed)

dojox/mobile/SpinWheelSlot does not notify for changes to value.

Reported by: Ed Chatelain Owned by: Eric Durocher
Priority: undecided Milestone: 1.8.1
Component: DojoX Mobile Version: 1.8.0rc1
Keywords: Cc:
Blocked By: Blocking:

Description

dojox/mobile/SpinWheelSlot does not update it's value attribute when it is changed, so we can not watch for changes. This is causing a problem when trying to use SpinWheelSlot? with dojox/mvc.

Attachments (2)

test_SpinWheel-custom-mvc.html (3.7 KB) - added by Ed Chatelain 7 years ago.
I have added some mvc bindings to the test_SpinWheel-custom.
patch15825.patch (4.6 KB) - added by Adrian Vasiliu 7 years ago.
a) Fix: watches are now notified after interactive value changes; b) Fix: support any keys in "items" (used to work only for 0..n values); c) Addition to DOH and interactive test files. Adrian Vasiliu, IBM, CCLA

Download all attachments as: .zip

Change History (15)

comment:1 Changed 7 years ago by Damien Mandrioli

This probably should be improved but according to the API doc, the value property is just the *initial* value.

comment:2 Changed 7 years ago by Adrian Vasiliu

Yes, the situation is a bit confusing, because the attribute "value" is the initial value (as documented), while the property "value" is the currently shown value. That is, slot.value = "2014" changes the initial value, while slot.set("value", "2014") changes the shown value (and slot.get("value") returns the shown value).

Now, if dojox/mvc needs to watch for the changes of the shown value, there's indeed a feature missing here. For updates of the shown value done programmatically (slot.set("value", newValue)), SpinWheelSlot does go through _WidgetBase._set("value", newValue), thus the user can listen to value changes this way (for example):

var picker = registry.byId("picker");
var yearSlot = picker.slots[0];
yearSlot.watch("value", function(name, oldVal, newVal){
  console.log("watch name: " + name + " oldVal: " + oldVal + " newVal: " + newVal);
});

This watch is notified as expected for programmatic changes of the shown value. But it is *not* notified for interactive changes of the value. To be notified in the interactive case, if you can afford using your own subclass of SpinWheelSlot, adding this override:

stopAnimation: function(){
  this._set("value", this.get("value"));
  this.inherited(arguments);
},		

does the trick (I first tried overriding "spin" or "slideTo", but this doesn't give the final value, while stopAnimation is always called after the last interactive value change).

I'm not sure if such a solution is satisfactory for dojox/mvc - in particular if it needs to work with any SpinWheelSlot (not only with such a subclass). Unless I'm missing some other way to get it work (in Dojo 1.8 as-is), I think we should indeed improve the widget to ensure the watches of value changes (if any) are always notified by default.

Last edited 7 years ago by Adrian Vasiliu (previous) (diff)

comment:3 Changed 7 years ago by Adrian Vasiliu

In fact, it may be possible to avoid subclassing SpinWheelSlot for being able to watch the interactive changes of the shown value. Instead, just listen to "onFlickAnimationEnd" events and call slot.get("value") to get the new value. I can't test it before tomorrow, but I think it would work.

comment:4 Changed 7 years ago by Adrian Vasiliu

I confirm that listening to "onFlickAnimationEnd" does allow to watch the interactive changes. Better, listen to "onYearSet"/"onMonthSet"/"onDaySet" on the date picker, say:

var picker, yearSlot, monthSlot, daySlot;
onYearSet = function(){
  if (!picker) picker = registry.byId("picker");
  if (!yearSlot) yearSlot = picker.slots[0];
  console.log("onYearSet value: " + yearSlot.get("value"));
}
onMonthSet = function(){
  if (!picker) picker = registry.byId("picker1");
  if (!monthSlot) monthSlot = picker.slots[1];
  console.log("onMonthSet value: " + monthSlot.get("value"));
}
onDaySet = function(){
  if (!picker) picker = registry.byId("picker");
  if (!daySlot) daySlot = picker.slots[2];
  console.log("onDaySet value: " + daySlot.get("value"));
}
...
<div id="picker" data-dojo-type="dojox/mobile/SpinWheelDatePicker"
  onYearSet="onYearSet()" onMonthSet="onMonthSet()" onDaySet="onDaySet()">
</div>

Note however that this will not catch the setting of the initial values (unfortunately, I would say), it catches only the interactive changes done after init time.

If also interested in watching the programmatic changes of the values, you can combine this with

var picker = registry.byId("picker");
var yearSlot = picker.slots[0];
yearSlot.watch("value", function(name, oldVal, newVal){
  console.log("watch name: " + name + " oldVal: " + oldVal + " newVal: " + newVal);
}); // ditto for month and day

as I mentioned yesterday.

If this fits the dojox/mvc needs, I think we can close this ticket ("worksforme" or the like), while we can plan for some doc improvements. edchat, please say if this works for you.

comment:5 Changed 7 years ago by Micha

I'm a bit confused.

I tried to use SpinWheel/SpinWheelSlot? in a dojox.mobile/dojox.app application and tried to bind a model value to the SpinWheelSlot?'s value attribute like that:

<div id="settings" data-dojo-type="dojox/mobile/SpinWheel">
	<div id="settingsSlot" data-dojo-type="dojox/mobile/SpinWheelSlot"
		data-dojo-props="
			items: [
				['key1', 'value1'], 
				['key2', 'value2'], 
			],
			value: at('rel:', 'selectedValue')
		" style="text-align:center;width:100%;">
	</div>
</div>

So, I read above that the value attribute just contains the initial value and as a result of that no data binding in the markup is possible. This is a major break in the design for dojox.mvc and a serious defect in consequence because dojox.mvc is pushed as a major technique.

Will there be no support for dojox.mvc data bindings or did I misunderstood the comments in this ticket (sounds like it's gonna be closed)?

According to that, the initial value doesn't work for items with keys like in my example. It seems like the spin wheel can only center the value, if the items array uses keys created by the labels attribute (0,1,2,...). But it would be nice to apply string values to the items, which are not used as labels.

So, this works:

data-dojo-props="items: [[0, 'first'], [1, 'second'], [2, 'third']], value: 'second'"

but this won't work:

data-dojo-props="items: [[3, 'first'], [4, 'second'], [5, 'third']], value: 'second'"

and this doesn't work too:

data-dojo-props="items: [['key1', 'first'], ['key2', 'second'], ['key3', 'third']], value: 'second'"

So I guess the items attribute is useless at the moment, cause only the keys created by the labels attribute seem to work. The documentation says:

An array of array of key-label paris

There is no hint, that only array based indexes starting with 0 will work. ;)

comment:6 Changed 7 years ago by Ed Chatelain

I am sorry that I was not able to reply to this sooner. dojox/mvc takes advantage of the fact that in general widgets use a dojo/stateful for their values, so we can watch the value, and also set the value in order to update it. The dojox/mvc code is not designed to be able to watch something like "onFlickAnimationEnd", and update something else. So I think the update to add the stopAnimation function should be made to SpinWheelSlot?.js, I think it should go into 1.8.1, but if it is not added to 1.8.1 any applications wanting to use dojox/mvc with SpinWheelSlot? can add the stopAnimation function themselves with a monkey patch, but that is a bit ugly.

comment:7 Changed 7 years ago by Adrian Vasiliu

No, it doesn't sound "like it's gonna be closed" (as Micha said) ;-) We started from the description saying it's not possible to watch for value changes, and I provided a way to do that, and I just said that, _if_ this fits your needs, I think we can close the ticket. Now we know we can't :-)

edchat: could please test whether the override of stopAnimation does the trick for the binding mechanism? Or, if there's already a test case in dojox/mvc or dojox/app, please point me to it such that I can test.

Concerning the issue with the items property: "There is no hint, that only array based indexes starting with 0 will work" (Micha). Indeed, I reproduced. Not sure if it can be considered a bug or a known but undocumented limitation (most docs and code examples I've seen refer to "labels", very few to "items", and I'm not sure how much plus-value is in "items"). Anyway, I prepared a patch but for now it gives an error in a DOH test, the polishing should be ready by tomorrow. (That said, this issue is unrelated, but we could treat them both here.)

Changed 7 years ago by Ed Chatelain

I have added some mvc bindings to the test_SpinWheel-custom.

comment:8 Changed 7 years ago by Ed Chatelain

Hi Adrian, I did verify that adding the stopAnimation update works with the mvc binding. And I have attached the test I was playing with test_SpinWheel-custom-mvc.html.

comment:9 Changed 7 years ago by Adrian Vasiliu

Great, thanks. I'll attach the candidate patch (for both issues) when I'll finish the testing.

Changed 7 years ago by Adrian Vasiliu

Attachment: patch15825.patch added

a) Fix: watches are now notified after interactive value changes; b) Fix: support any keys in "items" (used to work only for 0..n values); c) Addition to DOH and interactive test files. Adrian Vasiliu, IBM, CCLA

comment:10 Changed 7 years ago by cjolif

In [29531]:

refs #15825. a) Fix: watches are now notified after interactive value changes; b) Fix: support any keys in "items" (used to work only for 0..n values); c) Addition to DOH and interactive test files. Thanks Adrian Vasiliu, IBM, CCLA

comment:11 Changed 7 years ago by cjolif

Resolution: fixed
Status: newclosed

In [29532]:

fixes #15825. a) Fix: watches are now notified after interactive value changes; b) Fix: support any keys in "items" (used to work only for 0..n values); c) Addition to DOH and interactive test files. Thanks Adrian Vasiliu, IBM, CCLA

comment:12 Changed 7 years ago by cjolif

Milestone: tbd1.8.1

comment:13 Changed 7 years ago by cjolif

In [29538]:

refs #15825. Fix wrongly committed tests.

Note: See TracTickets for help on using tickets.