891953 - Implement empty screen state for reading list

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Lucas Rocha (:lucasr)]

i.e. show some helpful message when you have no reading list items.

Comment 1

[Lucas Rocha (:lucasr)]

Ian, any ideas?

Comment 2

[Ian Barlow (:ibarlow)]

Yes! Likely a quick description and a tip for how to add things to this list -- I'll mock up what I'm thinking about and post it here shortly

Comment 3

[Lucas Rocha (:lucasr)]

Implementation note: this should probably be implemented in the same fashion than the 'Visited' page.

Comment 5

[:Margaret Leibovic]

I'll take this for the hackathon, but I'll need some mockups :)

Comment 6

[Ian Barlow (:ibarlow)]

Attachment: mockup of empty states

Reading list is shown on the far right.

Comment 7

[Ian Barlow (:ibarlow)]

Icons for this screen can be found at https://bugzilla.mozilla.org/attachment.cgi?id=779998

Comment 8

[:Margaret Leibovic]

Passing this off to Chenxia, since she's already working on the empty screen state bugs for the other pages.

Comment 9

[Chenxia Liu [:liuche]]

Bug 895866 does most of the work for this bug, so this will only need a tiny patch after it's reviewed.

Comment 10

[Chenxia Liu [:liuche]]

Attachment: Part 1: Display empty image when list is empty v1

It's interesting that I didn't have to do this for any of the history/tabs empty screens, but for reading list, I need to set the topView, otherwise the empty view gets set most of the time for the history tab.

May need to file follow-ups to store the top View for the other pages.

Comment 11

[Chenxia Liu [:liuche]]

Comment on attachment 789847 [details] [diff] [review]
Part 1: Display empty image when list is empty v1

Actually, scratch the review for the moment - I think that I'm going to have to create another ViewStub specifically for readinglist that includes the TIP at the bottom.

Comment 12

[Chenxia Liu [:liuche]]

Attachment: Part 2: WIP Add tip to reading list page

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Screenshot: bottom tip

Comment 14

[Ian Barlow (:ibarlow)]

This is coming together nicely! 

Is there anything we can do to get that Reader icon aligning a little better with the tip text? Let me know if you need a new image cropped at a different size or something.

Comment 15

[Sriram Ramasubramanian [:sriram]]

(In reply to Ian Barlow (:ibarlow) from comment #14)
> This is coming together nicely! 
> 
> Is there anything we can do to get that Reader icon aligning a little better
> with the tip text? Let me know if you need a new image cropped at a
> different size or something.

There you go! yup yup yup.. we need a new icon with no padding around it :D

Comment 16

[Ian Barlow (:ibarlow)]

Here are some cropped icons http://cl.ly/412B1a3i140r

Comment 17

[Chenxia Liu [:liuche]]

Attachment: Part 2: Add "Tip" to empty reading list page.

This is almost complete, and just needs a slightly smaller icon so the line spacing looks correct.

Comment 18

[Chenxia Liu [:liuche]]

Attachment: Screenshot: line-spacing +3

This is the screenshot with the current icon sizing, linespacing of +3.

Comment 19

[Chenxia Liu [:liuche]]

Attachment: Part 2: Add "Tip" to empty reading list page v2

With smaller icons we don't need line spacing changes! Ready for review, finally.

Comment 20

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 789847 [details] [diff] [review]
Part 1: Display empty image when list is empty v1

Review of attachment 789847 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. r+ with recommended changes.

::: mobile/android/base/home/ReadingListPage.java
@@ +48,5 @@
> +    // Reference to the View to display when there are no results.
> +    private View mEmptyView;
> +
> +    // Reference to top view.
> +    private View topView;

mTopView.

@@ +84,5 @@
>      @Override
>      public void onViewCreated(View view, Bundle savedInstanceState) {
>          super.onViewCreated(view, savedInstanceState);
>  
> +        topView = view;

mTopView.

@@ +147,5 @@
>  
> +    private void updateUiFromCursor(Cursor c) {
> +        if (c == null || c.getCount() == 0) {
> +            // Cursor is empty, so set the empty view if it hasn't been set already.
> +            if (mEmptyView == null) {

Combine all the if together. One identation would be saved :)

if ((c == null || c.getCount() == 0) && (mEmptyView == null)) {
 ... 
}

@@ +218,5 @@
>                  case LOADER_ID_READING_LIST:
>                      mAdapter.swapCursor(c);
>                      break;
>             }
> +           updateUiFromCursor(c);

A new line before this line.

Comment 21

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 790889 [details] [diff] [review]
Part 2: Add "Tip" to empty reading list page v2

Review of attachment 790889 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I would like to move the text and images to XML as much as possible.
r+ with that.

::: mobile/android/base/home/ReadingListPage.java
@@ +159,2 @@
>                  final TextView emptyText = (TextView) mEmptyView.findViewById(R.id.home_empty_text);
>                  emptyText.setText(R.string.home_reading_list_empty);

This could go in XML.

@@ +159,3 @@
>                  final TextView emptyText = (TextView) mEmptyView.findViewById(R.id.home_empty_text);
>                  emptyText.setText(R.string.home_reading_list_empty);
> +                emptyText.setCompoundDrawablesWithIntrinsicBounds(0, R.drawable.icon_reading_list_empty, 0, 0);

This can be set in XML with "drawableTop".
I see that you are creating an empty page specific to reading-list. It's better to set the text and image values there.

::: mobile/android/base/resources/layout/home_empty_reading_page.xml
@@ +13,5 @@
> +    <View android:layout_width="fill_parent"
> +          android:layout_height="0dp"
> +          android:layout_weight="1"/>
> +
> +    <TextView android:id="@+id/home_empty_text"

When you move the text and the image here, since we don't have to refer to this TextView in java, please remove the id.

@@ +15,5 @@
> +          android:layout_weight="1"/>
> +
> +    <TextView android:id="@+id/home_empty_text"
> +              android:layout_width="fill_parent"
> +              android:layout_height="wrap_content"

0dip. Please use height as "0" whenever you set the layout_weight property. This would reduce unnecessary calculations of actual height during onMeasure() pass.

Additional notes: Setting a wrap_content will try to measure the size of text, image and everything before deciding on its inherent size. Setting as 0dip will return this to the parent during first pass. In second pass, parent won't do much work of asking the view to recalculate, as the weight is given a value, a ratio of the available space is set to this view by the parent.

@@ +23,5 @@
> +              android:paddingLeft="50dp"
> +              android:paddingRight="50dp"
> +              android:layout_weight="1"/>
> +
> +    <ImageView android:id="@+id/home_empty_divider"

Please remove this id. It's not used anywhere.

@@ +30,5 @@
> +               android:layout_height="wrap_content"
> +               android:paddingLeft="25dp"
> +               android:paddingRight="25dp"/>
> +
> +    <TextView android:id="@+id/home_empty_hint"

I would recommend try to set the text here. And adding the image alone in Java (if that's easier and possible). I believe you could probably do getText().doCalculationToInsertImage() in java.

Comment 22

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/projects/fig/rev/7b96d62ba83a
https://hg.mozilla.org/projects/fig/rev/1b57558f8619

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1b57558f8619
https://hg.mozilla.org/mozilla-central/rev/7b96d62ba83a