rhysbrettbowen/PlastronJS

onChange event does not fire with Boolean attribute

gmkumar2005 opened this issue · 5 comments

I have a bind function which is executed when model changes. When the model contains boolean attribute it does not get fired.
Apparently due to line 523 in model.js

return this.prev(key) != this.get(key);

in the above code LHS and RHS return false, hence it returns false. but a true is expected.
How ever it works fine when a string datatype is used in the model
Below is the proper test case. plz note I have set opt_silent for second change.

var menuModel1;

var setUp = function() {
    menuModel1 = new bs.models.menuModel();
};

var testOnChangeWithString = function() {

    var ran = 0;
    var that;
    var bindfn = function(isChecked) {
        ran++;
        that = isChecked;
    };

    menuModel1.bind(['isChecked'],bindfn);


    menuModel1.set('isChecked','one');
    assertEquals(1, ran);
    assertEquals('one', that);

    menuModel1.set('isChecked','two',true);
    assertEquals(1, ran); // should not run 
    assertEquals('one', that);

    menuModel1.set('isChecked','three');
    assertEquals(2, ran);
    assertEquals('three', that); 
};

var testOnChangeWithBoolean = function() {

    var ran = 0;
    var that;
    var bindfn = function(isChecked) {
        ran++;
        that = isChecked;
    };

    menuModel1.bind(['isChecked'],bindfn);

    menuModel1.set('isChecked',true);
    assertEquals(1, ran);
    assertEquals(true, that);

    menuModel1.set('isChecked',false,true);
    assertEquals(1, ran); // should not run 
    assertEquals(true, that);

    menuModel1.set('isChecked',true);
    assertEquals(2, ran); // test case fails here .
    assertEquals(true, that); 
};

The second test case fails.

i found even the testOnChangeWithString fail. The model is set three times. 1) fires correctly 2) opt_silent is used to suppress the change event 3) does not fire the event when the value set is same as first case.
failed test cases

var menuModel1;

var setUp = function() {
    menuModel1 = new bs.models.menuModel();
};

var testOnChangeWithString = function() {

    var ran = 0;
    var that;
    var bindfn = function(isChecked) {
        ran++;
        that = isChecked;
    };

    menuModel1.bind(['isChecked'],bindfn);


    menuModel1.set('isChecked','one');
    assertEquals(1, ran);
    assertEquals('one', that);

    menuModel1.set('isChecked','two',true);
    assertEquals(1, ran); // should not run 
    assertEquals('one', that);

    menuModel1.set('isChecked','one');    
    assertEquals(2, ran);
    assertEquals('one', that); 


};

var testOnChangeWithBoolean = function() {

    var ran = 0;
    var that;
    var bindfn = function(isChecked) {
        ran++;
        that = isChecked;
    };

    menuModel1.bind(['isChecked'],bindfn);

    menuModel1.set('isChecked',true);
    assertEquals(1, ran);
    assertEquals(true, that);

    menuModel1.set('isChecked',false,true);
    assertEquals(1, ran); // should not run 
    assertEquals(true, that);

    menuModel1.set('isChecked',true);
    assertEquals(2, ran); // test case fails here .
    assertEquals(true, that); 
};

This is expected behaviour as a change is defined by the model when it's different from the last change event.

So if you fire a change event on true or 'one' then your bound function should run on one, then silent change to false or 'two' then your bound event should not fire, so when you change back to true or 'one' the bound function doesn't need to change as it will receive the same information (true or 'one') as when it ran last time.

Is there a case you know where this behaviour is not wanted? Best thing is to run a change event after the second change, but if you need to know if there was any change (Even if the values are different) I'll have a look to see if I can put in an option somewhere

Following is my use-case plz suggest if it can be handled differently
I am using goog.ui.FilteredMenu in my application. When the user clicks on a menuItem (say User) i set the window.location.hash to caption of the menuItem. ( 'User' in this case) . The idea is to make use of route. When the user navigates away from 'User' and later comes back, the page should restore its state. Say highLight the 'User' menuItem.
The problem with goog.ui.FilteredMenu is that it does not have toggle feature ie, more than one menu items can be in CHECKED state. So I have toggleChecked method which will set all the menuitems to unchecked state except the one clicked buy the user. Further FilteredMenu does not know anything about navigation and route.
I need toggleChecked executed in two cases 1) when the user clicks menuItem 2) When user navigates using the anchor #User

In order to achieve this functionality i have created menuModel (inherits mvc.Model)
Case 1) When the user clicks menuItem , the eventListner for menuItem will set the isChecked attribute in the menuModel, The event handler which is bound to menuModel will execute toggleChecked.

menuModel1.bind(['id','isChecked'],toggleChecked);

Case 2) When the user navigates to #User the router will listen to the event. update the isChecked attribute in menuModel. When menumodel is updated toggleChecked is fired.

router.route('User', function(id) {
 var count = list.getLength();
     for (var i = 0; i < count; i++) {
          menuitem = list.at(i);
         if(menuitem.get('caption')===id){
         menuControl.toggleChecked(menuitem.get('id'),menuitem.get('isChecked'));
           }
     }            
});

Now coming to the toggleChecked. Here i need to unset the isChecked attribute of model. To avoid cyclic execution of event listener i have suppressed the change event using opt_silent .

menuControl (inherits mvc.control) contains the event handlers.

The problem in this is
initially the state of menuModel1 is 'zero' and menuModel2 is 'zero'
Step1 -- user clicks on menuModel1 , isChecked changes from 'zero' to 'one' ; menuModel2 does not change
Step2 -- user clicks menuModel2, isChecked changes from 'zero' to 'one; menuModle1 changes from 'one' to 'zero' here set opt_silent to avoid circular events.

 menuModel1.set('isChecked','zero',true);

Step3 -- User clicks menuModel1 , isChecked changes from 'zero' to 'one' ; menuModle2 changes from 'one' to 'zero' ## Here i expect the change event to be fired since the value changed from 'zero' to 'one'

by the looks of it I think that the route should only set the model 'isChecked' rather than call toggleChecked and toggleChecked should only listen to the model. so that way toggleChecked is only listening to changes on the model and will only get fired once.

have a look at: http://modernjavascript.blogspot.com/2012/05/principles-of-mvc-design.html

bascially ou want any changes to state or interactions to the UI that the user does to set the model and you want any changes to the model to set the view. Sounds like toggleChecked is doing both which is where you start to get cycles

Rhys you guessed it right. I am trying to update the model along with the state of the UI. I am doing multiple updates to the model. So one way to handle is listen to change on mvc.collection rather than on the model. I will try tomorrow and will update --Thanks a lot