Read later: Keep both section headers when there is only 1 item.

Keep both read/unread section header in reading list UI when there
is only 1 reading list item. Also add a 15dp padding between.

Bug: 1166394
Change-Id: Iac54ed461c0610045427825ac1b888cfd041239c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2628447
Commit-Queue: Xing Liu <xingliu@chromium.org>
Reviewed-by: David Trainor <dtrainor@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845916}

chrome/android/java/res/values/dimens.xml

@@ -380,6 +380,7 @@
     <dimen name="bookmark_minimum_dialog_size_tablet">500dp</dimen>
     <dimen name="bookmark_list_view_padding_top">64dp</dimen>
     <dimen name="bookmark_dialog_toolbar_shadow_height">4dp</dimen>
+    <dimen name="bookmark_reading_list_section_header_padding_top">15dp</dimen>
 
     <!-- Bookmark widget dimensions -->
     <dimen name="bookmark_widget_min_width">64dp</dimen>

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkItemsAdapter.java

@@ -246,6 +246,11 @@ class BookmarkItemsAdapter extends DragReorderableListAdapter<BookmarkListEntry>
         description.setText(listItem.getHeaderDescription());
         description.setVisibility(
                 TextUtils.isEmpty(listItem.getHeaderDescription()) ? View.GONE : View.VISIBLE);
+        if (listItem.getSectionHeaderData().topPadding > 0) {
+            title.setPaddingRelative(title.getPaddingStart(),
+                    listItem.getSectionHeaderData().topPadding, title.getPaddingEnd(),
+                    title.getPaddingBottom());
+        }
     }
 
     @Override
@@ -500,8 +505,7 @@ class BookmarkItemsAdapter extends DragReorderableListAdapter<BookmarkListEntry>
     private int getBookmarkItemEndIndex() {
         int endIndex = mElements.size() - 1;
         BookmarkItem bookmarkItem = mElements.get(endIndex).getBookmarkItem();
-        assert bookmarkItem != null;
-        if (!bookmarkItem.isMovable()) {
+        if (bookmarkItem == null || !bookmarkItem.isMovable()) {
             endIndex--;
         }
         return endIndex;

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkListEntry.java

@@ -8,10 +8,7 @@ import android.content.Context;
 
 import androidx.annotation.IntDef;
 import androidx.annotation.Nullable;
-import androidx.annotation.StringRes;
 
-import org.chromium.base.ContextUtils;
-import org.chromium.chrome.R;
 import org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkItem;
 
 import java.lang.annotation.Retention;
@@ -41,27 +38,33 @@ final class BookmarkListEntry {
         int SECTION_HEADER = 6;
     }
 
+    /**
+     * Contains data used by section header in bookmark UI.
+     */
+    static final class SectionHeaderData {
+        public final CharSequence headerTitle;
+        public final CharSequence headerDescription;
+        public final int topPadding;
+
+        SectionHeaderData(
+                @Nullable CharSequence title, @Nullable CharSequence description, int topPadding) {
+            headerTitle = title;
+            headerDescription = description;
+            this.topPadding = topPadding;
+        }
+    }
+
     private final @ViewType int mViewType;
     @Nullable
     private final BookmarkItem mBookmarkItem;
     @Nullable
-    private String mHeaderTitle;
-    @Nullable
-    private String mHeaderDescription;
+    private final SectionHeaderData mSectionHeaderData;
 
-    private BookmarkListEntry(int viewType, @Nullable BookmarkItem bookmarkItem) {
+    private BookmarkListEntry(int viewType, @Nullable BookmarkItem bookmarkItem,
+            @Nullable SectionHeaderData sectionHeaderData) {
         this.mViewType = viewType;
         this.mBookmarkItem = bookmarkItem;
-    }
-
-    /** Constructor for section headers. */
-    private BookmarkListEntry(
-            int viewType, String headerTitle, @Nullable String headerDescription) {
-        assert viewType == ViewType.SECTION_HEADER;
-        this.mViewType = viewType;
-        this.mBookmarkItem = null;
-        this.mHeaderTitle = headerTitle;
-        this.mHeaderDescription = headerDescription;
+        this.mSectionHeaderData = sectionHeaderData;
     }
 
     /**
@@ -69,8 +72,8 @@ final class BookmarkListEntry {
      * @param bookmarkItem The data object created from the bookmark backend.
      */
     static BookmarkListEntry createBookmarkEntry(@Nonnull BookmarkItem bookmarkItem) {
-        return new BookmarkListEntry(
-                bookmarkItem.isFolder() ? ViewType.FOLDER : ViewType.BOOKMARK, bookmarkItem);
+        return new BookmarkListEntry(bookmarkItem.isFolder() ? ViewType.FOLDER : ViewType.BOOKMARK,
+                bookmarkItem, /*sectionHeaderData=*/null);
     }
 
     /**
@@ -80,14 +83,15 @@ final class BookmarkListEntry {
     static BookmarkListEntry createSyncPromoHeader(@ViewType int viewType) {
         assert viewType == ViewType.PERSONALIZED_SIGNIN_PROMO
                 || viewType == ViewType.PERSONALIZED_SYNC_PROMO || viewType == ViewType.SYNC_PROMO;
-        return new BookmarkListEntry(viewType, /*bookmarkItem=*/null);
+        return new BookmarkListEntry(viewType, /*bookmarkItem=*/null, /*sectionHeaderData=*/null);
     }
 
     /**
      * Creates a divider to separate sections in the bookmark list.
      */
     static BookmarkListEntry createDivider() {
-        return new BookmarkListEntry(ViewType.DIVIDER, /*bookmarkItem=*/null);
+        return new BookmarkListEntry(
+                ViewType.DIVIDER, /*bookmarkItem=*/null, /*sectionHeaderData=*/null);
     }
 
     /**
@@ -101,27 +105,16 @@ final class BookmarkListEntry {
 
     /**
      * Create an entry representing the reading list read/unread section header.
-     * @param titleRes The string resource for title text.
-     * @param descriptionRes The string resource for description.
+     * @param title The title of the section header.
+     * @param description The description of the section header.
+     * @param topPadding The top padding of the section header. Only impacts the padding when
+     *         greater than 0.
      * @param context The context to use.
      */
-    static BookmarkListEntry createSectionHeader(@StringRes Integer titleRes,
-            @Nullable @StringRes Integer descriptionRes, Context context) {
-        return new BookmarkListEntry(ViewType.SECTION_HEADER, context.getString(titleRes),
-                descriptionRes == null ? null : context.getString(descriptionRes));
-    }
-
-    /**
-     * Create an entry representing the reading list read/unread section header.
-     * @param read True if it represents read section, false for unread section.
-     */
-    static BookmarkListEntry createReadingListSectionHeader(boolean read) {
-        Context context = ContextUtils.getApplicationContext();
-        return read ? new BookmarkListEntry(
-                       ViewType.SECTION_HEADER, context.getString(R.string.reading_list_read), null)
-                    : new BookmarkListEntry(ViewType.SECTION_HEADER,
-                            context.getString(R.string.reading_list_unread),
-                            context.getString(R.string.reading_list_ready_for_offline));
+    static BookmarkListEntry createSectionHeader(
+            CharSequence title, CharSequence description, int topPadding, Context context) {
+        SectionHeaderData sectionHeaderData = new SectionHeaderData(title, description, topPadding);
+        return new BookmarkListEntry(ViewType.SECTION_HEADER, null, sectionHeaderData);
     }
 
     /**
@@ -142,13 +135,21 @@ final class BookmarkListEntry {
 
     /** @return The title text to be shown if it is a section header. */
     @Nullable
-    String getHeaderTitle() {
-        return mHeaderTitle;
+    CharSequence getHeaderTitle() {
+        return mSectionHeaderData.headerTitle;
     }
 
     /** @return The description text to be shown if it is a section header. */
     @Nullable
-    String getHeaderDescription() {
-        return mHeaderDescription;
+    CharSequence getHeaderDescription() {
+        return mSectionHeaderData.headerDescription;
+    }
+
+    /**
+     * @return The {@link SectionHeaderData}. Could be null if this entry is not a section header.
+     */
+    @Nullable
+    SectionHeaderData getSectionHeaderData() {
+        return mSectionHeaderData;
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/bookmarks/ReadingListSectionHeader.java

@@ -34,7 +34,7 @@ class ReadingListSectionHeader {
             List<BookmarkListEntry> listItems, Context context) {
         if (listItems.isEmpty()) return;
 
-        // The topmost item(s) could be promo headers.
+        // Compute the first reading list index. The topmost item(s) could be promo headers.
         int readingListStartIndex = 0;
         for (BookmarkListEntry listItem : listItems) {
             boolean isReadingListItem = listItem.getBookmarkItem() != null
@@ -48,22 +48,23 @@ class ReadingListSectionHeader {
         sort(listItems, readingListStartIndex);
         recordMetrics(listItems);
 
-        // Add a section header at the top. If it is for read, exit right away.
-        assert listItems.get(readingListStartIndex).getBookmarkItem().getId().getType()
-                == BookmarkType.READING_LIST;
-        boolean isRead = listItems.get(readingListStartIndex).getBookmarkItem().isRead();
-        listItems.add(readingListStartIndex, createReadingListSectionHeader(isRead, context));
-        if (isRead) return;
+        // Always show both read/unread section headers even if we may have only one reading list
+        // item.
+        listItems.add(
+                readingListStartIndex, createReadingListSectionHeader(/*read=*/false, context));
 
         // Search for the first read element, and insert the read section header.
-        for (int i = readingListStartIndex + 2; i < listItems.size(); i++) {
+        for (int i = readingListStartIndex + 1; i < listItems.size(); i++) {
             BookmarkListEntry listItem = listItems.get(i);
             assert listItem.getBookmarkItem().getId().getType() == BookmarkType.READING_LIST;
             if (listItem.getBookmarkItem().isRead()) {
-                listItems.add(i, createReadingListSectionHeader(true /*read*/, context));
+                listItems.add(i, createReadingListSectionHeader(/*read=*/true, context));
                 return;
             }
         }
+
+        // If no read reading list items, add a read section header at the end.
+        listItems.add(listItems.size(), createReadingListSectionHeader(/*read=*/true, context));
     }
 
     /**
@@ -81,9 +82,14 @@ class ReadingListSectionHeader {
     }
 
     private static BookmarkListEntry createReadingListSectionHeader(boolean read, Context context) {
-        return BookmarkListEntry.createSectionHeader(
-                read ? R.string.reading_list_read : R.string.reading_list_unread,
-                read ? null : R.string.reading_list_ready_for_offline, context);
+        String title =
+                context.getString(read ? R.string.reading_list_read : R.string.reading_list_unread);
+        String description =
+                read ? null : context.getString(R.string.reading_list_ready_for_offline);
+        int paddingTop = read ? context.getResources().getDimensionPixelSize(
+                                 R.dimen.bookmark_reading_list_section_header_padding_top)
+                              : 0;
+        return BookmarkListEntry.createSectionHeader(title, description, paddingTop, context);
     }
 
     private static void recordMetrics(List<BookmarkListEntry> listItems) {

chrome/android/junit/src/org/chromium/chrome/browser/bookmarks/ReadingListSectionHeaderTest.java

@@ -8,6 +8,9 @@ import static org.junit.Assert.assertEquals;
 
 import android.content.Context;
 
+import androidx.annotation.StringRes;
+
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -16,11 +19,11 @@ import org.robolectric.annotation.Config;
 import org.chromium.base.ContextUtils;
 import org.chromium.base.metrics.test.ShadowRecordHistogram;
 import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.chrome.R;
 import org.chromium.chrome.browser.bookmarks.BookmarkBridge.BookmarkItem;
 import org.chromium.chrome.browser.bookmarks.BookmarkListEntry.ViewType;
 import org.chromium.components.bookmarks.BookmarkId;
 import org.chromium.components.bookmarks.BookmarkType;
-import org.chromium.url.GURL;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -34,11 +37,29 @@ public class ReadingListSectionHeaderTest {
         ShadowRecordHistogram.reset();
     }
 
+    private BookmarkListEntry createReadingListEntry(long id, boolean read) {
+        BookmarkId bookmarkId = new BookmarkId(id, BookmarkType.READING_LIST);
+        BookmarkItem bookmarkItem =
+                new BookmarkItem(bookmarkId, null, null, false, null, false, false, 0, read);
+        return BookmarkListEntry.createBookmarkEntry(bookmarkItem);
+    }
+
+    private void assertSectionHeader(
+            BookmarkListEntry entry, @StringRes int titleRes, int paddingTopResId) {
+        Context context = ContextUtils.getApplicationContext();
+        Assert.assertEquals(ViewType.SECTION_HEADER, entry.getViewType());
+        Assert.assertEquals(context.getString(titleRes), entry.getSectionHeaderData().headerTitle);
+        if (paddingTopResId > 0) {
+            Assert.assertEquals(context.getResources().getDimensionPixelSize(paddingTopResId),
+                    entry.getSectionHeaderData().topPadding);
+        }
+    }
+
     @Test
     public void testListWithReadUnreadItems() {
         Context context = ContextUtils.getApplicationContext();
-        String titleRead = context.getString(org.chromium.chrome.R.string.reading_list_read);
-        String titleUnread = context.getString(org.chromium.chrome.R.string.reading_list_unread);
+        String titleRead = context.getString(R.string.reading_list_read);
+        String titleUnread = context.getString(R.string.reading_list_unread);
 
         List<BookmarkListEntry> listItems = new ArrayList<>();
         listItems.add(createReadingListEntry(1, true));
@@ -73,7 +94,7 @@ public class ReadingListSectionHeaderTest {
     @Test
     public void testListWithReadUnreadAndPromoItems() {
         Context context = ContextUtils.getApplicationContext();
-        String titleRead = context.getString(org.chromium.chrome.R.string.reading_list_read);
+        String titleRead = context.getString(R.string.reading_list_read);
 
         List<BookmarkListEntry> listItems = new ArrayList<>();
         listItems.add(BookmarkListEntry.createSyncPromoHeader(ViewType.PERSONALIZED_SIGNIN_PROMO));
@@ -81,16 +102,16 @@ public class ReadingListSectionHeaderTest {
         listItems.add(createReadingListEntry(2, true));
         ReadingListSectionHeader.maybeSortAndInsertSectionHeaders(listItems, context);
 
-        assertEquals("Incorrect number of items in the adapter", 4, listItems.size());
+        assertEquals("Incorrect number of items in the adapter", 5, listItems.size());
         assertEquals("Expected promo section header", ViewType.PERSONALIZED_SIGNIN_PROMO,
                 listItems.get(0).getViewType());
-        assertEquals("Expected read section header", ViewType.SECTION_HEADER,
-                listItems.get(1).getViewType());
-        assertEquals("Expected read title text", titleRead, listItems.get(1).getHeaderTitle());
+        assertSectionHeader(listItems.get(1), R.string.reading_list_unread, 0);
+        assertSectionHeader(listItems.get(2), R.string.reading_list_read,
+                R.dimen.bookmark_reading_list_section_header_padding_top);
         assertEquals(
-                "Expected a different item", 1, listItems.get(2).getBookmarkItem().getId().getId());
+                "Expected a different item", 1, listItems.get(3).getBookmarkItem().getId().getId());
         assertEquals(
-                "Expected a different item", 2, listItems.get(3).getBookmarkItem().getId().getId());
+                "Expected a different item", 2, listItems.get(4).getBookmarkItem().getId().getId());
     }
 
     @Test
@@ -116,38 +137,31 @@ public class ReadingListSectionHeaderTest {
     @Test
     public void testUnreadItemsOnly() {
         Context context = ContextUtils.getApplicationContext();
-        String titleUnread = context.getString(org.chromium.chrome.R.string.reading_list_unread);
+        String titleUnread = context.getString(R.string.reading_list_unread);
 
         List<BookmarkListEntry> listItems = new ArrayList<>();
         listItems.add(createReadingListEntry(1, false));
         listItems.add(createReadingListEntry(2, false));
         ReadingListSectionHeader.maybeSortAndInsertSectionHeaders(listItems, context);
 
-        assertEquals("Incorrect number of items in the adapter", 3, listItems.size());
-        assertEquals(
-                "Expected section header", ViewType.SECTION_HEADER, listItems.get(0).getViewType());
-        assertEquals("Expected unread title text", titleUnread, listItems.get(0).getHeaderTitle());
+        assertEquals("Incorrect number of items in the adapter", 4, listItems.size());
+        assertSectionHeader(listItems.get(0), R.string.reading_list_unread, 0);
+        assertSectionHeader(listItems.get(3), R.string.reading_list_read,
+                R.dimen.bookmark_reading_list_section_header_padding_top);
     }
 
     @Test
     public void testReadItemsOnly() {
         Context context = ContextUtils.getApplicationContext();
-        String titleRead = context.getString(org.chromium.chrome.R.string.reading_list_read);
+        String titleRead = context.getString(R.string.reading_list_read);
 
         List<BookmarkListEntry> listItems = new ArrayList<>();
         listItems.add(createReadingListEntry(1, true));
         ReadingListSectionHeader.maybeSortAndInsertSectionHeaders(listItems, context);
 
-        assertEquals("Incorrect number of items in the adapter", 2, listItems.size());
-        assertEquals(
-                "Expected section header", ViewType.SECTION_HEADER, listItems.get(0).getViewType());
-        assertEquals("Expected read title text", titleRead, listItems.get(0).getHeaderTitle());
-    }
-
-    private BookmarkListEntry createReadingListEntry(long id, boolean read) {
-        BookmarkId bookmarkId = new BookmarkId(id, BookmarkType.READING_LIST);
-        BookmarkItem bookmarkItem = new BookmarkItem(
-                bookmarkId, null, GURL.emptyGURL(), false, null, false, false, 0, read);
-        return BookmarkListEntry.createBookmarkEntry(bookmarkItem);
+        assertEquals("Incorrect number of items in the adapter", 3, listItems.size());
+        assertSectionHeader(listItems.get(0), R.string.reading_list_unread, 0);
+        assertSectionHeader(listItems.get(1), R.string.reading_list_read,
+                R.dimen.bookmark_reading_list_section_header_padding_top);
     }
 }