Skip to content

refactor: simplify switch and improve locking in ClusterRendererMultipleItems #1557

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

refactor: simplify switch and improve locking in ClusterRendererMultipleItems #1557

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 67 additions & 59 deletions ...y/src/main/java/com/google/maps/android/clustering/view/ClusterRendererMultipleItems.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,31 +107,19 @@ public enum AnimationType {
}

public void setAnimationType(AnimationType type) {
switch (type) {
case EASE_IN, ACCELERATE:
animationInterp = new AccelerateInterpolator();
break;
case EASE_OUT:
animationInterp = new DecelerateInterpolator();
break;
case EASE_IN_OUT:
animationInterp = new AccelerateDecelerateInterpolator();
break;
case FAST_OUT_SLOW_IN:
animationInterp = new FastOutSlowInInterpolator();
break;
case BOUNCE:
animationInterp = new BounceInterpolator();
break;
case DECELERATE:
animationInterp = new DecelerateInterpolator();
break;
default:
animationInterp = new LinearInterpolator();
break;
}
animationInterp = switch (type) {
case LINEAR -> new LinearInterpolator();
case EASE_IN, ACCELERATE -> new AccelerateInterpolator();
case EASE_OUT, DECELERATE -> new DecelerateInterpolator();
case EASE_IN_OUT -> new AccelerateDecelerateInterpolator();
case FAST_OUT_SLOW_IN -> new FastOutSlowInInterpolator();
case BOUNCE -> new BounceInterpolator();
};
}

public void setAnimationInterpolator(TimeInterpolator interpolator) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Being this a public method, worth considering adding a line of documentation to explain that it sets the Interpolator for the current animation.

animationInterp = interpolator;
}

/**
* Markers that are currently on the map.
Expand Down Expand Up @@ -459,6 +447,7 @@ public void setMapZoom(float zoom) {
}

@SuppressLint("NewApi")
@Override
public void run() {
final MarkerModifier markerModifier = new MarkerModifier();
final float zoom = mMapZoom;
Expand Down Expand Up @@ -687,13 +676,16 @@ private MarkerModifier() {
*/
public void add(boolean priority, CreateMarkerTask c) {
lock.lock();
sendEmptyMessage(BLANK);
if (priority) {
mOnScreenCreateMarkerTasks.add(c);
} else {
mCreateMarkerTasks.add(c);
try {
sendEmptyMessage(BLANK);
if (priority) {
mOnScreenCreateMarkerTasks.add(c);
} else {
mCreateMarkerTasks.add(c);
}
} finally {
lock.unlock();
}
lock.unlock();
}

/**
Expand All @@ -704,13 +696,16 @@ public void add(boolean priority, CreateMarkerTask c) {
*/
public void remove(boolean priority, Marker m) {
lock.lock();
sendEmptyMessage(BLANK);
if (priority) {
mOnScreenRemoveMarkerTasks.add(m);
} else {
mRemoveMarkerTasks.add(m);
try {
sendEmptyMessage(BLANK);
if (priority) {
mOnScreenRemoveMarkerTasks.add(m);
} else {
mRemoveMarkerTasks.add(m);
}
} finally {
lock.unlock();
}
lock.unlock();
}

/**
Expand All @@ -722,18 +717,21 @@ public void remove(boolean priority, Marker m) {
*/
public void animate(MarkerWithPosition marker, LatLng from, LatLng to) {
lock.lock();
AnimationTask task = new AnimationTask(marker, from, to, lock);
try {
AnimationTask task = new AnimationTask(marker, from, to, lock);

for (AnimationTask existingTask : ongoingAnimations) {
if (existingTask.marker.getId().equals(task.marker.getId())) {
existingTask.cancel();
break;
for (AnimationTask existingTask : ongoingAnimations) {
if (existingTask.marker.getId().equals(task.marker.getId())) {
existingTask.cancel();
break;
}
}
}

mAnimationTasks.add(task);
ongoingAnimations.add(task);
lock.unlock();
mAnimationTasks.add(task);
ongoingAnimations.add(task);
} finally {
lock.unlock();
}
}

/**
Expand All @@ -746,18 +744,21 @@ public void animate(MarkerWithPosition marker, LatLng from, LatLng to) {
*/
public void animateThenRemove(MarkerWithPosition marker, LatLng from, LatLng to) {
lock.lock();
AnimationTask animationTask = new AnimationTask(marker, from, to, lock);
for (AnimationTask existingTask : ongoingAnimations) {
if (existingTask.marker.getId().equals(animationTask.marker.getId())) {
existingTask.cancel();
break;
try {
AnimationTask animationTask = new AnimationTask(marker, from, to, lock);
for (AnimationTask existingTask : ongoingAnimations) {
if (existingTask.marker.getId().equals(animationTask.marker.getId())) {
existingTask.cancel();
break;
}
}
}

ongoingAnimations.add(animationTask);
animationTask.removeOnAnimationComplete(mClusterManager.getMarkerManager());
mAnimationTasks.add(animationTask);
lock.unlock();
ongoingAnimations.add(animationTask);
animationTask.removeOnAnimationComplete(mClusterManager.getMarkerManager());
mAnimationTasks.add(animationTask);
} finally {
lock.unlock();
}
}

@Override
Expand Down Expand Up @@ -821,9 +822,13 @@ private void removeMarker(Marker m) {
* @return true if there is still work to be processed.
*/
public boolean isBusy() {
lock.lock();
try {
lock.lock();
return !(mCreateMarkerTasks.isEmpty() && mOnScreenCreateMarkerTasks.isEmpty() && mOnScreenRemoveMarkerTasks.isEmpty() && mRemoveMarkerTasks.isEmpty() && mAnimationTasks.isEmpty());
return !(mCreateMarkerTasks.isEmpty()
&& mOnScreenCreateMarkerTasks.isEmpty()
&& mOnScreenRemoveMarkerTasks.isEmpty()
&& mRemoveMarkerTasks.isEmpty()
&& mAnimationTasks.isEmpty());
} finally {
lock.unlock();
}
Expand Down Expand Up @@ -1257,11 +1262,11 @@ public void cancel() {
new Handler(Looper.getMainLooper()).post(this::cancel);
return;
}
lock.lock();
try {
markerWithPosition.position = to;
mRemoveOnComplete = false;
valueAnimator.cancel();
lock.lock();
ongoingAnimations.remove(this);
} finally {
lock.unlock();
Expand All @@ -1279,8 +1284,11 @@ public void onAnimationEnd(Animator animation) {

// Remove the task from the queue
lock.lock();
ongoingAnimations.remove(this);
lock.unlock();
try {
ongoingAnimations.remove(this);
} finally {
lock.unlock();
}
}

public void removeOnAnimationComplete(MarkerManager markerManager) {
Expand Down
Loading