Bug 1254797 - Intersperse blank sites between pinned sites if needed r=rnewman
This fixes an edge case that is most likely to happen to new users if they pin a site followed
by removing one or more suggested sites. This results in the topsites table containing
less sites than needed, leading to some pinned sites being displayed in a higher
than expected position. This also broke unpinning since our code assumes
that a topsites physical position corresponds to its DB position (which prior
to this patch was not the case).
MozReview-Commit-ID: JgTUa55eXnz
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserProvider.java
@@ -782,21 +782,56 @@ public class BrowserProvider extends Sha
suggestedSiteArgs = DBUtils.appendSelectionArgs(suggestedSiteArgs,
new String[] {
suggestedSitesCursor.getString(idColumnIndex),
suggestedSitesCursor.getString(urlColumnIndex),
suggestedSitesCursor.getString(titleColumnIndex)
});
}
+ boolean hasPreparedBlankTiles = false;
+
+ // We can somewhat reduce the number of blanks we produce by eliminating suggested sites.
+ // We do the actual limit calculation in SQL (since we need to take into account the number
+ // of pinned sites too), but this might avoid producing 5 or so additional blank tiles
+ // that would then need to be filtered out.
+ final int maxBlanksNeeded = suggestedGridLimit - suggestedSitesCursor.getCount();
+
+ final StringBuilder blanksBuilder = new StringBuilder();
+ for (int i = 0; i < maxBlanksNeeded; i++) {
+ if (hasPreparedBlankTiles) {
+ blanksBuilder.append(" UNION ALL");
+ } else {
+ hasPreparedBlankTiles = true;
+ }
+
+ blanksBuilder.append(" SELECT" +
+ " -1 AS " + Bookmarks._ID + "," +
+ " '' AS " + Bookmarks.URL + "," +
+ " '' AS " + Bookmarks.TITLE);
+ }
+
+
+
// To restrict suggested sites to the grid, we simply subtract the number of topsites (which have already had
// the pinned sites filtered out), and the number of pinned sites.
// SQLite completely ignores negative limits, hence we need to manually limit to 0 in this case.
final String suggestedLimitClause = " LIMIT MAX(0, (" + suggestedGridLimit + " - (SELECT COUNT(*) FROM " + TABLE_TOPSITES + ") - (SELECT COUNT(*) " + pinnedSitesFromClause + "))) ";
+ // Pinned site positions are zero indexed, but we need to get the maximum 1-indexed position.
+ // Hence to correctly calculate the largest pinned position (which should be 0 if there are
+ // no sites, or 1-6 if we have at least one pinned site), we coalesce the DB position (0-5)
+ // with -1 to represent no-sites, which allows us to directly add 1 to obtain the expected value
+ // regardless of whether a position was actually retrieved.
+ final String blanksLimitClause = " LIMIT MAX(0, " +
+ "COALESCE((SELECT " + Bookmarks.POSITION + " " + pinnedSitesFromClause + "), -1) + 1" +
+ " - (SELECT COUNT(*) " + pinnedSitesFromClause + ")" +
+ " - (SELECT COUNT(*) FROM " + TABLE_TOPSITES + ")" +
+ ")";
+
db.beginTransaction();
try {
db.execSQL("DROP TABLE IF EXISTS " + TABLE_TOPSITES);
db.execSQL("CREATE TEMP TABLE " + TABLE_TOPSITES + " AS" +
" SELECT " +
Bookmarks._ID + ", " +
Combined.BOOKMARK_ID + ", " +
@@ -830,16 +865,33 @@ public class BrowserProvider extends Sha
Bookmarks.URL + " NOT IN (SELECT url FROM " + TABLE_TOPSITES + ")" +
" AND " +
Bookmarks.URL + " NOT IN (SELECT url " + pinnedSitesFromClause + ")" +
suggestedLimitClause + " )",
suggestedSiteArgs);
}
+ if (hasPreparedBlankTiles) {
+ db.execSQL("INSERT INTO " + TABLE_TOPSITES +
+ // We need to LIMIT _after_ selecting the relevant suggested sites, which requires us to
+ // use an additional internal subquery, since we cannot LIMIT a subquery that is part of UNION ALL.
+ // Hence the weird SELECT * FROM (SELECT ...relevant suggested sites... LIMIT ?)
+ " SELECT * FROM (SELECT " +
+ Bookmarks._ID + ", " +
+ Bookmarks._ID + " AS " + Combined.BOOKMARK_ID + ", " +
+ " -1 AS " + Combined.HISTORY_ID + ", " +
+ Bookmarks.URL + ", " +
+ Bookmarks.TITLE + ", " +
+ "NULL AS " + Combined.HISTORY_ID + ", " +
+ TopSites.TYPE_BLANK + " as " + TopSites.TYPE +
+ " FROM ( " + blanksBuilder.toString() + " )" +
+ blanksLimitClause + " )");
+ }
+
// If we retrieve more topsites than we have free positions for in the freeIdSubquery,
// we will have topsites that don't receive a position when joining TABLE_TOPSITES
// with freeIdSubquery. Hence we need to coalesce the position with a generated position.
// We know that the difference in positions will be at most suggestedGridLimit, hence we
// can add that to the rowid to generate a safe position.
// I.e. if we have 6 pinned sites then positions 0..5 are filled, the JOIN results in
// the first N rows having positions 6..(N+6), so row N+1 should receive a position that is at
// least N+1+6, which is equal to rowid + 6.