Closed Bug 1383736 Opened 7 years ago Closed 7 years ago

Activity Stream: Use large icons for top sites

Categories

(Firefox for Android Graveyard :: General, enhancement, P1)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
enhancement

Tracking

(firefox57 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: sebastian, Assigned: sebastian)

References

(
URL
)

Details

(Whiteboard: [MobileAS] 1.27)

Attachments

(2 files)

Bug 1383736 - Use full size icons in top sites (and highlights).
59 bytes, text/x-review-board-request
mcomella
: review+
Details
topsites.png
298.83 KB, image/png
Details 
(See linked mocks)
Iteration: --- → 1.27
Priority: P2 → P1
Whiteboard: [MobileAS] → [MobileAS] 1.27
Sebastian, does use large icons for top sites mean:

1) resize the views to be larger

2) use PageMetadata.image_url to provide a larger image for top sites icons?

3) Something else (maybe even similar to 2?)

If 2, I did this in bug 1301718 (I hope I understood the purpose of that bug correctly).
Flags: needinfo?(s.kaspari)
Primarily I was thinking about:

1) Make the view larger so that it looks like in the mock

AND

3) Make sure we store icons at a size that makes sense for this. Somewhere in the icon code we resize the icon. This size might be too small for the new UI.


Regarding (2): We only use images in the highlights section and not in top sites afaik.
Flags: needinfo?(s.kaspari)
This patch gets us closer to the mocks.

Some things are not nice yet and I'll file follow-up bugs, e.g.:

* Our suggested tiles (Facebook, YouTube, etc.) are not really made for the new dimensions - we should update them to look nice.

* Small icons are not scaled indefinitely and centered instead. This doesn't look that good. I wonder what's the best fallback here. Maybe we should just use a generated icon instead.
Attached image topsites.png — 
That's how it looks like here.
Comment on attachment 8894564 [details]
Bug 1383736 - Use full size icons in top sites (and highlights).

https://reviewboard.mozilla.org/r/165726/#review170926

::: mobile/android/app/src/australis/res/values/dimens.xml:67
(Diff revision 1)
>           this will be downscaled to this value. If you need to use larger Favicons (Due to a UI
>           redesign sometime after this is written) you should increase this value to the largest
>           commonly-used size of favicon and, performance permitting, fetch the remainder from the
>           database. The largest available size is always stored in the database, regardless of this
>           value.-->
>      <dimen name="favicon_largest_interesting_size">32dp</dimen>

Do we want to increase this value?

::: mobile/android/app/src/main/res/layout/activity_stream_topsites_card.xml:16
(Diff revision 1)
>          android:layout_width="match_parent"
>          android:layout_height="match_parent"
>          gecko:enableRoundCorners="false"
>          tools:background="@drawable/favicon_globe"
> -        android:layout_marginTop="0dp" />
> +        android:scaleType="centerInside"
> +        android:layout_marginTop="0dp"

nit: why is explicitly specifying 0 necessary?

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightItem.java:101
(Diff revision 1)
>          });
>  
>          ViewUtil.enableTouchRipple(menuButton);
>      }
>  
> -    public void bind(Highlight highlight, int position, int tilesWidth, int tilesHeight) {
> +    public void bind(Highlight highlight, int position, int tilesSize) {

`tilesSize`, to me, indicates that both dimensions of the tiles will be equal to this value so I think this (and all the methods that call it) should use `tilesWidth` instead, with a comment that `tilesHeight` will be calculated dynamically.

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/HighlightItem.java:111
(Diff revision 1)
>          final String uiHighlightTitle = !TextUtils.isEmpty(backendHightlightTitle) ? backendHightlightTitle : highlight.getUrl();
>          pageTitleView.setText(uiHighlightTitle);
>  
>          ViewGroup.LayoutParams layoutParams = pageIconView.getLayoutParams();
> -        layoutParams.width = tilesWidth - tilesMargin;
> -        layoutParams.height = tilesHeight;
> +        layoutParams.width = tilesSize;
> +        layoutParams.height = (int) (tilesSize * 0.75);

nit: should we use `Math.floor`? imo, it indicates more intention than `(int)`, which could be a mistake.

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconGenerator.java:78
(Diff revision 1)
>  
>          paint.setColor(Color.WHITE);
>  
>          final String character = getRepresentativeCharacter(pageURL);
>  
> -        final float textSize = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, TEXT_SIZE_DP, context.getResources().getDisplayMetrics());
> +        final float textSize = TypedValue.applyDimension(TypedValue.COMPLEX_UNIT_DIP, widthAndHeight / 8, context.getResources().getDisplayMetrics());

Why `/ 8`? Add a comment.
Attachment #8894564 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8894564 [details]
Bug 1383736 - Use full size icons in top sites (and highlights).

https://reviewboard.mozilla.org/r/165726/#review170926

> Do we want to increase this value?

Yes, looks like we are using this in our ICO decoder!

> nit: why is explicitly specifying 0 necessary?

It's not. Removed!
Pushed by s.kaspari@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b9e3f9520cf4
Use full size icons in top sites (and highlights). r=mcomella
https://hg.mozilla.org/mozilla-central/rev/b9e3f9520cf4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Depends on: 1389710
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: