Skip to content

Conversation

@m-kubis
Copy link

@m-kubis m-kubis commented May 11, 2018

The animation no longer needs to start from scratch anymore for certain Android versions (#25, #26, maybe #22 and possibly others) in order to work at all.
Fixed issue when only one pulse was moving while the others were not. Also the issue when the animation was not seen.
The behavior should be much more consistent across various versions now.

Tested on 5, 5.1.1, 6, 7, 8, 8.1, P preview

m-kubis added 2 commits April 6, 2018 12:09
AnimatorSet is no longer used as it seems not to work well with animators having ValueAnimator#setCurrentPlayTime(long) set. Also the actual timing of the call itself relative to starting the animation differs between Android versions.
@kanchanakakans
Copy link

as you have said it is resolved but you have not used this animator set where an option is there to play the animation together ..... have implemented this without animatorset but the effect i found out is single pulse moving forward how can i get the effect of multiple pulse generating with use of Animator in list please Prescribe

Copy link

@MuhammadHamzaShahid-9D MuhammadHamzaShahid-9D left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical Issues

Memory Leak Risk in start()


java   for (int x = 0; x < mAnimators.size(); x++) {
       ObjectAnimator objectAnimator = (ObjectAnimator) mAnimators.get(x);
       // ...
       objectAnimator.start();
   }

Each animator is started individually, but if an exception occurs mid-loop, some animators will be running while mIsStarted might not be set properly. Consider wrapping in try-catch.

Inconsistent State Management
The mIsStarted flag is only set when the first animator starts and cleared when the last one ends. If animators have different durations or if some fail, this could lead to inconsistent state.

Code Quality Issues

Redundant Code in build()

java   // Each animator sets these properties individually
   scaleXAnimator.setRepeatCount(repeatCount);
   scaleXAnimator.setRepeatMode(ObjectAnimator.RESTART);
   scaleXAnimator.setStartDelay(delay);

Then later:

java   for (Animator animator : mAnimators) {
       objectAnimator.setRepeatCount(repeatCount);  // Set AGAIN
       objectAnimator.setRepeatMode(ObjectAnimator.RESTART);  // Set AGAIN
   }

Fix: Remove the redundant initial settings and only set them once in the loop at the end.

Unsafe Casting

java   ObjectAnimator objectAnimator = (ObjectAnimator) mAnimators.get(x);
While you control creation, explicit casting without checks is risky. Consider using a typed list: List<ObjectAnimator>.

Magic Number

java   mAnimators = new ArrayList<>(3 * mCount);
The 3 should be a named constant (e.g., ANIMATORS_PER_PULSE = 3).

Inconsistent Formatting

java if(shouldStartBeforeSettingCurrentTime){ // Missing space after 'if'
Unnecessary Variable

java for (int x = 0; x < mAnimators.size(); x++)
Use a foreach loop or rename x to something meaningful like i or index.
Design Concerns

Listener Pattern is Fragile
Attaching listeners only to the first and last animators assumes:

They'll always start/end in order
No animator will fail
All animators have the same duration

Consider using an AtomicInteger to track running animators instead.
Empty Listener Methods
The AnimatorSimpleListener class has empty implementations. Consider using AnimatorListenerAdapter from the Android framework instead of creating your own.

📝 Suggested Improvements

java// Use Android's built-in adapter
private final AnimatorListenerAdapter mAnimatorStartListener = new AnimatorListenerAdapter() {
    @Override
    public void onAnimationStart(Animator animator) {
        mIsStarted = true;
    }
};

// Better constant definition
private static final int ANIMATORS_PER_PULSE = 3; // ScaleX, ScaleY, Alpha

// Type-safe list
private List<ObjectAnimator> mAnimators;

// Remove redundant property settings in build()
ObjectAnimator scaleXAnimator = ObjectAnimator.ofFloat(pulseView, "ScaleX", 0f, 1f);
scaleXAnimator.setStartDelay(delay);
mAnimators.add(scaleXAnimator);
// Don't set repeat count/mode/interpolator/duration here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants