Closed Bug 860759 Opened 7 years ago Closed 7 years ago

Don't treat pinned sites as bookmarks unless they are bookmarks

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23
Tracking Status
firefox21 --- affected
firefox22 --- verified
firefox23 --- verified
fennec 21+ ---

People

(Reporter: aaronmt, Assigned: Margaret)

References

Details

Attachments

(1 file, 1 obsolete file)

Why are pinned tabs using the bookmark star in the Top-Sites tab? This gives off the impression that it's bookmarked. When checking the bookmark tab, one wont see the item.

STR

i) http://mozilla.com
ii) Pin it as a top-site on about:home
iii) Visit the Top-Sites tab; see it starred

--
Nightly (04/11)
LG Nexus 4 (Android 4.2.2)
Summary: Denote pinned bookmarks differently in Top-Sites tab; don't use bookmark star → Denote pinned sites differently in Top-Sites tab; don't use bookmark star
tracking-fennec: --- → ?
Pinned sites are being treated as bookmarks, even if they aren't. The check needs to be "pinned aware"
Assignee: nobody → wjohnston
tracking-fennec: ? → 21+
Summary: Denote pinned sites differently in Top-Sites tab; don't use bookmark star → Don't treat pinned sites as bookmarks unless they are bookmarks
The relevant code is here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/awesomebar/AllPagesTab.java#749

It looks like we may need to add the Bookmarks.PARENT column to the cursor, or we should update our filter() method to handle this:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#223

getTopSites() already does something to deal with pinned sites, I'm wondering if we can move that logic into filterAllSites.

Note, we already account for this is our other isBookmark() check, which handles things like updating the star in the menu:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/LocalBrowserDB.java#475
Assignee: wjohnston → margaret.leibovic
Attached patch patch (obsolete) — Splinter Review
This does the job, but I'm not sure if there's a more elegant way to go about it. I'm also not sure if we're actually expecting to have this pinned bookmark data in any consumers of this view.

These combined view migrations are pretty gross to review. To help, here's the diff with createCombinedViewOn13:

                                         Bookmarks.FIXED_READING_LIST_ID + " THEN " + Combined.DISPLAY_READER + " ELSE " +
                                         Combined.DISPLAY_NORMAL + " END AS " + Combined.DISPLAY + ", " +
                                      "-1 AS " + Combined.HISTORY_ID + ", " +
                                      "-1 AS " + Combined.VISITS + ", " +
                                      "-1 AS " + Combined.DATE_LAST_VISITED + ", " +
                                      qualifyColumn(TABLE_BOOKMARKS, Bookmarks.FAVICON_ID) + " AS " + Combined.FAVICON_ID +
                         " FROM " + TABLE_BOOKMARKS +
                         " WHERE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TYPE)  + " = " + Bookmarks.TYPE_BOOKMARK + " AND " +
+                                    // Ignore pinned bookmarks.
+                                    qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT)  + " <> " + Bookmarks.FIXED_PINNED_LIST_ID + " AND " +
                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED)  + " = 0 AND " +
                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks.URL) +
                                         " NOT IN (SELECT " + History.URL + " FROM " + TABLE_HISTORY + ")" +
                         " UNION ALL" +
                         // History with and without bookmark.
                         " SELECT " + "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " WHEN 0 THEN " +
-                                     qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) +  " ELSE NULL END AS " + Combined.BOOKMARK_ID + ", " +
+                                        // Give pinned bookmarks a NULL ID so that they're not treated as bookmarks. We can't
+                                        // completely ignore them here because they're joined with history entries we care about.
+                                        "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.PARENT) + " WHEN " +
+                                        Bookmarks.FIXED_PINNED_LIST_ID + " THEN NULL ELSE " +
+                                        qualifyColumn(TABLE_BOOKMARKS, Bookmarks._ID) + " END " +
+                                     "ELSE NULL END AS " + Combined.BOOKMARK_ID + ", " +
                                      qualifyColumn(TABLE_HISTORY, History.URL) + " AS " + Combined.URL + ", " +
                                      // Prioritize bookmark titles over history titles, since the user may have
                                      // customized the title for a bookmark.
                                      "COALESCE(" + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.TITLE) + ", " +
                                                    qualifyColumn(TABLE_HISTORY, History.TITLE) +")" + " AS " + Combined.TITLE + ", " +
                                      // Only use DISPLAY_READER if the matching bookmark entry inside reading
                                      // list folder is not marked as deleted.
                                      "CASE " + qualifyColumn(TABLE_BOOKMARKS, Bookmarks.IS_DELETED) + " WHEN 0 THEN CASE " +
Attachment #737033 - Flags: feedback?(wjohnston)
Attachment #737033 - Flags: feedback?(bnicholson)
Once the new Home page design lands, we won't need this fix. Top Sites will only bookmarked sites.
(In reply to Mark Finkle (:mfinkle) from comment #4)
> Once the new Home page design lands, we won't need this fix. Top Sites will
> only bookmarked sites.

I hope that's your way of saying we should just WONTFIX this :)
In case we decide to go with this patch: does it handle the case where an item is both pinned and a bookmark?
I think we should be able to do this in LocalBrowserDB like we do in getTopSites. i.e. filterAllSites already does some modifying to selection and selectionArgs if the filter is non-null. Can we just add:

selection = DBUtils.concatenateWhere("", Combined.URL + " NOT IN (SELECT "+ Bookmarks.URL + " FROM bookmarks WHERE bookmarks." + Bookmarks.PARENT + " == ?)");

String[] selectionArgs = DBUtils.appendSelectionArgs(new String[0], new String[] { String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) });
Orthogonal, but I wonder if we can make update createCombinedViewOn15 to something more like:

void createCombinedViewForVersion(SQLiteDatabase db, int version) {
  StringBuilder createStatement = new StringBuilder("CREATE TABLE " ....);

  if (version == 15) createStatement.append("DO SOME 15 stuff");
  else createStatement.append("DO SOME other stuff");

  db.executeSQL(createStatement.toString());
}
(In reply to Brian Nicholson (:bnicholson) from comment #6)
> In case we decide to go with this patch: does it handle the case where an
> item is both pinned and a bookmark?

Yes, in that case there are two different bookmarks, so we keep the data associated with the regular bookmark, but not with the pinned bookmark. I just tested to verify.

(In reply to Wesley Johnston (:wesj) from comment #7)
> I think we should be able to do this in LocalBrowserDB like we do in
> getTopSites. i.e. filterAllSites already does some modifying to selection
> and selectionArgs if the filter is non-null. Can we just add:
> 
> selection = DBUtils.concatenateWhere("", Combined.URL + " NOT IN (SELECT "+
> Bookmarks.URL + " FROM bookmarks WHERE bookmarks." + Bookmarks.PARENT + " ==
> ?)");
> 
> String[] selectionArgs = DBUtils.appendSelectionArgs(new String[0], new
> String[] { String.valueOf(Bookmarks.FIXED_PINNED_LIST_ID) });

I believe that will make us filter out the entry altogether, when what we really want to do is just make sure that it's not marked as being a bookmark. We do want to show these history entries, we just want the star to not show up.
Blocks: 858994
No longer blocks: 858994
Attached patch patchSplinter Review
Updated to apply on top of bug 858994.

We should decide how much we care about this problem. If we we're willing to live with it until the new about:home redesign lands, we can WONTFIX this bug, but otherwise, this is the way to fix it.
Attachment #737033 - Attachment is obsolete: true
Attachment #737033 - Flags: feedback?(wjohnston)
Attachment #737033 - Flags: feedback?(bnicholson)
Attachment #741044 - Flags: review?(bnicholson)
Comment on attachment 741044 [details] [diff] [review]
patch

LGTM
Attachment #741044 - Flags: review?(bnicholson) → review+
https://hg.mozilla.org/mozilla-central/rev/b0268b65bc46
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Depends on: 858994
Comment on attachment 741044 [details] [diff] [review]
patch

Requesting uplift per Margaret's note in Bug 858994 comment 29.
Attachment #741044 - Flags: approval-mozilla-aurora?
Depends on: 866150
Attachment #741044 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer depends on: 866150
Attention auto-uplifters, I want to see how bug 866150 plays out before uplifting this to aurora, so please don't uplift this for me.
Verified fixed on:
Build: Firefox for Android 22 Beta 1 (2013-05-15)
Device: LG Nexus 4 
OS:Android 4.2.2
Verified fixed on:
Build: Firefox for Android 23.0a2 (2013-05-16)
Device: Samsung Galaxy Tab
OS:Android 4.0.4
You need to log in before you can comment on or make changes to this bug.