826075 - Add an indicator to pinned sites

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

We should show an indicator on sites on about:home that are pinned. That would help make it clearer to users why they can't "Clear" (or unpin if we change terminology) some sites but can others. We could add a tiny little icon to the left (or right) of the title by using a compound drawable.

Comment 1

[Wesley Johnston (:wesj)]

Ping ux.

Comment 2

[Ian Barlow (:ibarlow)]

Attachment: mockup

Hi Wes, I'm having some difficulty settling on a design I'm happy with, but this is something we can start with anyway -- a little transparent triangle marker in the top right corner of the thumbnail. 

Is this a thing you can draw in code, or would you need a graphic for it?

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This adds a little black triangle in the upper right corner:

http://dl.dropbox.com/u/72157/pinned.png

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 698826 [details] [diff] [review]
Patch v1

>     public void pinSite() {

>         final String url = holder.url;
>         final String title = holder.titleView.getText().toString();
>+        holder.setPinned(true);
>         // update the database on a background thread

nit: blank line before comment

otherwise looks OK to me, but I want Sriram to take a look too and think about any "overdraw" issues.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 698826 [details] [diff] [review]
Patch v1

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

Patch looks good to me. Couple of reordering of statements here and there. And few suggestions.

::: mobile/android/base/AboutHomeContent.java
@@ +35,5 @@
>  import android.graphics.BitmapFactory;
>  import android.graphics.Canvas;
>  import android.graphics.Color;
> +import android.graphics.drawable.ShapeDrawable;
> +import android.graphics.drawable.shapes.PathShape;

This doesn't seem to be used. You could remove this.

@@ +858,5 @@
> +        public TopSitesViewHolder(View v) {
> +            titleView = (TextView) v.findViewById(R.id.title);
> +            thumbnailView = (ImageView) v.findViewById(R.id.thumbnail);
> +            pinnedView = (ImageView) v.findViewById(R.id.pinned);
> +        }

Probably we could refactor the holder to a TopSitesThumbailView or something. As the constructor is doing the findViewById(), which is basically a Custom-View. That could be a followup.

@@ +861,5 @@
> +            pinnedView = (ImageView) v.findViewById(R.id.pinned);
> +        }
> +
> +        private Drawable getPinDrawable() {
> +            if (sPinDrawable == null) {

Could we initialize this in the startup path?
Win: we don't have to do a if() check every time.
Loss: one Path object will be created even there is no pinned site.

@@ +864,5 @@
> +        private Drawable getPinDrawable() {
> +            if (sPinDrawable == null) {
> +                // Draw a little triangle in the upper right corner
> +                Path path = new Path();
> +                int size = mContext.getResources().getDimensionPixelSize(R.dimen.abouthome_topsite_pinsize);

Move this line above Path instantiation, as the path related statements will be together.

@@ +871,5 @@
> +                path.lineTo(size, size);
> +                path.close();
> +
> +                sPinDrawable = new ShapeDrawable(new PathShape(path, size, size));
> +                Paint p = ((ShapeDrawable)sPinDrawable).getPaint();

Space between (ShapeDrawable) and sPinDrawable.

@@ +878,5 @@
> +            return sPinDrawable;
> +        }
> +
> +        public void setPinned(boolean aPinned) {
> +            pinnedView.setBackgroundDrawable(aPinned ? getPinDrawable() : null);

We could set it as the Image rather than the background with setImageDrawable(). If it's just the background, we could use a generic View itself. That could hold this background.

@@ +939,2 @@
>              }
> +            return buildView(url, title, pinned, convertView);

One new line before "return".

::: mobile/android/base/resources/layout/abouthome_topsite_item.xml
@@ +13,5 @@
>      <TextView android:id="@+id/title"
>                style="@style/AboutHome.Thumbnail.Label"/>
>  
> +    <ImageView android:id="@+id/pinned"
> +               style="@style/AboutHome.Thumbnail.Pinned"/>

This will cause an overdraw as we paint over something that is already there. But then, the entire thumbnail is already having a problem. We draw text over a thumbnail image -- overdraw.

The other approach would be to use a Shader to merge the path with the thumbnail-bitmap and draw it as one single Bitmap image. It's a bit complex though. If we want a win in overdraw, that's the approach to use. But for the first pass, we could land this approach here.

Also, if the text is not "over" the thumbnail, it could be better.

::: mobile/android/base/resources/values/dimens.xml
@@ +11,5 @@
>      <dimen name="abouthome_gutter_large">0dp</dimen>
>      <dimen name="abouthome_icon_crop">-14dp</dimen>
>      <dimen name="abouthome_icon_radius">2dp</dimen>
>      <dimen name="abouthome_topsite_shadow_offset">2dp</dimen>
> +    <dimen name="abouthome_topsite_pinsize">20dp</dimen>

Please preserve alphabetical order. (Aah! My OCD :( )

Comment 6

[Wesley Johnston (:wesj)]

BUilding this one more time before I land. I might not fix some of these things though and wanted to make sure it was ok?

(In reply to Sriram Ramasubramanian [:sriram] from comment #5)
> > +import android.graphics.drawable.shapes.PathShape;
> This doesn't seem to be used. You could remove this.
PathShape? It is used....

> Could we initialize this in the startup path?
> Win: we don't have to do a if() check every time.
> Loss: one Path object will be created even there is no pinned site.
I kinda want to avoid putting stuff in the startup path unless we really need it at startup. I'd like to not do this unless you've got really strong feelings about it.

> We could set it as the Image rather than the background with
> setImageDrawable(). If it's just the background, we could use a generic View
> itself. That could hold this background.
I tried this and am building to test it again (my inbound was a bit out of date so that will take a bit). I tried it the first time I wrote this though, and for some reason the drawable never showed up. I'm not sure why....

> This will cause an overdraw as we paint over something that is already
> there. But then, the entire thumbnail is already having a problem. We draw
> text over a thumbnail image -- overdraw.

I agree. I think ultimately, on these areas where we really want to minimize overdraw, we're best to just draw things in our own custom onDraw methods, where we can control exactly what is painted at any time and do our own smart bitmap caching... I think that's essentially the same as using Shaders for them.

Comment 7

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a31ee09606bf

Comment 8

[Ed Morley [:emorley]]

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

Comment 10

[Wesley Johnston (:wesj)]

Comment on attachment 698826 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 783312
User impact if declined: Hard to tell what's pinned
Testing completed (on m-c, etc.): Landed on mc early this week.
Risk to taking this patch (and alternatives if risky): Low risk. This is polish work for pinned sites stuff. Should only affect pinned views on home screen. Just adds an overlay on them.
String or UUID changes made by this patch: None.

Comment 11

[bhavana bajaj [:bajaj]]

Comment on attachment 698826 [details] [diff] [review]
Patch v1

low risk fix for a feature bug 783312 which landed in FF20 . Approving on aurora

Comment 12

[Cristian Nicolae (:xti)]

Pinned site indicator was added on the latest Nightly.

Firefox for Android
Version: 21.0a1 (2013-01-23)
Device: Galaxy R
OS: Android 2.3.4

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/82551fd7085b

Comment 14

[Cristian Nicolae (:xti)]

The pin indicator was added on Aurora too. Closing bug as verified fixed.