nativescript-community / ui-lottie

NativeScript plugin to expose Airbnb Lottie
https://github.com/airbnb/lottie-android
Other
177 stars 57 forks source link

Unexpected loop behaviour #16

Closed mudlabs closed 6 years ago

mudlabs commented 6 years ago

When setting the loop property in xml it doesn't follow expected behaviour.

Outcome

Expected


Setting loop property in code-behind does produce expected results.

mudlabs commented 6 years ago

Looks like the loop conditionals in nativescript-lottie.<platform>.js only check if loop is true.

When creating a Lottie or setting its loop value in code-behind this is fine because we're explicitly setting loop to the boolean of true or false. But when set in xml loop is a string, so any value is true. Which is why loop="false" results in a looped animation.

Current

// nativescript-lottie.ios.js
// inside LottieView.prototype.createAnimationView
if (this.loop) {
  this._animationView.loopAnimation = this.loop;
}

// nativescript-lottie.android.js
LottieView.prototype[
  nativescript_lottie_common_1.loopProperty.setNative
] = function(loop) {
  if (loop) {
    this.nativeView.loop(true);
  } else {
    this.nativeView.loop(false);
  }
};

Proposed change

// nativescript-lottie.ios.js
if(RegExp("^true$", "i").test(this.loop)){
  // ...
}

// nativescript-lottie.android.js
if(RegExp("^true$", "i").test(loop)){
  // ...
}

This change will fix the issue of false or irregular values being passed into loop via xml, while still allowing boolean assignment from code-behind.

mudlabs commented 6 years ago

Oh, and the same is true for autoPlay.

So it might be best to export a testTruthiness function from common.js.

rhanb commented 6 years ago

Hi @mudlabs ,

Thank you for your report! I managed to reproduce the issue and you're right, the loop value can be a string if it's set directly in XML (loop="false").

if (this.loop) { // can be a string
  this._animationView.loopAnimation = this.loop;
}

but in the setNative function the value is already typed to boolean so no need to check the type

  [loopProperty.setNative](loop: boolean) { // function called when `loop` property set through data binding
    if (loop) { 
      this.nativeView.loop(true);
    } else {
      this.nativeView.loop(false);
    }

Same for autoPlay. I'll make a PR 👍

rhanb commented 6 years ago

@mudlabs instead of exporting a testTruthiness function from common.js, I'll do the following:

export const loopProperty = new Property<LottieViewBase, boolean>({
    name: "loop",
    defaultValue: false, // default value
    valueConverter: booleanConverter //always a boolean
});