Closed Bug 832561 Opened 11 years ago Closed 11 years ago

Don't update about:home top sites immediately when pins are added/cleared

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(relnote-firefox -)

VERIFIED FIXED
Firefox 21
Tracking Status
relnote-firefox --- -

People

(Reporter: wesj, Assigned: wesj)

Details

Attachments

(3 files, 1 obsolete file)

When you pin or unpin a site on about:home, we update the entire set of top sites. It feels confusing to the user. My current proposal is to just:

1.) When a site is unpinned, just replace it with the blank (+) icon and no title. Tapping on that row should bring up the "Edit" dialog box like a normal blank row.

2.) When a site is pinned, just pin it. If a site is pinned through the Edit dialog, pin it and switch any other top sites that reference the url to the (+) icon.
Attached patch Patch (obsolete) — Splinter Review
I think this feels a lot better and makes more sense to me when I use it. I removed all of our update calls to the DB due to pin/unpin actions, to ensure that whatever state we're in, its not "randomly" shifting under the user. Since all click/long tap menus are based on the view you touched, they're independent of what's in the database. (i.e. the cursor and views aren't always going to be in sync anymore. At this point that is by ian's design. We don't want to fill in with frecency every time a site is pinned/unpinned.)

I also moved some things (showing/hiding the url and title of the pinned site) into the view holder in order to avoid some mistakes I had seen. Its close to being its own view at this point, and I'd like to do that soon.
Assignee: nobody → wjohnston
Attachment #704161 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 704161 [details] [diff] [review]
Patch

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

Look good but I'd like to see a new cleaned up patch before giving r+.

::: mobile/android/base/AboutHomeContent.java
@@ +189,5 @@
>                  MenuInflater inflater = mActivity.getMenuInflater();
>                  inflater.inflate(R.menu.abouthome_topsites_contextmenu, menu);
>  
>                  // If nothing is pinned at all, hide both clear items
> +                View view = mTopSitesGrid.getChildAt(info.position);

There's an implicit assumption here that child count is same as adapter count, which is not usually true for GridView. Our case is different because we hacked this GridView to always make all items visible. I think this deserves a comment.

@@ +849,5 @@
>              thumbnailView = (ImageView) v.findViewById(R.id.thumbnail);
>              pinnedView = (ImageView) v.findViewById(R.id.pinned);
>          }
>  
> +        public void setTitle(String title) {

Maybe add an early return here if title is the same?

@@ +851,5 @@
>          }
>  
> +        public void setTitle(String title) {
> +            mTitle = title;
> +            updateTitle();

updateTitleView() is probably a better name for this method.

@@ +858,5 @@
> +        public String getTitle() {
> +            return TextUtils.isEmpty(mTitle) ? mTitle : mUrl;
> +        }
> +
> +        public void setUrl(String url) {

Maybe add an early return here if url is the same?

@@ +876,5 @@
> +                titleView.setText(mUrl);
> +                titleView.setVisibility(View.VISIBLE);
> +            } else {
> +                titleView.setVisibility(View.INVISIBLE);
> +            }

I'd simplify this method a bit:

String title = (!TextUtils.isEmpty(mTitle) ? mTitle : mUrl);
if (!TextUtils.isEmpty(title) {
    titleView.setText(title);
    titleView.setVisibility(View.VISIBLE);
} else {
    titleView.setVisibility(View.INVISIBLE);
}

@@ +902,5 @@
>              pinnedView.setBackgroundDrawable(aPinned ? getPinDrawable() : null);
>          }
> +
> +        public boolean isPinned() {
> +            return !(pinnedView.getBackground() == null);

This looks a bit hacky. It's not entirely obvious that having a background means a different pinned state. Maybe add a private boolean property to hold this state for clarity?

@@ +936,5 @@
>                  viewHolder = (TopSitesViewHolder) convertView.getTag();
>              }
>  
> +            viewHolder.setTitle(title);
> +            viewHolder.setUrl(url);

Small overhead from potentially setting the titleView text twice here. No big deal I guess.

@@ +999,5 @@
>                  return null;
>              }
>  
>              @Override
> +            public void onPostExecute(Void v) { }

No need to leave this empty method here. Remove it.

@@ +1022,2 @@
>              @Override
> +            public void onPostExecute(Void v) { }

No need to leave this empty method here. Remove it.

@@ +1083,4 @@
>                      @Override
> +                    public void onPostExecute(Bitmap b) {
> +                        if (b != null) {
> +                            displayThumbnail(v, b);

No need to set empty placeholder if b is null here I guess?
Attachment #704161 - Flags: review?(lucasr.at.mozilla) → review-
Attached patch Patch v2 1/3Splinter Review
In order to not implement onPostExecute, it needs to be stubbed in GeckoAsyncTask.
Attachment #704161 - Attachment is obsolete: true
Attachment #705466 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch v2 2/3Splinter Review
The old patch, with nits addressed.
Attachment #705467 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch v2 3/3Splinter Review
A quick fix to not show the "Pin Site" option on empty slots.
Attachment #705466 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 705467 [details] [diff] [review]
Patch v2 2/3

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

Nice.
Attachment #705467 - Flags: review?(lucasr.at.mozilla) → review+
(In reply to Wesley Johnston (:wesj) from comment #5)
> Created attachment 705468 [details] [diff] [review]
> Patch v2 3/3
> 
> A quick fix to not show the "Pin Site" option on empty slots.

Is this ready for review?
Comment on attachment 705468 [details] [diff] [review]
Patch v2 3/3

Sure. I reorganized this again in bug 833942, but that bug is less likely to be uplifted to Aurora.
Attachment #705468 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 705468 [details] [diff] [review]
Patch v2 3/3

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

Ok.
Attachment #705468 - Flags: review?(lucasr.at.mozilla) → review+
Verified fixed on Nightly 21.0a1 (2013-01-28)
Status: RESOLVED → VERIFIED
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

Created:
Updated:
Size: