980152 - Panning on grid panes can be painful with large images

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

I was writing this little weather add-on that added weather panes in Firefox Hub. It pulls down random "weather" images from jpg.to, some of which I assume can be huge. Scrolling in the pane can be extremely slow sometimes, but because the images are random it can be a bit hard to detect...

I'm guessing that extremely large images cause issues. I wonder if we need to resize them in Picasso?

Addon:
people.mozilla.com/~wjohnston/weather.xpi

Comment 1

[Lucas Rocha (:lucasr)]

(In reply to Wesley Johnston (:wesj) from comment #0)
> I was writing this little weather add-on that added weather panes in Firefox
> Hub. It pulls down random "weather" images from jpg.to, some of which I
> assume can be huge. Scrolling in the pane can be extremely slow sometimes,
> but because the images are random it can be a bit hard to detect...
> 
> I'm guessing that extremely large images cause issues. I wonder if we need
> to resize them in Picasso?
> 
> Addon:
> people.mozilla.com/~wjohnston/weather.xpi

What do you mean by 'slow' here? The images take too long to download? Scrolling is not smooth?

Picasso has a fit() option:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/thirdparty/com/squareup/picasso/RequestCreator.java#109

Could you patch a local build to see if it helps?

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch

This forces Picasso to resize these images. cropCenter makes it maintain the aspect ratio. The images won't have a size initially, so we force a minimum. I picked 160dp because that seemed like a reasonable halfwidth for the screen. There may be better options though. i.e. maybe we have a dimen that's good for this?

Comment 3

[Lucas Rocha (:lucasr)]

Comment on attachment 8387458 [details] [diff] [review]
Patch

Review of attachment 8387458 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/home/PanelItemView.java
@@ +72,5 @@
>          if (hasImageUrl) {
>              Picasso.with(getContext())
>                     .load(imageUrl)
> +                   .resize(Math.max(mImage.getWidth(), minSize),
> +                           Math.max(mImage.getHeight(), minSize))

You can use the panel_grid_view_column_width dimen but that should only apply to ImageItemViews when used in a PanelGridView. Maybe you could add APIs to PanelItemView that allows the caller to set the target image size or something. If target image size is not explicitly defined, fallback to mImage's width/height (Picasso's fit() behaviour?).

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

This does what you suggested. I had to modify the Adapater to pass the width/height through.

Comment 5

[Lucas Rocha (:lucasr)]

Comment on attachment 8390844 [details] [diff] [review]
Patch

Review of attachment 8390844 [details] [diff] [review]:
-----------------------------------------------------------------

Nice, just want to see an updated version before giving r+.

::: mobile/android/base/home/PanelGridView.java
@@ +35,5 @@
>  
>          mViewConfig = viewConfig;
>          mUrlHandler = new PanelViewUrlHandler(viewConfig);
>  
>          mAdapter = new PanelViewAdapter(context, viewConfig.getItemType());

Technically, PanelGridView can contain ARTICLE panel views too. You should only apply the target size when mViewConfig.getItemType() is IMAGE.

::: mobile/android/base/home/PanelItemView.java
@@ +26,5 @@
>      private final TextView mDescription;
>      private final ImageView mImage;
>      private final LinearLayout mTitleDescContainer;
> +    private int targetWidth;
> +    private int targetHeight;

You should keep the coding style consistent with the rest of the file. Only use the new coding style for new files. 

Initialize with 0 for extra clarity.

@@ +38,5 @@
>          mImage = (ImageView) findViewById(R.id.image);
>          mTitleDescContainer = (LinearLayout) findViewById(R.id.title_desc_container);
>      }
>  
> +    // Allows setting the size that images are resized to explicitly

nit: Use javadoc style comment.

@@ +76,5 @@
> +            RequestCreator picasso = Picasso.with(getContext())
> +                                            .load(imageUrl);
> +
> +            if (targetWidth == 0 || targetHeight == 0) {
> +                picasso.fit();

Is this necessary? Make sure fit() doesn't cause any visible delay to display the images. I'd avoid using it if possible.

::: mobile/android/base/home/PanelViewAdapter.java
@@ +14,5 @@
>  import android.view.ViewGroup;
>  
>  class PanelViewAdapter extends CursorAdapter {
>  	private final Context mContext;
>  	private final ItemType mItemType;

Replace these tabs with spaces while at it?

@@ +39,3 @@
>      @Override
>      public View newView(Context context, Cursor cursor, ViewGroup parent) {
> +        PanelItemView item = PanelItemView.create(mContext, mItemType);

final

@@ +40,5 @@
>      public View newView(Context context, Cursor cursor, ViewGroup parent) {
> +        PanelItemView item = PanelItemView.create(mContext, mItemType);
> +        if (targetWidth > 0 && targetHeight > 0) {
> +            item.setTargetImageSize(targetWidth, targetHeight);
> +        }

nit: add empty line here.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch 1/2 - Cleanup

This is just moving things to the new style/fixing the tabs-spaces err.

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch 2/2

And this is the picasso fix. I removed the .fit() call. I only added it because you suggested it before, but since the consumers don't really know the image size we want, I don't think its fair to force them into an aspect ratio...

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8391353 [details] [diff] [review]
Patch 2/2

Review of attachment 8391353 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great but still needs to fix the issue in PanelGridView.

::: mobile/android/base/home/PanelGridView.java
@@ +39,5 @@
>          adapter = new PanelViewAdapter(context, viewConfig.getItemType());
> +        Resources res = getResources();
> +        int size = res.getDimensionPixelSize(R.dimen.panel_grid_view_column_width);
> +        // Gridview images are square
> +        adapter.setTargetImageSize(size, size);

It seems you forgot to cover my previous review comment here:

Technically, PanelGridView can contain ARTICLE panel views too. You should only apply the target size when mViewConfig.getItemType() is IMAGE.

::: mobile/android/base/home/PanelItemView.java
@@ +26,5 @@
>      private final TextView description;
>      private final ImageView image;
>      private final LinearLayout titleDescContainer;
> +    private int targetWidth;
> +    private int targetHeight;

nit: init with 0 for extra clarity.

::: mobile/android/base/home/PanelViewAdapter.java
@@ +16,5 @@
>  class PanelViewAdapter extends CursorAdapter {
>      private final Context context;
>      private final ItemType itemType;
> +    private int targetWidth;
> +    private int targetHeight;

nit: Init with 0 for extra clarity.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8391349 [details] [diff] [review]
Patch 1/2 - Cleanup

Review of attachment 8391349 [details] [diff] [review]:
-----------------------------------------------------------------

Slightly mixed about this change as it makes these classes inconsistent with the rest of the home package. But I'll survive.

Comment 10

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/e4b7b3164899
https://hg.mozilla.org/integration/fx-team/rev/e777fa3d7fa9

Comment 11

[Wesley Johnston (:wesj)]

grr. didn't unbitrot this right from the folders stuff. backed out part 2:
https://hg.mozilla.org/integration/fx-team/rev/b3f3c802372a

will ask for re-review since this has changed a bunch.

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch

This seems right for me. All that's changed is the folder view support.

Comment 13

[Wesley Johnston (:wesj)]

Attachment: Patch

Comment 14

[Lucas Rocha (:lucasr)]

Comment on attachment 8396658 [details] [diff] [review]
Patch

Review of attachment 8396658 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with the suggested changes.

::: mobile/android/base/home/PanelGridView.java
@@ +41,5 @@
>          adapter = new PanelViewAdapter(context, viewConfig);
> +        Resources res = getResources();
> +        int size = res.getDimensionPixelSize(R.dimen.panel_grid_view_column_width);
> +        // Gridview images are square
> +        adapter.setTargetImageSize(size, size);

This should be:

if (viewConfig.getItemType() == ItemType.IMAGE) {
    final Resources res = getResources();
    final int size = res.getDimensionPixelSize(R.dimen.panel_grid_view_column_width);

    // Gridview images are square. This will ensure the images are resized to the
    // expected size in the grid.
    adapter.setTargetImageSize(size);
}

::: mobile/android/base/home/PanelViewAdapter.java
@@ +77,4 @@
>          }
> +
> +        PanelItemView item = PanelItemView.create(context, viewConfig.getItemType());
> +        if (viewConfig.getItemType() == ItemType.IMAGE && targetWidth > 0 && targetHeight > 0) {

This itemType check should be in PanelGridView, not here in the adapter.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e4b7b3164899