From 0599e880630892becf1a1991e6712e0e6d9df3c8 Mon Sep 17 00:00:00 2001 From: Andrew Lee Date: Mon, 24 Aug 2015 17:56:17 -0700 Subject: Rewrite grouping logic in Dialer. - Remove expand/collapse and item type logic in GroupingListAdapter. Losing some potential functionality, but it does not adversely affect how we currently group, and makes grouping easier to understanding. + Rewrite GroupingListAdapter to provide O(1) lookup for group size and getItem. This requires maintaining a SparseIntArray of metadata for each list item. Cut metadata storage from long to int, to help adjust for the larger memory overhead. + Simplify the logic for building and maintaing the metadata in the GroupingListAdapter, offloading much of it to the SparseIntArray. + Explictily add all groups, including groups with single items, in the CallLogBroupBuilder. + Tidied up logic in CallLogGroupBuilder to make it more intuitive what cases it's handling and what's happening. + Updated tests to work and pass with new tests. Bug: 23422274 Change-Id: Ia7a00c4b580813cade87fdc054ffdd702f59c12c --- .../android/dialer/calllog/CallLogAdapterTest.java | 53 +++++- .../dialer/calllog/CallLogGroupBuilderTest.java | 23 +-- .../dialer/calllog/GroupingListAdapterTests.java | 188 +++------------------ 3 files changed, 81 insertions(+), 183 deletions(-) (limited to 'tests/src') diff --git a/tests/src/com/android/dialer/calllog/CallLogAdapterTest.java b/tests/src/com/android/dialer/calllog/CallLogAdapterTest.java index b4162e166..2bdc1972b 100644 --- a/tests/src/com/android/dialer/calllog/CallLogAdapterTest.java +++ b/tests/src/com/android/dialer/calllog/CallLogAdapterTest.java @@ -35,7 +35,9 @@ import java.util.List; */ @SmallTest public class CallLogAdapterTest extends AndroidTestCase { - private static final String TEST_NUMBER = "12345678"; + private static final String TEST_NUMBER_1 = "12345678"; + private static final String TEST_NUMBER_2 = "87654321"; + private static final String TEST_NUMBER_3 = "18273645"; private static final String TEST_NAME = "name"; private static final String TEST_NUMBER_LABEL = "label"; private static final int TEST_NUMBER_TYPE = 1; @@ -46,7 +48,7 @@ public class CallLogAdapterTest extends AndroidTestCase { private MatrixCursor mCursor; private View mView; - private ViewHolder mViewHolder; + private CallLogListItemViewHolder mViewHolder; @Override protected void setUp() throws Exception { @@ -98,7 +100,7 @@ public class CallLogAdapterTest extends AndroidTestCase { TestContactInfoCache.Request request = mAdapter.getContactInfoCache().requests.get(0); // It is for the number we need to show. - assertEquals(TEST_NUMBER, request.number); + assertEquals(TEST_NUMBER_1, request.number); // It has the right country. assertEquals(TEST_COUNTRY_ISO, request.countryIso); // Since there is nothing in the cache, it is an immediate request. @@ -125,7 +127,7 @@ public class CallLogAdapterTest extends AndroidTestCase { public void testBindView_NoCallLogButMemoryCache_EnqueueRequest() { mCursor.addRow(createCallLogEntry()); - mAdapter.injectContactInfoForTest(TEST_NUMBER, TEST_COUNTRY_ISO, createContactInfo()); + mAdapter.injectContactInfoForTest(TEST_NUMBER_1, TEST_COUNTRY_ISO, createContactInfo()); // Bind the views of a single row. mAdapter.changeCursor(mCursor); @@ -141,7 +143,7 @@ public class CallLogAdapterTest extends AndroidTestCase { public void testBindView_BothCallLogAndMemoryCache_NoEnqueueRequest() { mCursor.addRow(createCallLogEntryWithCachedValues()); - mAdapter.injectContactInfoForTest(TEST_NUMBER, TEST_COUNTRY_ISO, createContactInfo()); + mAdapter.injectContactInfoForTest(TEST_NUMBER_1, TEST_COUNTRY_ISO, createContactInfo()); // Bind the views of a single row. mAdapter.changeCursor(mCursor); @@ -157,7 +159,7 @@ public class CallLogAdapterTest extends AndroidTestCase { // Contact info contains a different name. ContactInfo info = createContactInfo(); info.name = "new name"; - mAdapter.injectContactInfoForTest(TEST_NUMBER, TEST_COUNTRY_ISO, info); + mAdapter.injectContactInfoForTest(TEST_NUMBER_1, TEST_COUNTRY_ISO, info); // Bind the views of a single row. mAdapter.changeCursor(mCursor); @@ -171,10 +173,37 @@ public class CallLogAdapterTest extends AndroidTestCase { assertFalse("should not be immediate", request.immediate); } + public void testBindVoicemailPromoCard() { + mCursor.addRow(createCallLogEntry(TEST_NUMBER_1)); + mCursor.addRow(createCallLogEntry(TEST_NUMBER_1)); + mCursor.addRow(createCallLogEntry(TEST_NUMBER_2)); + mCursor.addRow(createCallLogEntry(TEST_NUMBER_2)); + mCursor.addRow(createCallLogEntry(TEST_NUMBER_2)); + mCursor.addRow(createCallLogEntry(TEST_NUMBER_3)); + + // Bind the voicemail promo card. + mAdapter.showVoicemailPromoCard(true); + mAdapter.changeCursor(mCursor); + mAdapter.onBindViewHolder(PromoCardViewHolder.createForTest(getContext()), 0); + + // Check that displaying the promo card does not affect the grouping or list display. + mAdapter.onBindViewHolder(mViewHolder, 1); + assertEquals(2, mAdapter.getGroupSize(1)); + assertEquals(TEST_NUMBER_1, mViewHolder.number); + + mAdapter.onBindViewHolder(mViewHolder, 2); + assertEquals(3, mAdapter.getGroupSize(2)); + assertEquals(TEST_NUMBER_2, mViewHolder.number); + + mAdapter.onBindViewHolder(mViewHolder, 3); + assertEquals(1, mAdapter.getGroupSize(3)); + assertEquals(TEST_NUMBER_3, mViewHolder.number); + } + /** Returns a contact info with default values. */ private ContactInfo createContactInfo() { ContactInfo info = new ContactInfo(); - info.number = TEST_NUMBER; + info.number = TEST_NUMBER_1; info.name = TEST_NAME; info.type = TEST_NUMBER_TYPE; info.label = TEST_NUMBER_LABEL; @@ -183,8 +212,12 @@ public class CallLogAdapterTest extends AndroidTestCase { /** Returns a call log entry without cached values. */ private Object[] createCallLogEntry() { + return createCallLogEntry(TEST_NUMBER_1); + } + + private Object[] createCallLogEntry(String testNumber) { Object[] values = CallLogQueryTestUtils.createTestValues(); - values[CallLogQuery.NUMBER] = TEST_NUMBER; + values[CallLogQuery.NUMBER] = testNumber; values[CallLogQuery.COUNTRY_ISO] = TEST_COUNTRY_ISO; return values; } @@ -212,6 +245,10 @@ public class CallLogAdapterTest extends AndroidTestCase { public TestContactInfoCache getContactInfoCache() { return (TestContactInfoCache) mContactInfoCache; } + + public void showVoicemailPromoCard(boolean show) { + mShowVoicemailPromoCard = true; + } } private static final class TestContactInfoCache extends ContactInfoCache { diff --git a/tests/src/com/android/dialer/calllog/CallLogGroupBuilderTest.java b/tests/src/com/android/dialer/calllog/CallLogGroupBuilderTest.java index 891f0686f..95558bcf6 100644 --- a/tests/src/com/android/dialer/calllog/CallLogGroupBuilderTest.java +++ b/tests/src/com/android/dialer/calllog/CallLogGroupBuilderTest.java @@ -82,7 +82,7 @@ public class CallLogGroupBuilderTest extends AndroidTestCase { addCallLogEntry(TEST_NUMBER1, Calls.INCOMING_TYPE); mBuilder.addGroups(mCursor); assertEquals(1, mFakeGroupCreator.groups.size()); - assertGroupIs(0, 3, false, mFakeGroupCreator.groups.get(0)); + assertGroupIs(0, 3, mFakeGroupCreator.groups.get(0)); } public void testAddGroups_MatchingIncomingAndOutgoing() { @@ -91,13 +91,12 @@ public class CallLogGroupBuilderTest extends AndroidTestCase { addCallLogEntry(TEST_NUMBER1, Calls.INCOMING_TYPE); mBuilder.addGroups(mCursor); assertEquals(1, mFakeGroupCreator.groups.size()); - assertGroupIs(0, 3, false, mFakeGroupCreator.groups.get(0)); + assertGroupIs(0, 3, mFakeGroupCreator.groups.get(0)); } public void testAddGroups_Voicemail() { // Does not group with other types of calls, include voicemail themselves. assertCallsAreNotGrouped(Calls.VOICEMAIL_TYPE, Calls.MISSED_TYPE); - //assertCallsAreNotGrouped(Calls.VOICEMAIL_TYPE, Calls.MISSED_TYPE, Calls.MISSED_TYPE); assertCallsAreNotGrouped(Calls.VOICEMAIL_TYPE, Calls.VOICEMAIL_TYPE); assertCallsAreNotGrouped(Calls.VOICEMAIL_TYPE, Calls.INCOMING_TYPE); assertCallsAreNotGrouped(Calls.VOICEMAIL_TYPE, Calls.OUTGOING_TYPE); @@ -150,8 +149,8 @@ public class CallLogGroupBuilderTest extends AndroidTestCase { Calls.OUTGOING_TYPE); mBuilder.addGroups(mCursor); assertEquals(2, mFakeGroupCreator.groups.size()); - assertGroupIs(1, 4, false, mFakeGroupCreator.groups.get(0)); - assertGroupIs(8, 3, false, mFakeGroupCreator.groups.get(1)); + assertGroupIs(1, 4, mFakeGroupCreator.groups.get(0)); + assertGroupIs(8, 3, mFakeGroupCreator.groups.get(1)); } public void testEqualPhoneNumbers() { @@ -228,7 +227,7 @@ public class CallLogGroupBuilderTest extends AndroidTestCase { addMultipleCallLogEntries(TEST_NUMBER1, types); mBuilder.addGroups(mCursor); assertEquals(1, mFakeGroupCreator.groups.size()); - assertGroupIs(0, types.length, false, mFakeGroupCreator.groups.get(0)); + assertGroupIs(0, types.length, mFakeGroupCreator.groups.get(0)); } @@ -266,10 +265,9 @@ public class CallLogGroupBuilderTest extends AndroidTestCase { } /** Asserts that the group matches the given values. */ - private void assertGroupIs(int cursorPosition, int size, boolean expanded, GroupSpec group) { + private void assertGroupIs(int cursorPosition, int size, GroupSpec group) { assertEquals(cursorPosition, group.cursorPosition); assertEquals(size, group.size); - assertEquals(expanded, group.expanded); } /** Defines an added group. Used by the {@link FakeGroupCreator}. */ @@ -278,13 +276,10 @@ public class CallLogGroupBuilderTest extends AndroidTestCase { public final int cursorPosition; /** The number of elements in the group. */ public final int size; - /** Whether the group should be initially expanded. */ - public final boolean expanded; - public GroupSpec(int cursorPosition, int size, boolean expanded) { + public GroupSpec(int cursorPosition, int size) { this.cursorPosition = cursorPosition; this.size = size; - this.expanded = expanded; } } @@ -294,8 +289,8 @@ public class CallLogGroupBuilderTest extends AndroidTestCase { public final List groups = newArrayList(); @Override - public void addGroup(int cursorPosition, int size, boolean expanded) { - groups.add(new GroupSpec(cursorPosition, size, expanded)); + public void addGroup(int cursorPosition, int size) { + groups.add(new GroupSpec(cursorPosition, size)); } @Override diff --git a/tests/src/com/android/dialer/calllog/GroupingListAdapterTests.java b/tests/src/com/android/dialer/calllog/GroupingListAdapterTests.java index 53583e0a7..45bc59812 100644 --- a/tests/src/com/android/dialer/calllog/GroupingListAdapterTests.java +++ b/tests/src/com/android/dialer/calllog/GroupingListAdapterTests.java @@ -16,10 +16,6 @@ package com.android.dialer.calllog; -import static com.android.dialer.calllog.GroupingListAdapter.ITEM_TYPE_GROUP_HEADER; -import static com.android.dialer.calllog.GroupingListAdapter.ITEM_TYPE_IN_GROUP; -import static com.android.dialer.calllog.GroupingListAdapter.ITEM_TYPE_STANDALONE; - import android.content.Context; import android.database.Cursor; import android.database.MatrixCursor; @@ -63,17 +59,12 @@ public class GroupingListAdapterTests extends AndroidTestCase { if (TextUtils.equals(value, currentValue)) { groupItemCount++; } else { - if (groupItemCount > 1) { - addGroup(i - groupItemCount, groupItemCount, false); - } - + addGroup(i - groupItemCount, groupItemCount); groupItemCount = 1; currentValue = value; } } - if (groupItemCount > 1) { - addGroup(count - groupItemCount, groupItemCount, false); - } + addGroup(count - groupItemCount, groupItemCount); } @Override @@ -92,7 +83,6 @@ public class GroupingListAdapterTests extends AndroidTestCase { } }; - private void buildCursor(String... numbers) { mCursor = new MatrixCursor(PROJECTION); mNextId = 1; @@ -107,170 +97,51 @@ public class GroupingListAdapterTests extends AndroidTestCase { mAdapter.changeCursor(mCursor); assertEquals(3, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_STANDALONE, false, 1); - assertPositionMetadata(2, ITEM_TYPE_STANDALONE, false, 2); + assertMetadata(0, 1, "1"); + assertMetadata(1, 1, "2"); + assertMetadata(2, 1, "3"); } - public void testGroupingWithCollapsedGroupAtTheBeginning() { + public void testGroupingWithGroupAtTheBeginning() { buildCursor("1", "1", "2"); mAdapter.changeCursor(mCursor); assertEquals(2, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_GROUP_HEADER, false, 0); - assertPositionMetadata(1, ITEM_TYPE_STANDALONE, false, 2); - } - - public void testGroupingWithExpandedGroupAtTheBeginning() { - buildCursor("1", "1", "2"); - mAdapter.changeCursor(mCursor); - mAdapter.toggleGroup(0); - - assertEquals(4, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_GROUP_HEADER, true, 0); - assertPositionMetadata(1, ITEM_TYPE_IN_GROUP, false, 0); - assertPositionMetadata(2, ITEM_TYPE_IN_GROUP, false, 1); - assertPositionMetadata(3, ITEM_TYPE_STANDALONE, false, 2); + assertMetadata(0, 2, "1"); + assertMetadata(1, 1, "2"); } - public void testGroupingWithExpandCollapseCycleAtTheBeginning() { - buildCursor("1", "1", "2"); - mAdapter.changeCursor(mCursor); - mAdapter.toggleGroup(0); - mAdapter.toggleGroup(0); - - assertEquals(2, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_GROUP_HEADER, false, 0); - assertPositionMetadata(1, ITEM_TYPE_STANDALONE, false, 2); - } - - public void testGroupingWithCollapsedGroupInTheMiddle() { + public void testGroupingWithGroupInTheMiddle() { buildCursor("1", "2", "2", "2", "3"); mAdapter.changeCursor(mCursor); assertEquals(3, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_GROUP_HEADER, false, 1); - assertPositionMetadata(2, ITEM_TYPE_STANDALONE, false, 4); + assertMetadata(0, 1, "1"); + assertMetadata(1, 3, "2"); + assertMetadata(2, 1, "3"); } - public void testGroupingWithExpandedGroupInTheMiddle() { - buildCursor("1", "2", "2", "2", "3"); - mAdapter.changeCursor(mCursor); - mAdapter.toggleGroup(1); - - assertEquals(6, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_GROUP_HEADER, true, 1); - assertPositionMetadata(2, ITEM_TYPE_IN_GROUP, false, 1); - assertPositionMetadata(3, ITEM_TYPE_IN_GROUP, false, 2); - assertPositionMetadata(4, ITEM_TYPE_IN_GROUP, false, 3); - assertPositionMetadata(5, ITEM_TYPE_STANDALONE, false, 4); - } - - public void testGroupingWithCollapsedGroupAtTheEnd() { + public void testGroupingWithGroupAtTheEnd() { buildCursor("1", "2", "3", "3", "3"); mAdapter.changeCursor(mCursor); assertEquals(3, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_STANDALONE, false, 1); - assertPositionMetadata(2, ITEM_TYPE_GROUP_HEADER, false, 2); + assertMetadata(0, 1, "1"); + assertMetadata(1, 1, "2"); + assertMetadata(2, 3, "3"); } - public void testGroupingWithExpandedGroupAtTheEnd() { - buildCursor("1", "2", "3", "3", "3"); - mAdapter.changeCursor(mCursor); - mAdapter.toggleGroup(2); - - assertEquals(6, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_STANDALONE, false, 1); - assertPositionMetadata(2, ITEM_TYPE_GROUP_HEADER, true, 2); - assertPositionMetadata(3, ITEM_TYPE_IN_GROUP, false, 2); - assertPositionMetadata(4, ITEM_TYPE_IN_GROUP, false, 3); - assertPositionMetadata(5, ITEM_TYPE_IN_GROUP, false, 4); - } - - public void testGroupingWithMultipleCollapsedGroups() { - buildCursor("1", "2", "2", "3", "4", "4", "5", "5", "6"); - mAdapter.changeCursor(mCursor); - - assertEquals(6, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_GROUP_HEADER, false, 1); - assertPositionMetadata(2, ITEM_TYPE_STANDALONE, false, 3); - assertPositionMetadata(3, ITEM_TYPE_GROUP_HEADER, false, 4); - assertPositionMetadata(4, ITEM_TYPE_GROUP_HEADER, false, 6); - assertPositionMetadata(5, ITEM_TYPE_STANDALONE, false, 8); - } - - public void testGroupingWithMultipleExpandedGroups() { - buildCursor("1", "2", "2", "3", "4", "4", "5", "5", "6"); - mAdapter.changeCursor(mCursor); - mAdapter.toggleGroup(1); - - // Note that expanding the group of 2's shifted the group of 5's down from the - // 4th to the 6th position - mAdapter.toggleGroup(6); - - assertEquals(10, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_GROUP_HEADER, true, 1); - assertPositionMetadata(2, ITEM_TYPE_IN_GROUP, false, 1); - assertPositionMetadata(3, ITEM_TYPE_IN_GROUP, false, 2); - assertPositionMetadata(4, ITEM_TYPE_STANDALONE, false, 3); - assertPositionMetadata(5, ITEM_TYPE_GROUP_HEADER, false, 4); - assertPositionMetadata(6, ITEM_TYPE_GROUP_HEADER, true, 6); - assertPositionMetadata(7, ITEM_TYPE_IN_GROUP, false, 6); - assertPositionMetadata(8, ITEM_TYPE_IN_GROUP, false, 7); - assertPositionMetadata(9, ITEM_TYPE_STANDALONE, false, 8); - } - - public void testPositionCache() { + public void testGroupingWithMultipleGroups() { buildCursor("1", "2", "2", "3", "4", "4", "5", "5", "6"); mAdapter.changeCursor(mCursor); - // First pass - building up cache assertEquals(6, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_GROUP_HEADER, false, 1); - assertPositionMetadata(2, ITEM_TYPE_STANDALONE, false, 3); - assertPositionMetadata(3, ITEM_TYPE_GROUP_HEADER, false, 4); - assertPositionMetadata(4, ITEM_TYPE_GROUP_HEADER, false, 6); - assertPositionMetadata(5, ITEM_TYPE_STANDALONE, false, 8); - - // Second pass - using cache - assertEquals(6, mAdapter.getItemCount()); - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_GROUP_HEADER, false, 1); - assertPositionMetadata(2, ITEM_TYPE_STANDALONE, false, 3); - assertPositionMetadata(3, ITEM_TYPE_GROUP_HEADER, false, 4); - assertPositionMetadata(4, ITEM_TYPE_GROUP_HEADER, false, 6); - assertPositionMetadata(5, ITEM_TYPE_STANDALONE, false, 8); - - // Invalidate cache by expanding a group - mAdapter.toggleGroup(1); - - // First pass - building up cache - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_GROUP_HEADER, true, 1); - assertPositionMetadata(2, ITEM_TYPE_IN_GROUP, false, 1); - assertPositionMetadata(3, ITEM_TYPE_IN_GROUP, false, 2); - assertPositionMetadata(4, ITEM_TYPE_STANDALONE, false, 3); - assertPositionMetadata(5, ITEM_TYPE_GROUP_HEADER, false, 4); - assertPositionMetadata(6, ITEM_TYPE_GROUP_HEADER, false, 6); - assertPositionMetadata(7, ITEM_TYPE_STANDALONE, false, 8); - - // Second pass - using cache - assertPositionMetadata(0, ITEM_TYPE_STANDALONE, false, 0); - assertPositionMetadata(1, ITEM_TYPE_GROUP_HEADER, true, 1); - assertPositionMetadata(2, ITEM_TYPE_IN_GROUP, false, 1); - assertPositionMetadata(3, ITEM_TYPE_IN_GROUP, false, 2); - assertPositionMetadata(4, ITEM_TYPE_STANDALONE, false, 3); - assertPositionMetadata(5, ITEM_TYPE_GROUP_HEADER, false, 4); - assertPositionMetadata(6, ITEM_TYPE_GROUP_HEADER, false, 6); - assertPositionMetadata(7, ITEM_TYPE_STANDALONE, false, 8); + assertMetadata(0, 1, "1"); + assertMetadata(1, 2, "2"); + assertMetadata(2, 1, "3"); + assertMetadata(3, 2, "4"); + assertMetadata(4, 2, "5"); + assertMetadata(5, 1, "6"); } public void testGroupDescriptorArrayGrowth() { @@ -287,14 +158,9 @@ public class GroupingListAdapterTests extends AndroidTestCase { assertEquals(250, mAdapter.getItemCount()); } - private void assertPositionMetadata(int position, int itemType, boolean isExpanded, - int cursorPosition) { - GroupingListAdapter.PositionMetadata metadata = new GroupingListAdapter.PositionMetadata(); - mAdapter.obtainPositionMetadata(metadata, position); - assertEquals(itemType, metadata.itemType); - if (metadata.itemType == ITEM_TYPE_GROUP_HEADER) { - assertEquals(isExpanded, metadata.isExpanded); - } - assertEquals(cursorPosition, metadata.cursorPosition); + private void assertMetadata(int listPosition, int groupSize, String objectValue) { + assertEquals(groupSize, mAdapter.getGroupSize(listPosition)); + MatrixCursor cursor = (MatrixCursor) mAdapter.getItem(listPosition); + assertEquals(objectValue, (String) cursor.getString(GROUPING_COLUMN_INDEX)); } } -- cgit v1.2.3