Closed Bug 787765 Opened 7 years ago Closed 7 years ago

Retheme about:home thumbnails

Categories

(Firefox for Android :: Theme and Visual Design, defect)

x86
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 19

People

(Reporter: wesj, Assigned: wesj)

References

Details

(Keywords: uiwanted)

Attachments

(8 files, 4 obsolete files)

There's a mockup in bug 775334 and comments about thumbnail changes, but they never seemed to happen? Our thumbs on the Nexus 7 and tablets feel a bit funny to me. We should probably scale them to the available size.

Patch and some screenshots coming...
Attached patch WIP Patch (obsolete) — Splinter Review
Attached image Screenshot with patch (obsolete) —
Screenshot with the patch. I think maybe on these larger screens we should default to three across rather than giving such huge click targets... ian?
Attachment #657634 - Flags: feedback?(ibarlow)
Keywords: uiwanted
Duplicate of this bug: 787932
Wes this is fantastic. I have a few comments below, but it's great to see these updates being applied. Also here is a flickr set you can use as reference for the thumbnail styling: http://www.flickr.com/photos/61892693@N03/sets/72157630730911182/detail/ -- as you can see the thumbnail layout adapts based on screen size and orientation

1. We should make sure we aren't generating thumbnails that are being stretched up in size. These look a pretty blurry.

2. Adjust thumbnail aspect ratio -- size can vary depending on screen, but ratio should be roughly 7w x 5h http://cl.ly/image/0v0R3P3I3j0r

3. Add drop shadow under thumbnails -- 2dp, no blur, black with 25% opacity

4. Enlarge page title text, bar overlay height

5. Bonus points for 2dp rounded corners :)
Oh, we should also check that our empty thumbnail box is updated as per the flickr set as well -- it should be a flat white icon, compared to the recessed looking one we used before. I believe I provided assets for this at some point, if not let me know!
(In reply to Ian Barlow (:ibarlow) from comment #4)
> 1. We should make sure we aren't generating thumbnails that are being
> stretched up in size. These look a pretty blurry.

Yeah I should have explained that. Basically we cache these thumbnails. Since this is making them bigger (and Android really really REALLY sucks at resizing images), we initially get these bad looking ones until you visit the sites and they update to the correct size/aspect ratio. We could change this to just not show the thumbnails if they're the wrong size. Let me play with trying to find a way to turn on pretty resizing. I assume you would rather show white firefox icons than badly scaled thumbs?
Drive by:
>>> int expandedHeightSpec = MeasureSpec.makeMeasureSpec(widthMeasureSpec,
Why does this use "width" to calculate the height?
Attached patch WIP Patch v2 (obsolete) — Splinter Review
Getting better. Rounded corners are not Android's friend. Still some issues with 1.) rotation and 2.) gingerbread. Some screenies at: http://people.mozilla.org/~wjohnston/home/

I think I need that white firefox logo ian. I also rolled by own shadow + white background for the label.
Attachment #657633 - Attachment is obsolete: true
Attached file White Firefox Icon
Attached the white Firefox icon for empty thumbnails.
Attached patch Patch v1 (obsolete) — Splinter Review
Looks pretty good to me. I'm sharing the same mask for all the thumbnails. I avoided using the CanvasDelegate stuff because I have a feeling that clipping using a Bitmap is faster than using a Path (but takes more memory).

Need to test on a tablet tomorrow. I'll post screenshots from two other devices first though.
Attachment #663220 - Attachment is obsolete: true
Attachment #668356 - Flags: review?(sriram)
Attached image Nexus 7
Screenshot on Nexus 7
Attachment #657634 - Attachment is obsolete: true
Attachment #657634 - Flags: feedback?(ibarlow)
Attached image Screenshot on DroidX
Attached image UI tweaks
Looks awesome, we're almost there -- attached are a few tweaks.
Hm, one of my comments was about thumbnail spacing on the phone UI, but looking at the Nexus 7 screenshot, they actually look good there. So if it's possible to adjust the thumbnail spacing on the phone without affecting 7" tablets great. Otherwise maybe we can leave that part alone.
Attached patch Patch v3Splinter Review
We can adjust phones, large, and xlarge ui separately with this.

Fixed up a bug in xlarge devices and adjusted for ians comments. Also found some annoying bugs when rotating xlarge devices that I fixed.

Tried hard to get the same or more padding on the left and right sides of the grid as there is between thumbs on a phone (right now there's 8dp padding around the thumbs which means there's 16dp between two thumbnails as well). Adding padding to the left and right of the grid view doesn't seem to fix that (maybe an issue with the onMeasure stuff, but none of my fixes seem to work).

I updated screenshots at:
http://people.mozilla.org/~wjohnston/home/
Assignee: nobody → wjohnston
Attachment #668356 - Attachment is obsolete: true
Attachment #668356 - Flags: review?(sriram)
Attachment #668559 - Flags: review?(sriram)
This uses a canvasDelegate for the mask. Its slightly simpler, but requires some casting because we need to pass some objects around.

I've tried using Paint.setShadowLayer, but since we're not drawing these objects directly, we don't have the paint used for them. Maybe there's a technique I can't find?
Comment on attachment 668671 [details] [diff] [review]
CanvasDelegate patch

This applies on top of the other patch.
Attachment #668671 - Flags: review?(sriram)
Comment on attachment 668559 [details] [diff] [review]
Patch v3

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

static private int RADIUS = 2;

Please make it 0dp by default. Also use sRadius for a static "non-final" variable. The ALL-CAPS is usually used for a final variable.

Also, values-large/ and values-xlarge/ aren't needed.
Note behaves as a LARGE device on GB, and NORMAL on ICS. So, we force the v11 qualifier with it. Could you please change that?

The second patch had the null checks for static variables avoiding re-initialization. Please import them to this patch.

r+ with these changes.
Attachment #668559 - Flags: review?(sriram) → review+
Comment on attachment 668671 [details] [diff] [review]
CanvasDelegate patch

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

I'm sorry, but, this is getting more messier than I expected. We might not need this patch :(
Your patch sans this looks better than this. :(
Attachment #668671 - Flags: review?(sriram) → review-
https://hg.mozilla.org/mozilla-central/rev/a7702d5a4834
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Blocks: 802139
No longer blocks: 802139
Attached image Screenshot
Build ID: 19.0 (2012-10-16) Nightly Channel
Device: Samsung Galaxy Nexus
OS: Android 4.1

Now About:home thumbnails are bigger as intended. Setting the bug to Verified Fixed.
Status: RESOLVED → VERIFIED
Depends on: 802115
 
-    int getThumbnailWidth() {
-        int desiredWidth = (int) (GeckoApp.mAppContext.getResources().getDimension(R.dimen.tab_thumbnail_width));
-        return desiredWidth & ~0x1;
-    }
-
-    int getThumbnailHeight() {
-        int desiredHeight = (int) (GeckoApp.mAppContext.getResources().getDimension(R.dimen.tab_thumbnail_height));
-        return desiredHeight & ~0x1;
-    }
-

This code from Tab.java specifically returned even numbers from getThumbnailWidth and getThumbnailHeight to fix bug 776906. It looks like the new code doesn't do that any more, and so may introduce a regression on some devices.
Can you file a bug for it?
Filed bug 803690 for it. Will also file a separate bug for the thumbnail corruption that bnicholson had STR for.
Depends on: 802510
Depends on: 823297
Depends on: 792410
You need to log in before you can comment on or make changes to this bug.