From cc90d1b7671e437f1d31a4e73eb3687deeb093ae Mon Sep 17 00:00:00 2001 From: Pinyao Ting Date: Mon, 21 Nov 2022 15:50:30 -0800 Subject: [PATCH] Make grid size migration less confusing for users. Grid size migration is a confusing experience for users because the content of the grid is not predicatable to the user. Part of the reason was the dedupe logic which merges items from new grid into old grid, this mean if user removes an item, change to another grid and go back, user will see that same item re-appears. This CL keeps the content of target grid the same as source gird, i.e user will will get the exact same set of app icons/shortcuts/folders and widgets (if size permits) across grid sizes. The only difference being their placement in the grid. Bug: 256859723 Test: atest GridSizeMigrationUtilTest Change-Id: I1732c91e441ad44bc43e0a943566a83563d12b07 --- .../model/GridSizeMigrationUtil.java | 104 ++++---- .../model/GridSizeMigrationUtilTest.kt | 224 +++++++++++++++++- .../launcher3/util/LauncherModelHelper.java | 6 + 3 files changed, 290 insertions(+), 44 deletions(-) diff --git a/src/com/android/launcher3/model/GridSizeMigrationUtil.java b/src/com/android/launcher3/model/GridSizeMigrationUtil.java index d63408b9f1..eded5ea6a9 100644 --- a/src/com/android/launcher3/model/GridSizeMigrationUtil.java +++ b/src/com/android/launcher3/model/GridSizeMigrationUtil.java @@ -66,7 +66,7 @@ import java.util.stream.Collectors; public class GridSizeMigrationUtil { private static final String TAG = "GridSizeMigrationUtil"; - private static final boolean DEBUG = false; + private static final boolean DEBUG = true; private GridSizeMigrationUtil() { // Util class should not be instantiated @@ -188,27 +188,54 @@ public class GridSizeMigrationUtil { @NonNull final DeviceGridState srcDeviceState, @NonNull final DeviceGridState destDeviceState) { - final List hotseatItems = destReader.loadHotseatEntries(); - final List workspaceItems = destReader.loadAllWorkspaceEntries(); - final List hotseatDiff = - calcDiff(srcReader.loadHotseatEntries(), hotseatItems); - final List workspaceDiff = - calcDiff(srcReader.loadAllWorkspaceEntries(), workspaceItems); + final List srcHotseatItems = srcReader.loadHotseatEntries(); + final List srcWorkspaceItems = srcReader.loadAllWorkspaceEntries(); + final List dstHotseatItems = destReader.loadHotseatEntries(); + final List dstWorkspaceItems = destReader.loadAllWorkspaceEntries(); + final List hotseatToBeAdded = new ArrayList<>(1); + final List workspaceToBeAdded = new ArrayList<>(1); + final IntArray toBeRemoved = new IntArray(); + + calcDiff(srcHotseatItems, dstHotseatItems, hotseatToBeAdded, toBeRemoved); + calcDiff(srcWorkspaceItems, dstWorkspaceItems, workspaceToBeAdded, toBeRemoved); final int trgX = targetSize.x; final int trgY = targetSize.y; - if (hotseatDiff.isEmpty() && workspaceDiff.isEmpty()) { + if (DEBUG) { + Log.d(TAG, "Start migration:" + + "\n Source Device:" + + srcWorkspaceItems.stream().map(DbEntry::toString).collect( + Collectors.joining(",\n", "[", "]")) + + "\n Target Device:" + + dstWorkspaceItems.stream().map(DbEntry::toString).collect( + Collectors.joining(",\n", "[", "]")) + + "\n Removing Items:" + + dstWorkspaceItems.stream().filter(entry -> + toBeRemoved.contains(entry.id)).map(DbEntry::toString).collect( + Collectors.joining(",\n", "[", "]")) + + "\n Adding Workspace Items:" + + workspaceToBeAdded.stream().map(DbEntry::toString).collect( + Collectors.joining(",\n", "[", "]")) + + "\n Adding Hotseat Items:" + + hotseatToBeAdded.stream().map(DbEntry::toString).collect( + Collectors.joining(",\n", "[", "]")) + ); + } + if (!toBeRemoved.isEmpty()) { + removeEntryFromDb(destReader.mDb, destReader.mTableName, toBeRemoved); + } + if (hotseatToBeAdded.isEmpty() && workspaceToBeAdded.isEmpty()) { return false; } // Sort the items by the reading order. - Collections.sort(hotseatDiff); - Collections.sort(workspaceDiff); + Collections.sort(hotseatToBeAdded); + Collections.sort(workspaceToBeAdded); // Migrate hotseat solveHotseatPlacement(db, srcReader, - destReader, context, destHotseatSize, hotseatItems, hotseatDiff); + destReader, context, destHotseatSize, dstHotseatItems, hotseatToBeAdded); // Migrate workspace. // First we create a collection of the screens @@ -229,8 +256,8 @@ public class GridSizeMigrationUtil { Log.d(TAG, "Migrating " + screenId); } solveGridPlacement(db, srcReader, - destReader, context, screenId, trgX, trgY, workspaceDiff, false); - if (workspaceDiff.isEmpty()) { + destReader, context, screenId, trgX, trgY, workspaceToBeAdded, false); + if (workspaceToBeAdded.isEmpty()) { break; } } @@ -238,42 +265,37 @@ public class GridSizeMigrationUtil { // In case the new grid is smaller, there might be some leftover items that don't fit on // any of the screens, in this case we add them to new screens until all of them are placed. int screenId = destReader.mLastScreenId + 1; - while (!workspaceDiff.isEmpty()) { + while (!workspaceToBeAdded.isEmpty()) { solveGridPlacement(db, srcReader, - destReader, context, screenId, trgX, trgY, workspaceDiff, preservePages); + destReader, context, screenId, trgX, trgY, workspaceToBeAdded, preservePages); screenId++; } return true; } - /** Return what's in the src but not in the dest */ - private static List calcDiff(List src, List dest) { - Map destIdSet = new HashMap<>(); - for (DbEntry entry : dest) { - String entryID = entry.getEntryMigrationId(); - if (destIdSet.containsKey(entryID)) { - destIdSet.put(entryID, destIdSet.get(entryID) + 1); - } else { - destIdSet.put(entryID, 1); + /** + * Calculate the differences between {@code src} (denoted by A) and {@code dest} + * (denoted by B). + * All DbEntry in A - B will be added to {@code toBeAdded} + * All DbEntry.id in B - A will be added to {@code toBeRemoved} + */ + private static void calcDiff(@NonNull final List src, + @NonNull final List dest, @NonNull final List toBeAdded, + @NonNull final IntArray toBeRemoved) { + src.forEach(entry -> { + if (!dest.contains(entry)) { + toBeAdded.add(entry); } - } - List diff = new ArrayList<>(); - for (DbEntry entry : src) { - String entryID = entry.getEntryMigrationId(); - if (destIdSet.containsKey(entryID)) { - Integer count = destIdSet.get(entryID); - if (count <= 0) { - diff.add(entry); - destIdSet.remove(entryID); - } else { - destIdSet.put(entryID, count - 1); + }); + dest.forEach(entry -> { + if (!src.contains(entry)) { + toBeRemoved.add(entry.id); + if (entry.itemType == LauncherSettings.Favorites.ITEM_TYPE_FOLDER) { + entry.mFolderItems.values().forEach(ids -> ids.forEach(toBeRemoved::add)); } - } else { - diff.add(entry); } - } - return diff; + }); } private static void insertEntryInDb(SQLiteDatabase db, Context context, DbEntry entry, @@ -682,12 +704,12 @@ public class GridSizeMigrationUtil { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; DbEntry entry = (DbEntry) o; - return Objects.equals(mIntent, entry.mIntent); + return Objects.equals(getEntryMigrationId(), entry.getEntryMigrationId()); } @Override public int hashCode() { - return Objects.hash(mIntent); + return Objects.hash(getEntryMigrationId()); } public void updateContentValues(ContentValues values) { diff --git a/tests/src/com/android/launcher3/model/GridSizeMigrationUtilTest.kt b/tests/src/com/android/launcher3/model/GridSizeMigrationUtilTest.kt index 85d7bf9d70..76a186bd83 100644 --- a/tests/src/com/android/launcher3/model/GridSizeMigrationUtilTest.kt +++ b/tests/src/com/android/launcher3/model/GridSizeMigrationUtilTest.kt @@ -17,6 +17,7 @@ package com.android.launcher3.model import android.content.Context import android.content.Intent +import android.database.Cursor; import android.database.sqlite.SQLiteDatabase import android.graphics.Point import android.os.Process @@ -183,15 +184,232 @@ class GridSizeMigrationUtilTest { // Expected dest grid icons // _ _ _ _ // 5 6 7 8 - // 9 _ 10_ + // 9 _ _ _ // _ _ _ _ - assertThat(locMap.size.toLong()).isEqualTo(6) + assertThat(locMap.size.toLong()).isEqualTo(5) assertThat(locMap[testPackage5]).isEqualTo(Point(0, 1)) assertThat(locMap[testPackage6]).isEqualTo(Point(1, 1)) assertThat(locMap[testPackage7]).isEqualTo(Point(2, 1)) assertThat(locMap[testPackage8]).isEqualTo(Point(3, 1)) assertThat(locMap[testPackage9]).isEqualTo(Point(0, 2)) - assertThat(locMap[testPackage10]).isEqualTo(Point(2, 2)) + } + + /** + * Old migration logic, should be modified once [FeatureFlags.ENABLE_NEW_MIGRATION_LOGIC] is + * not needed anymore + */ + @Test + @Throws(Exception::class) + fun testMigrationBackAndForth() { + // Hotseat items in grid A + // 1 2 _ 3 4 + modelHelper.addItem(APP_ICON, 0, HOTSEAT, 0, 0, testPackage1, 1, TMP_CONTENT_URI) + modelHelper.addItem(SHORTCUT, 1, HOTSEAT, 0, 0, testPackage2, 2, TMP_CONTENT_URI) + modelHelper.addItem(SHORTCUT, 3, HOTSEAT, 0, 0, testPackage3, 3, TMP_CONTENT_URI) + modelHelper.addItem(APP_ICON, 4, HOTSEAT, 0, 0, testPackage4, 4, TMP_CONTENT_URI) + // Workspace items in grid A + // _ _ _ _ _ + // _ _ _ _ 5 + // _ _ 6 _ 7 + // _ _ 8 _ _ + // _ _ _ _ _ + modelHelper.addItem(APP_ICON, 0, DESKTOP, 4, 1, testPackage5, 5, TMP_CONTENT_URI) + modelHelper.addItem(APP_ICON, 0, DESKTOP, 2, 2, testPackage6, 6, TMP_CONTENT_URI) + modelHelper.addItem(APP_ICON, 0, DESKTOP, 4, 2, testPackage7, 7, TMP_CONTENT_URI) + modelHelper.addItem(APP_ICON, 0, DESKTOP, 2, 3, testPackage8, 8, TMP_CONTENT_URI) + + // Hotseat items in grid B + // 2 _ _ _ + modelHelper.addItem(SHORTCUT, 0, HOTSEAT, 0, 0, testPackage2) + // Workspace items in grid B + // _ _ _ _ + // _ _ _ 10 + // _ _ _ _ + // _ _ _ _ + modelHelper.addItem(APP_ICON, 0, DESKTOP, 1, 3, testPackage10) + + idp.numDatabaseHotseatIcons = 4 + idp.numColumns = 4 + idp.numRows = 4 + val readerGridA = DbReader(db, TMP_TABLE, context, validPackages) + val readerGridB = DbReader(db, TABLE_NAME, context, validPackages) + // migrate from A -> B + GridSizeMigrationUtil.migrate( + context, + db, + readerGridA, + readerGridB, + idp.numDatabaseHotseatIcons, + Point(idp.numColumns, idp.numRows), + DeviceGridState(context), + DeviceGridState(idp) + ) + + // Check hotseat items in grid B + var c = context.contentResolver.query( + CONTENT_URI, + arrayOf(SCREEN, INTENT), + "container=$CONTAINER_HOTSEAT", + null, + SCREEN, + null + ) ?: throw IllegalStateException() + // Expected hotseat items in grid B + // 2 1 3 4 + verifyHotseat(c, idp, + mutableListOf(testPackage2, testPackage1, testPackage3, testPackage4).toList()) + + // Check workspace items in grid B + c = context.contentResolver.query( + CONTENT_URI, + arrayOf(SCREEN, CELLX, CELLY, INTENT), + "container=$CONTAINER_DESKTOP", + null, + null, + null + ) ?: throw IllegalStateException() + var locMap = parseLocMap(context, c) + // Expected items in grid B + // _ _ _ _ + // 5 6 7 8 + // _ _ _ _ + // _ _ _ _ + assertThat(locMap.size.toLong()).isEqualTo(4) + assertThat(locMap[testPackage5]).isEqualTo(Triple(0, 0, 1)) + assertThat(locMap[testPackage6]).isEqualTo(Triple(0, 1, 1)) + assertThat(locMap[testPackage7]).isEqualTo(Triple(0, 2, 1)) + assertThat(locMap[testPackage8]).isEqualTo(Triple(0, 3, 1)) + + // add item in B + modelHelper.addItem(APP_ICON, 0, DESKTOP, 0, 2, testPackage9) + + // migrate from B -> A + GridSizeMigrationUtil.migrate( + context, + db, + readerGridB, + readerGridA, + 5, + Point(5, 5), + DeviceGridState(idp), + DeviceGridState(context) + ) + // Check hotseat items in grid A + c = context.contentResolver.query( + TMP_CONTENT_URI, + arrayOf(SCREEN, INTENT), + "container=$CONTAINER_HOTSEAT", + null, + SCREEN, + null + ) ?: throw IllegalStateException() + // Expected hotseat items in grid A + // 1 2 _ 3 4 + verifyHotseat(c, idp, mutableListOf( + testPackage1, testPackage2, null, testPackage3, testPackage4).toList()) + + // Check workspace items in grid A + c = context.contentResolver.query( + TMP_CONTENT_URI, + arrayOf(SCREEN, CELLX, CELLY, INTENT), + "container=$CONTAINER_DESKTOP", + null, + null, + null + ) ?: throw IllegalStateException() + locMap = parseLocMap(context, c) + // Expected workspace items in grid A + // _ _ _ _ _ + // _ _ _ _ 5 + // 9 _ 6 _ 7 + // _ _ 8 _ _ + // _ _ _ _ _ + assertThat(locMap.size.toLong()).isEqualTo(5) + // Verify items that existed in grid A remains in same position + assertThat(locMap[testPackage5]).isEqualTo(Triple(0, 4, 1)) + assertThat(locMap[testPackage6]).isEqualTo(Triple(0, 2, 2)) + assertThat(locMap[testPackage7]).isEqualTo(Triple(0, 4, 2)) + assertThat(locMap[testPackage8]).isEqualTo(Triple(0, 2, 3)) + // Verify items that didn't exist in grid A are added in new screen + assertThat(locMap[testPackage9]).isEqualTo(Triple(0, 0, 2)) + + // remove item from B + modelHelper.deleteItem(7, TMP_TABLE) + + // migrate from A -> B + GridSizeMigrationUtil.migrate( + context, + db, + readerGridA, + readerGridB, + idp.numDatabaseHotseatIcons, + Point(idp.numColumns, idp.numRows), + DeviceGridState(context), + DeviceGridState(idp) + ) + + // Check hotseat items in grid B + c = context.contentResolver.query( + CONTENT_URI, + arrayOf(SCREEN, INTENT), + "container=$CONTAINER_HOTSEAT", + null, + SCREEN, + null + ) ?: throw IllegalStateException() + // Expected hotseat items in grid B + // 2 1 3 4 + verifyHotseat(c, idp, + mutableListOf(testPackage2, testPackage1, testPackage3, testPackage4).toList()) + + // Check workspace items in grid B + c = context.contentResolver.query( + CONTENT_URI, + arrayOf(SCREEN, CELLX, CELLY, INTENT), + "container=$CONTAINER_DESKTOP", + null, + null, + null + ) ?: throw IllegalStateException() + locMap = parseLocMap(context, c) + // Expected workspace items in grid B + // _ _ _ _ + // 5 6 _ 8 + // 9 _ _ _ + // _ _ _ _ + assertThat(locMap.size.toLong()).isEqualTo(4) + assertThat(locMap[testPackage5]).isEqualTo(Triple(0, 0, 1)) + assertThat(locMap[testPackage6]).isEqualTo(Triple(0, 1, 1)) + assertThat(locMap[testPackage8]).isEqualTo(Triple(0, 3, 1)) + assertThat(locMap[testPackage9]).isEqualTo(Triple(0, 0, 2)) + } + + private fun verifyHotseat(c: Cursor, idp: InvariantDeviceProfile, expected: List) { + assertThat(c.count).isEqualTo(idp.numDatabaseHotseatIcons) + val screenIndex = c.getColumnIndex(SCREEN) + val intentIndex = c.getColumnIndex(INTENT) + expected.forEachIndexed { idx, pkg -> + if (pkg == null) return@forEachIndexed + c.moveToNext() + assertThat(c.getInt(screenIndex).toLong()).isEqualTo(idx) + assertThat(c.getString(intentIndex)).contains(pkg) + } + c.close() + } + + private fun parseLocMap(context: Context, c: Cursor): Map> { + // Check workspace items + val intentIndex = c.getColumnIndex(INTENT) + val screenIndex = c.getColumnIndex(SCREEN) + val cellXIndex = c.getColumnIndex(CELLX) + val cellYIndex = c.getColumnIndex(CELLY) + val locMap = mutableMapOf>() + while (c.moveToNext()) { + locMap[Intent.parseUri(c.getString(intentIndex), 0).getPackage()] = + Triple(c.getInt(screenIndex), c.getInt(cellXIndex), c.getInt(cellYIndex)) + } + c.close() + return locMap.toMap() } @Test diff --git a/tests/src/com/android/launcher3/util/LauncherModelHelper.java b/tests/src/com/android/launcher3/util/LauncherModelHelper.java index e7e551fd0d..93bf31256d 100644 --- a/tests/src/com/android/launcher3/util/LauncherModelHelper.java +++ b/tests/src/com/android/launcher3/util/LauncherModelHelper.java @@ -362,6 +362,12 @@ public class LauncherModelHelper { sandboxContext.getContentResolver().insert(contentUri, values); } + public void deleteItem(int itemId, @NonNull final String tableName) { + final Uri uri = Uri.parse("content://" + + LauncherProvider.AUTHORITY + "/" + tableName + "/" + itemId); + sandboxContext.getContentResolver().delete(uri, null, null); + } + public int[][][] createGrid(int[][][] typeArray) { return createGrid(typeArray, 1); }