Closed Bug 721032 Opened 8 years ago Closed 8 years ago

Make thumbnails in about:home and tab menu identical

Categories

(Firefox for Android :: General, defect)

11 Branch
x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 12
Tracking Status
firefox11 --- verified
firefox12 --- verified
firefox13 --- verified
blocking-fennec1.0 --- beta+
fennec 11+ ---

People

(Reporter: ibarlow, Assigned: blassey)

References

Details

(Whiteboard: tabs-ux [inbound])

Attachments

(5 files, 2 obsolete files)

No description provided.
Specs and assets to follow shortly
Whiteboard: tabs-ux
this is a dupe of bug 718410 (or the other way around)
Attached image mockup
The intent with thumbnails in Firefox Mobile has always been to use one consistent size and style throughout the UI. It looks like we've veered off in a couple of directions, so right now they look a little different on the start page compared to the tab menu. 

Attached is a mockup of how thumbs should look (ie, the same everywhere)

The next comment will include detailed specs describing size, image placement and cropping
Attached image specs
specs for mdpi, hdpi, xhdpi
covers all device densities
tracking-fennec: --- → ?
Duplicate of this bug: 718410
(In reply to Ian Barlow (:ibarlow) from comment #4)
> Created attachment 591527 [details]
> specs
> 
> specs for mdpi, hdpi, xhdpi

Ian, what we really need is how wide and tall you want these in dip (android terminology). Right now the tab tray thumbnails are 138x78dip and the about:home ones are 120x80. I'm kinda fond of the 120x80 for 2 reasons, one its a nice easy ration to deal with. Second, its the size and ratio that the stock browser stores in the system DB (which was more important when we were using the system DB).
(In reply to Brad Lassey [:blassey] from comment #7)
> (In reply to Ian Barlow (:ibarlow) from comment #4)
> > specs for mdpi, hdpi, xhdpi
> 
> Ian, what we really need is how wide and tall you want these in dip (android
> terminology).

1 dip = 1 pixel in MDPI, so you can use treat the MDPI dimensions from the spec as dips.
Attached patch WIP patch (obsolete) — Splinter Review
this patch just starts making the thumbnails at the same size (120x80dip). It does not change the layout at all, so the thumbnails are not wide enough for the existing tab menu layout
Attached patch patch (obsolete) — Splinter Review
this patch uses 136x77dip for the thumbnail dimensions in both about:home and the tab menu
Assignee: nobody → blassey.bugs
Attachment #591655 - Attachment is obsolete: true
Attachment #591659 - Flags: review?(mark.finkle)
Comment on attachment 591659 [details] [diff] [review]
patch

>diff -r 438c497043c3 -r aa31aa4623ab mobile/android/base/Tab.java

> public final class Tab {

>+    private static int kMinScreenshotWidth = 0;
>+    private static int kMinScreenshotHeight = 0;

Let's not use "k" prefix here. I thought "k" was reserved for constants

>+    float getMinDim() {
>+        if (sMinDim == 0) {
>+            DisplayMetrics metrics = new DisplayMetrics();
>+            GeckoApp.mAppContext.getWindowManager().getDefaultDisplay().getMetrics(metrics);
>+            sMinDim = Math.min(metrics.widthPixels / kThumbnailWidth, metrics.heightPixels / kThumbnailHeight);
>+            sDensity = metrics.density;
>+        }
>+        return sMinDim;
>+    }
>+
>+    int getMinScreenshotWidth() {
>+        if (kMinScreenshotWidth != 0)
>+            return kMinScreenshotWidth;
>+        return kMinScreenshotWidth = (int)(getMinDim() * kThumbnailWidth);
>+    }
>+
>+    int getMinScreenshotHeight() {
>+        if (kMinScreenshotHeight != 0)
>+            return kMinScreenshotHeight;
>+        return kMinScreenshotHeight = (int)(getMinDim() * kThumbnailHeight);
>+    }
>+


>+    int getThumbnailWidth() {
>+        return (int)(kThumbnailWidth * sDensity);
>+    }
>+
>+    int getThumbnailHeight() {
>+        return (int)(kThumbnailHeight * sDensity);
>+    }

These are a little dangerous since they requires getMinDim() to be called first. I know you'll grind your teeth at this, but can we add a getDentsity() method and use it?

>     public void updateThumbnail(final Bitmap b) {

>+                getMinDim();

I am not a fan of calling this function to "set" sMinDim and then using sMinDim everywhere else. I think we should cal getMinDim() everywhere you are currently using sMinDim. If someone removes the getMinDim() line here thinking it is not used, it will wreck the rest of the function. You already have a short circuit in getMinDim() to make it return fast. Let's use it.

>                 if (b != null) {
>                     try {
>-                        Bitmap cropped = Bitmap.createBitmap(b, 0, 0, sMinDim * 3, sMinDim * 2);
>-                        Bitmap bitmap = Bitmap.createScaledBitmap(cropped, (int) (kThumbnailWidth * sDensity), (int) (kThumbnailHeight * sDensity), false);
>+                        Bitmap cropped = null;
>+                        if (getMinScreenshotWidth() < b.getWidth() && getMinScreenshotHeight() < b.getHeight())

Since I am not the smartest guy you'll meet, a comment here about what the various if blocks are doing would be nice. Looks like you are dealing with different aspect ratios.

>+                            cropped = Bitmap.createBitmap(b, 0, 0, getMinScreenshotWidth(), getMinScreenshotHeight());
>+                        else if (b.getWidth() * kMinScreenshotHeight == b.getHeight() * kMinScreenshotWidth)

Same issue with kMinScreenshotHeight and kMinScreenshotWidth. We have getters for those. Let's keep using them.

r- for your thoughts on the getters
Attachment #591659 - Flags: review?(mark.finkle) → review-
Attached patch patchSplinter Review
Attachment #591659 - Attachment is obsolete: true
Attachment #591700 - Flags: review?(mark.finkle)
Comment on attachment 591700 [details] [diff] [review]
patch


>diff -r 438c497043c3 -r 8ef11d3eb030 mobile/android/base/Tab.java

>     public void updateThumbnail(final Bitmap b) {

>+                getMinDim();
>                 if (b != null) {

You can remove this getMinDim() call, I think

>+                         * smaller dimention, then crop the larger dimention.

dimension
Attachment #591700 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/0e7c2ed8e866
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Comment on attachment 591700 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): 
User impact if declined: 
UI is inconsistent. Also we have to spend twice the time and memory on an essentially duplicate thumbnails
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky):
pretty safe
Attachment #591700 - Flags: approval-mozilla-aurora?
Comment on attachment 591700 [details] [diff] [review]
patch

[Triage Comment]
Mobile only - approved for Aurora.
Attachment #591700 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attached image thumbnails
The thumbnails now look the same in tab menu and in about:home, but some have that black right and bottom edges which is bug 721841 and for some the page is not cropped to its full width (see screenshot) for which I don't know if there is a bug filed or, it should be fixed by this bug?
Verified fixed on:

Firefox 13.0a1 (2012-02-24)
20120224031039
http://hg.mozilla.org/mozilla-central/rev/cd120efbe4c6

Firefox 12.0a2 (2012-02-24)
20120224042008
http://hg.mozilla.org/releases/mozilla-aurora/rev/643e4dd65350

Firefox 11.0 (2012-02-24)
20120223235138
http://hg.mozilla.org/releases/mozilla-beta/rev/0e30f13e9012

--
Device: Motorola Droid 2
OS: Android 2.3.3
Status: RESOLVED → VERIFIED
blocking-fennec1.0: --- → beta+
You need to log in before you can comment on or make changes to this bug.