Ticket #294 (closed defect: fixed)

Opened 4 months ago

Last modified 4 months ago

Potential issues in MochiKit.Visual.pulsate with unused code and suspicios usage of bind()

Reported by: cederberg@gmail.com Assigned to: cederberg@gmail.com
Priority: normal Milestone: MochiKit 1.4
Component: component1 Version:
Severity: normal Keywords:
Cc:

Description (Last modified by cederberg@gmail.com)

It seems the MochiKit.Visual.pulsate() function uses MochiKit.Base.bind() incorrectly. I think I need an extra pair of eyeballs to check the following code for sanity (see my inlined XXX-comments): 

/** @id MochiKit.Visual.pulsate */
MochiKit.Visual.pulsate = function (element, /* optional */ options) {
    /***

    Pulse an element between appear/fade.

    ***/
    var d = MochiKit.DOM;
    var v = MochiKit.Visual;
    var b = MochiKit.Base;
    var oldOpacity = MochiKit.Style.getStyle(element, 'opacity');
    options = b.update({
        duration: 3.0,
        from: 0,
        afterFinishInternal: function (effect) {
            MochiKit.Style.setStyle(effect.element, {'opacity': oldOpacity});
        }
    }, options);
    var transition = options.transition || v.Transitions.sinoidal;
// XXX: Why do we bind the function with "this" set to transition? Seems unnecessary.
    var reverser = b.bind(function (pos) {
        return transition(1 - v.Transitions.pulse(pos, options.pulses));
    }, transition);
// XXX: The following line doesn't change anything, right?
    b.bind(reverser, transition);
// XXX: If someone sets options.transition, the pulsating effect is totally lost here. Instead we should
// overwrite options.transitions, since the value has already been taken into consideration... I think.
    return new v.Opacity(element, b.update({
        transition: reverser}, options));
};

Change History

03/23/08 09:26:15 changed by bob@redivi.com

Nice work!

Yeah, it looks like both uses of bind are superfluous, and you are also correct about the call to update. It should probably be calling something like 

    opts = b.merge(options, {transition: reverser});

or 

    opts = clone(options);
    opts.transition = reverser;

03/23/08 14:02:42 changed by cederberg@gmail.com

  • owner changed from somebody to cederberg@gmail.com.
  • status changed from new to assigned.
  • type changed from task to defect.
  • description changed.

Ok. I'll fix and test this one tomorrow then.

03/24/08 01:18:27 changed by cederberg@gmail.com

  • status changed from assigned to closed.
  • resolution set to fixed.

Fixed and committed to trunk.