Skip to content

Fixes #3479: Implement Progress Bar for Zoom Activity #3481

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

Merged
merged 5 commits into from
Mar 29, 2020
Merged

Fixes #3479: Implement Progress Bar for Zoom Activity #3481

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ba41f3f
ZoomableActivity: add hierarchy to view for displaying progress bar
kbhardwaj123 Mar 10, 2020
8bf1d64
CircularProgressBar: circular progress bar for ZoomableDraweeView
kbhardwaj123 Mar 10, 2020
125f8c5
ZoomableActivity: add indeterministic loading spinner
kbhardwaj123 Mar 10, 2020
a972bb5
activity_zoomable: add inderterministic Progressbar
kbhardwaj123 Mar 10, 2020
85e00af
remove circular progressbar and make changes to controller listener
kbhardwaj123 Mar 12, 2020
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
31 changes: 31 additions & 0 deletions app/src/main/java/fr/free/nrw/commons/media/ZoomableActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,21 +6,30 @@
import butterknife.BindView;
import butterknife.ButterKnife;
import fr.free.nrw.commons.R;
import fr.free.nrw.commons.media.zoomControllers.loading.CircleProgressBarDrawable;
import fr.free.nrw.commons.media.zoomControllers.zoomable.DoubleTapGestureListener;
import fr.free.nrw.commons.media.zoomControllers.zoomable.ZoomableDraweeView;
import timber.log.Timber;

import android.graphics.Bitmap;
import android.graphics.BitmapFactory;
import android.graphics.drawable.Animatable;
import android.net.Uri;
import android.os.Bundle;
import com.facebook.drawee.backends.pipeline.Fresco;
import com.facebook.drawee.controller.BaseControllerListener;
import com.facebook.drawee.controller.ControllerListener;
import com.facebook.drawee.drawable.ScalingUtils;
import com.facebook.drawee.generic.GenericDraweeHierarchy;
import com.facebook.drawee.generic.GenericDraweeHierarchyBuilder;
import com.facebook.drawee.interfaces.DraweeController;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
import android.widget.ProgressBar;

import com.facebook.drawee.view.SimpleDraweeView;
import com.facebook.imagepipeline.image.ImageInfo;
import com.github.chrisbanes.photoview.PhotoView;

import java.io.InputStream;
Expand All @@ -32,6 +41,8 @@ public class ZoomableActivity extends AppCompatActivity {

@BindView(R.id.zoomable)
ZoomableDraweeView photo;
@BindView(R.id.zoom_progress_bar)
ProgressBar spinner;

@Override
protected void onCreate(Bundle savedInstanceState) {
Expand All @@ -48,13 +59,33 @@ protected void onCreate(Bundle savedInstanceState) {
init();
}

private final ControllerListener loadingListener = new BaseControllerListener<ImageInfo>() {
@Override
public void onSubmit(String id, Object callerContext) {
}

@Override
public void onIntermediateImageSet(String id, @Nullable ImageInfo imageInfo) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sometimes intermediate image is also slowly appears. I think we should also use the progressbar for that phase. What do you think @kbhardwaj123 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neslihanturan actually i inspected this thing and found that if i place a log statement for onIntermediateImageSet it won't appear in the logs, i think it would be better if i make the Indeterminate spinner INVISIBLE as soon as Intermediate image is set

}
@Override
public void onFinalImageSet(String id, @Nullable ImageInfo imageInfo, @Nullable Animatable animatable) {
spinner.setVisibility(View.GONE);
}
};
private void init() {
if( imageUri != null ) {
GenericDraweeHierarchy hierarchy = GenericDraweeHierarchyBuilder.newInstance(getResources())
.setActualImageScaleType(ScalingUtils.ScaleType.FIT_CENTER)
.setProgressBarImage(new CircleProgressBarDrawable())
.setProgressBarImageScaleType(ScalingUtils.ScaleType.FIT_CENTER)
.build();
photo.setHierarchy(hierarchy);
photo.setAllowTouchInterceptionWhileZoomed(true);
photo.setIsLongpressEnabled(false);
photo.setTapListener(new DoubleTapGestureListener(photo));
DraweeController controller = Fresco.newDraweeControllerBuilder()
.setUri(imageUri)
.setControllerListener(loadingListener)
.build();
photo.setController(controller);
}
Expand Down
44 changes: 44 additions & 0 deletions ...ain/java/fr/free/nrw/commons/media/zoomControllers/loading/CircleProgressBarDrawable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package fr.free.nrw.commons.media.zoomControllers.loading;

import android.graphics.Canvas;
import android.graphics.Paint;
import android.graphics.Rect;
import android.graphics.RectF;

import com.facebook.drawee.drawable.ProgressBarDrawable;

public class CircleProgressBarDrawable extends ProgressBarDrawable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why we are using a custom ProgressBar drawable, is this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

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

The Fresco API demands it and the drawable they use in their sample is maybe the one used here, seeing as it has the same error, link

I'd prefer to not use it and just have the indeterminate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@neslihanturan i just realised that SimpleDraweeView in the Contributions home page while uploading shows linear progress bar so i think it would be more appropriate to keep linear one for UI consistency, i will change it

@macgills i have added the progressbar because if the image size is heavy then it might take some time to load as @neslihanturan elucidated in the issue thread, i will replace circular progressbar with linear one while keeping the indeterminate spinner

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems really confusing as a user that my image is loading twice. I don't mind if the bar is linear or circular but there should only be one loading indication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@macgills i completely agree with you but the problem is that the

  • In-deterministic spinner would not account for the downloading of a big image file
  • Keeping only progressBar will show a show black screen for 95% of the time as you correctly pointed out previously

I think the best middle ground is to keep a linear progress bar which is much less intrusive and would fulfill the purpose of showing download progress,
What do you think about this :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah just stop setting it Gone in onIntermediateImageSet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is how it looks with the changes:
The blue bar at the bottom is the progress bar
GIF-200312_172652 1

Copy link
Contributor

Choose a reason for hiding this comment

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

That works, just frustrating this scenario doesn't have better support in fresco.

Maybe if we had a good placeholderImage it would feel less weird to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@macgills should i change anything else ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll leave final say with @neslihanturan
I still think only using the ProgressBar makes for a better visual experience

private final Paint mPaint = new Paint(Paint.ANTI_ALIAS_FLAG);
private int mLevel = 0;
private int maxLevel = 10000;

@Override
protected boolean onLevelChange(int level) {
mLevel = level;
invalidateSelf();
return true;
}

@Override
public void draw(Canvas canvas) {
if (getHideWhenZero() && mLevel == 0) {
return;
}
drawBar(canvas, maxLevel, getBackgroundColor());
drawBar(canvas, mLevel, getColor());
}

private void drawBar(Canvas canvas, int level, int color) {
Rect bounds = getBounds();
RectF rectF =
new RectF(
(float) (bounds.right * .4),
(float) (bounds.bottom * .45),
(float) (bounds.right * .6),
(float) (bounds.bottom * .55));
mPaint.setColor(color);
mPaint.setStyle(Paint.Style.STROKE);
mPaint.setStrokeWidth(6);
if (level != 0) canvas.drawArc(rectF, 0, (float) (level * 360 / maxLevel), false, mPaint);
}
}
9 changes: 9 additions & 0 deletions app/src/main/res/layout/activity_zoomable.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,14 @@
android:layout_width="match_parent"
app:actualImageScaleType="fitCenter"
/>
<ProgressBar
android:layout_width="@dimen/dimen_72"
android:layout_height="@dimen/dimen_72"
android:id="@+id/zoom_progress_bar"
app:layout_constraintBottom_toBottomOf="parent"
app:layout_constraintLeft_toLeftOf="parent"
app:layout_constraintRight_toRightOf="parent"
app:layout_constraintTop_toTopOf="parent"
/>

</androidx.constraintlayout.widget.ConstraintLayout>