Closed Bug 1193374 Opened 9 years ago Closed 9 years ago

Remove tab panel preview dimension dependency from TopSites

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox43 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

Attachments

(1 file, 1 obsolete file)

MozReview Request: Bug 1193374 - Remove tab panel preview dimension dependency from TopSites; r?mcomella
40 bytes, text/x-review-board-request
mcomella
: review+
Details 
Currently the tabs panel previews are dependent on the top sites thumbnail aspect ratio.  In order to prep for upcoming work to the tabs tray, where we will need to support different aspect ratios for tabs panel and top sites, we should refactor this code to ensure that both previews will work independently of each other.
Here's an APK to test: https://dl.dropboxusercontent.com/u/7163922/Work/1193374.apk

Of note the preview sizes of the tabs panel previews are now different than the top sites - may be worth clearing data before running if you've installed one of my apks before.

antlam: can you check out and let me know if you spot any issues with top site previews or tabs panel previews, also comments on sizes of tabs in the tabs panel would be good.
Flags: needinfo?(alam)
Blocks: 1193745
In this patch I allow the top site and tab tray previews to use different aspect ratios.

In Thumbnail helper I get the biggest aspect ratio to use for creating the thumbnail.  I then create a new type of ImageView which will draw a bitmap in to the view starting from the top left and cropping any excess without scaling - this means that given any two aspect ratios, both images will mostly always show correctly.  The only time this looks a bit funky is if the website isn't as long as required by the thumbnail width and aspect ratio, then the preview will fit vertically and the right hand side will be cut off - this is acceptable from my POV.
Attachment #8647506 - Flags: feedback?(michael.l.comella)
Flags: needinfo?(alam)
As discussed, measurements aren't correct but this is looking good!
Comment on attachment 8647506 [details] [diff] [review]
Remove tab panel preview dimension dependency from TopSites

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

Martyn, I'll get to this tomorrow.
Comment on attachment 8647506 [details] [diff] [review]
Remove tab panel preview dimension dependency from TopSites

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

Makes sense.

::: mobile/android/base/ThumbnailHelper.java
@@ +60,5 @@
>  
>      private ThumbnailHelper() {
>          final Resources res = GeckoAppShell.getContext().getResources();
>  
> +        mThumbnailAspectRatio = Math.max(TABS_PANEL_THUMBNAIL_ASPECT_RATIO, TOP_SITES_THUMBNAIL_ASPECT_RATIO);

Why do we use the max here? Add a comment.

::: mobile/android/base/tabqueue/TabQueueService.java
@@ +133,5 @@
>                  tabQueueHandler.post(new Runnable() {
>                      @Override
>                      public void run() {
>                          // If there is a runnable around, that means that the previous process hasn't yet completed, so
> +                        // we will need to prevent it from running and remove the view from the winScaleAndCropImageViewdow manager.

hee hee winScaleAndCropImageViewdow? :)

Something tells me you didn't mean to change this. :P

::: mobile/android/base/tabs/TabsLayoutThumbnailView.java
@@ +46,5 @@
> +
> +        if (drawable == null) {
> +            drawable = getResources().getDrawable(R.drawable.tab_panel_tab_background);
> +            resize = false;
> +            setScaleType(ScaleType.FIT_XY);

We should probably undo scaleType. If this view gets reused, e.g.:

setImageDrawable(null);
...
setImageDrawable(someDrawable);

scaleType will be set to the wrong value, right?

However, since we change scaleType dynamically in CropImageView, this could get confusing.

::: mobile/android/base/widget/CropImageView.java
@@ +1,5 @@
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +        

nit: ws

@@ +17,5 @@
> +public class CropImageView extends ImageView {
> +    public static final String LOGTAG = "Gecko" + CropImageView.class.getSimpleName();
> +    private final RectF layoutRect = new RectF();
> +    private int width;
> +    private int height;

nit: -> viewWidth/Height

It took me a second w/ the drawableWidth/Height

@@ +46,5 @@
> +        init();
> +    }
> +
> +    protected void init() {
> +        setScaleType(ScaleType.FIT_START);

Is this ever used?

@@ +87,5 @@
> +
> +        float scale = Math.max(verticalScaleValue, horizontalScaleValue);
> +
> +        layoutNextMatrix.setScale(scale, scale);
> +        getDrawable().setBounds(0, 0, width, height);

Does the drawable really not just draw within the bounds of the ImageView? Why is that?

@@ +91,5 @@
> +        getDrawable().setBounds(0, 0, width, height);
> +
> +        setImageMatrix(layoutNextMatrix);
> +
> +        //You can't modify the matrix in place and I want to avoid allocation, so let's keep two references to two

nit: // Y

@@ +106,5 @@
> +        updateImageMatrix();
> +    }
> +
> +    private void clearLayoutVars() {
> +        layoutRect.setEmpty();

Unused?

@@ +110,5 @@
> +        layoutRect.setEmpty();
> +    }
> +
> +    @Override
> +    public void setImageResource(final int resId) {

setImageBitmap (and eventually updateImageMatrix) changes the scaleType meaning any calls here could use the wrong scaleType, right?

Same w/ setImageDrawable.
Attachment #8647506 - Flags: feedback?(michael.l.comella) → feedback+
Bug 1193374 - Remove tab panel preview dimension dependency from TopSites; r?mcomella
Attachment #8650435 - Flags: review?(michael.l.comella)
Regarding scale type concerns: This code was mostly taken from the existing codebase, just moved around.  I've been using this locally and have only seem one issue, which was that after loading a second url in a tab, the resultant thumbnail was very small (this was fixed by clearing the matrix before loading the new drawable).
Comment on attachment 8650435 [details]
MozReview Request: Bug 1193374 - Remove tab panel preview dimension dependency from TopSites; r?mcomella

https://reviewboard.mozilla.org/r/16627/#review15027

No glaring issues – just a bunch of nits! :P

::: mobile/android/base/ThumbnailHelper.java:68
(Diff revision 1)
> +        mThumbnailAspectRatio = Math.max(TABS_PANEL_THUMBNAIL_ASPECT_RATIO, TOP_SITES_THUMBNAIL_ASPECT_RATIO);

nit: Since this value is technically a constant, I'd prefer it to be `static`.

::: mobile/android/base/home/TopSitesThumbnailView.java:22
(Diff revision 1)
>   * A height constrained ImageView to show thumbnails of top and pinned sites.

CropImageView is constrained by the width so this is also constrained by the width now, right?

::: mobile/android/base/resources/layout-v11/tablet_tabs_item_cell.xml:71
(Diff revision 1)
> -                                                android:layout_height="@dimen/new_tablet_tab_thumbnail_height"
> +                                                        android:layout_height="@dimen/new_tablet_tab_thumbnail_height"/>

btw, I think you'll need to rebase.

::: mobile/android/base/resources/layout/tabs_item_cell.xml:28
(Diff revision 1)
> -        <org.mozilla.gecko.widget.ThumbnailView android:id="@+id/thumbnail"
> +        <org.mozilla.gecko.tabs.TabsLayoutThumbnailView android:id="@+id/thumbnail"

Why `TabsLayout...` and not `TabsPanel...`?

::: mobile/android/base/tabs/TabsLayoutThumbnailView.java:17
(Diff revision 1)
> +public class TabsLayoutThumbnailView extends CropImageView {

Class comment

::: mobile/android/base/tabs/TabsLayoutThumbnailView.java:37
(Diff revision 1)
> +    protected void init() {

@Override

::: mobile/android/base/widget/CropImageView.java:14
(Diff revision 1)
> +import com.nineoldandroids.view.ViewHelper;

ws above import

::: mobile/android/base/widget/CropImageView.java:15
(Diff revision 1)
> +

Class comment.

::: mobile/android/base/widget/CropImageView.java:16
(Diff revision 1)
> +public class CropImageView extends ImageView {

Is this intended to be instantiated directly? If not, perhaps this should be `abstract`.

::: mobile/android/base/widget/CropImageView.java:47
(Diff revision 1)
> +    public CropImageView(final Context context, final AttributeSet attrs, final int defStyleAttr, final int defStyleRes) {

nit: here & TabsLayoutThumbnailView

I prefer the style of calling out to the constructor with the most arguments:

public CropImageView() {
  this(new Context());
}

public CropImageView(Context context) {
  super(context);
}

I think it'd also simplify the `init` call hierarchy, which is currently a bit confusing.

::: mobile/android/base/widget/CropImageView.java:95
(Diff revision 1)
> +        float verticalScaleValue = (float) viewHeight / (float) drawableHeight;

nit: `final`s here

::: mobile/android/base/widget/CropImageView.java:109
(Diff revision 1)
> +    public void setImageBitmap(final Bitmap bm, final boolean resize) {

You don't need to override `setImageBitmap(Bitmap)` because it calls out to `setImageDrawable`, right? Add a comment. If not, override it!

::: mobile/android/base/widget/CropImageView.java:117
(Diff revision 1)
> +    public void setImageResource(final int resId) {

This does not call out to `setImageDrawable` so you should call `setImageMatrix(null)` in here too.
Attachment #8650435 - Flags: review?(michael.l.comella) → review+
url:        https://hg.mozilla.org/integration/fx-team/rev/62a3a7f33f0915012ccfa3f948390ed5d53d8bd7
changeset:  62a3a7f33f0915012ccfa3f948390ed5d53d8bd7
user:       Martyn Haigh <mhaigh@mozilla.org>
date:       Mon Aug 24 15:34:24 2015 +0100
description:
Bug 1193374 - Remove tab panel preview dimension dependency from TopSites r=mcomella
https://hg.mozilla.org/mozilla-central/rev/62a3a7f33f09
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Blocks: 1204875
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: