Announcement

Collapse
No announcement yet.
X
  • Filter
  • Time
Clear All
new posts

    question about DynamicForm.compareValues method and handling of NaN values

    Hi there, we are noticing an issue where DynamicForm.compareValues(NaN,NaN,null,true); always returns false. It seems that method is not checking for isNaN() when inspecting the parameters passed since NaN==NaN will always return false. This is causing calls for exiting the editing a grid cell to incorrectly report that some values which are NaN have changed when calling _storeEditValue() > fieldValuesAreEqual() > compareValues()

    Is the following patch of DynamicForm.compareValues() sensible and should it be applied to the framework? See the comment below like this: //updated to compare isNaN values


    Code:
    compareValues : function (value1, value2, field, exactEquality) {
    
    
    
        if (field) {
            if(field.type) {
            var simpleType = isc.SimpleType.getType(field.type);
            if (simpleType && simpleType.compareValues) {
                return simpleType.compareValues(value1, value2, field) == 0;
            }
    
    
                if (field.type == "valueMap") {
                    if (isc.isAn.Array(value1) && isc.isAn.Array(value2)) {
                        return value1.equals(value2)
    
                    } else if (isc.isAn.Object(value1) && isc.isAn.Object(value2)) {
                        for (var i in value1) {
                            if (value2[i] != value1[i]) return false;
                        }
    
                        for (var j in value2) {
                            if (value1[j] != value2[j]) return false;
                        }
    
                        // everything matched
                        return true;
                    }
                }
            }
    
            if (isc.isA.Date(value1) && isc.isA.Date(value2)) {
                if ((isc.SimpleType.inheritsFrom(field.type, "date") &&
                    !isc.SimpleType.inheritsFrom(field.type, "datetime")) ||
                    value1.logicalDate || value2.logicalDate)
                {
                    return (isc.DateUtil.compareLogicalDates(value1, value2) == 0);
                } else if (isc.SimpleType.inheritsFrom(field.type, "time") || value1.logicalTime || value2.logicalTime) {
                    return (isc.Time.compareLogicalTimes(value1, value2) == 0);
                } else {
                    return (isc.DateUtil.compareDates(value1, value2) == 0);
                }
            }
        }
    
        if (isc.isAn.Array(value1) && isc.isAn.Array(value2)) {
            if (value1.length != value2.length) return false;
            for (var i = 0; i < value1.length; i++) {
    
                if (!isc.DynamicForm.compareValues(value1[i], value2[i], field)) {
                    return false;
                }
            }
            return true;
        }
    
        // handle having values set to Number, String etc instance
        // IE var foo = new Number(2); rather than just var foo = 2;
        // This returns true for isA.Object()
        if (isc.isA.Number(value1) || isc.isA.String(value1) || isc.isA.Boolean(value1)) {
            value1 = value1.valueOf();
        }
        if (isc.isA.Number(value2) || isc.isA.String(value2) || isc.isA.Boolean(value2)) {
            value2 = value2.valueOf();
        }
    
        // if either value is a DateRange, compare the start and end attributes
        if ((value1 && value1._constructor == "DateRange") || (value2 && value2._constructor == "DateRange")) {
            return isc.DynamicForm.compareValues(value1 && value1.start, value2 && value2.start, field) &&
                   isc.DynamicForm.compareValues(value1 && value1.end, value2 && value2.end, field);
        }
    
    
        //updated to compare isNaN values
        if ((exactEquality && value1 === value2) || (!exactEquality && value1 == value2) || (isNaN(value1) && isNaN(value2))) {
            return true;
        }
    
        if (isc.isAn.Object(value1) && isc.isAn.Object(value2)) {
            var recursive = isc.DynamicForm.compareValuesRecursive,
                tempObj = isc.addProperties({}, value2),
                isSGWT = isc.Browser.isSGWT
            ;
            for (var attr in value1) {
    
                if (isSGWT && (attr == isc.gwtRef || attr == isc.gwtModule)) {
                    // assume SGWT wrapper and module reference always match
                } else if (recursive) {
                    if (!isc.DynamicForm.compareValues(value1[attr], value2[attr])) {
                        return false;
                    }
                } else {
                    if (value2[attr] != value1[attr]) return false;
                }
                delete tempObj[attr];
            }
            // tempObj should now be empty if they match
            for (var attr in tempObj) {
                return false;
            }
            return true;
        }
        return false;
    },

    #2
    Hi there, we also see a similar issue with undefined values not being evaluated properly by the fieldValuesAreEqual() method when called in the storeUpdatedEditorValue() method. The method storeUpdatedEditorValue() forces the value returned from _parseEditorValue() to null if it is undefined before it is passed to setEditValue(). However, the oldValue remains as undefined in the _storeEditValue() method which is called by storeUpdatedEditorValue() > setEditValue(). As a result, a call to fieldValuesAreEqual() will return a true because value has been forced to null in storeUpdatedEditorValue() while oldValue is undefined.

    These issues with NaN (in my initial message) and undefined values are causing unneeded calls to calculateRecordSummaries() in the storeUpdatedEditorValue() method because the changed variable is being erroneously set to true.

    We are implementing workarounds on our end for these issues but wanted to let you know and get your feedback.

    Comment


      #3
      We've incorporated your suggested change to check for NaN explicitly in DynamicForm.compareValues() - that will be present in nightly builds going forward.

      The _parseEditorValue() case (undefined vs null) is harder to comment on without seeing the problem in action as there are valid cases where we want to consider an empty string from a grid edit as an explicit "null" to be sent to the server.
      We're happy to take a deeper look here but to do so I think we'll need to see exactly what you're doing. Any chance you can either share steps to reproduce the problem in one of our showcase examples, or show us a runnable snippet of code with instructions to reproduce the issue?

      Regards
      Isomorphic Software

      Comment


        #4
        Thanks for the update. I tried briefly to recreate the issue I was seeing with null vs undefined but no luck yet. Will post a sample if I see it again.

        Comment

        Working...
        X