Closed Bug 895866 Opened 7 years ago Closed 6 years ago

Implement empty screen state for 'history'

Categories

(Firefox for Android :: Theme and Visual Design, defect, P1)

24 Branch
ARM
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 26

People

(Reporter: ibarlow, Assigned: liuche)

References

Details

(Whiteboard: abouthome-hackathon, fixed-fig)

Attachments

(8 files, 11 obsolete files)

150.78 KB, image/png
Details
45.08 KB, image/png
Details
45.43 KB, image/png
ibarlow
: review+
Details
10.34 KB, patch
sriram
: review+
Details | Diff | Splinter Review
12.50 KB, patch
sriram
: review+
Details | Diff | Splinter Review
4.30 KB, patch
sriram
: review+
Details | Diff | Splinter Review
37.42 KB, image/png
Details
39.44 KB, image/png
Details
i.e. show some helpful message when you have nothing in your history
Attached image mockup of empty states
Icons for this screen can be found at https://bugzilla.mozilla.org/attachment.cgi?id=779998
Assignee: nobody → liuche
OS: Mac OS X → Android
Hardware: x86 → ARM
Status: NEW → ASSIGNED
This needs some padding, wrap, and font tweaks.
Attached patch Part 2: icons (obsolete) — Splinter Review
This layout will be reusable for all the other empty screens.

Opening the soft keyboard will resize the content, which looks kind of strange.
Attachment #781830 - Attachment is obsolete: true
Attachment #782022 - Flags: review?(sriram)
Attachment #781833 - Flags: review?(sriram)
Attachment #781832 - Flags: review?(sriram)
Attached image Screenshot: landscape (obsolete) —
Attachment #782025 - Flags: review?(ibarlow)
Attached image Screenshot: vertical (obsolete) —
This needs to be rebased on the current fig, but I want to make sure it's not broken, so here's a build with an older version of Shilpan's bottom tabs to play with: http://people.mozilla.com/~liuche/bug-895866/
Attachment #782027 - Flags: review?(ibarlow)
Empty pages for Bug 895867 (last tabs) and bug 891953 (reading list) will build on the xml introduced in this bug.
Looks great, Chenxia. Let's change the font weight to Light, and the colour to #222222 and we're good!
Rebased on top of current fig.
Attachment #782022 - Attachment is obsolete: true
Attachment #782022 - Flags: review?(sriram)
Attachment #782514 - Flags: review?(sriram)
Attached patch Part 2: icons (obsolete) — Splinter Review
Attachment #781832 - Attachment is obsolete: true
Attachment #781832 - Flags: review?(sriram)
Sriram: I'm not sure how to get text style to be Light, as Ian suggested. This isn't italics, right?
Attachment #782516 - Flags: review?(sriram)
Flags: needinfo?(sriram)
Attachment #781833 - Attachment is obsolete: true
Attachment #781833 - Flags: review?(sriram)
Attachment #782515 - Flags: review?(sriram)
Blocks: 895867
Blocks: 891953
This uses styles instead of a ton of android attrs.
Attachment #782025 - Attachment is obsolete: true
Attachment #782027 - Attachment is obsolete: true
Attachment #782514 - Attachment is obsolete: true
Attachment #782025 - Flags: review?(ibarlow)
Attachment #782027 - Flags: review?(ibarlow)
Attachment #782514 - Flags: review?(sriram)
Attachment #782700 - Flags: review?(sriram)
Attachment #782516 - Attachment is obsolete: true
Attachment #782516 - Flags: review?(sriram)
Attached image Screenshot: Vertical
Attached image Screenshot: landscape
Attachment #782703 - Flags: review?(ibarlow)
Attachment #782701 - Flags: review?(sriram)
Comment on attachment 782515 [details] [diff] [review]
Part 2: icons

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

I have a weird dream of using one size and scaling them down as appropriate. This screen is not a primary screen. There are going to be history elements. Only when there is nothing, this will be shown. And in that case, a performance regression of 20-50ms in re-scaling a bitmap is fine, I guess. Could you try putting the xhdpi in drawable-nodpi/ folder and scaling it down? I would like to see that before r+ for this (Note: this patch looks good, just want to try something new :)).
Comment on attachment 782700 [details] [diff] [review]
Part 1: xml for empty home screens v3

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

I like how the refactoring is done. This patch needs a couple of changes though. I would like to see another version before deciding on r+.

::: mobile/android/base/home/HomeFragment.java
@@ +254,5 @@
> +     * @param makeVisible boolean to show empty content screen visible
> +     * @param imgRes resource of image to display in empty content screen
> +     * @param textRes resource of text to display in empty content screen
> +     */
> +    protected void displayEmptyHint(boolean makeVisible, int imgRes, int textRes) {

I recommend renaming makeVisible to "show". Also, the method name can be "showEmptyHint(...)". We tend to use "showXYZ" and not "displayXYZ".

@@ +255,5 @@
> +     * @param imgRes resource of image to display in empty content screen
> +     * @param textRes resource of text to display in empty content screen
> +     */
> +    protected void displayEmptyHint(boolean makeVisible, int imgRes, int textRes) {
> +        LinearLayout emptyHintLayout = (LinearLayout) getActivity().findViewById(R.id.home_layout_empty);

Using getActivity() scares me. Here's the problem. The home_page_empty is common for all home-list-views-with-title. Now, this findViewById will return the first occurence of it -- which may not be the intended one.

Using getActivity() to find views is good, if the view id is used only once. For such re-usable views, it's better to use the nearest parent. In this case, you could store the view returned during onViewCreated() (and null it on destroy), and call findViewById from that view as the parent. This solves the issue, and findViewById() is faster too.

Or, you could store the empty view in a variable during onViewCreated() and use it. -- this would be much better, as there is just one call to findViewById(). (Though this cannot be done in HomeFragment)

@@ +258,5 @@
> +    protected void displayEmptyHint(boolean makeVisible, int imgRes, int textRes) {
> +        LinearLayout emptyHintLayout = (LinearLayout) getActivity().findViewById(R.id.home_layout_empty);
> +        if (makeVisible) {
> +            // Display empty hint page.
> +            emptyHintLayout.setVisibility(LinearLayout.VISIBLE);

View.VISIBLE is better (though LinearLayout shadows it). We use it with View everywhere. So for the sake of consistency and my OCD. ;)

::: mobile/android/base/resources/layout/home_list_with_title.xml
@@ +4,5 @@
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
>  
>  <merge xmlns:android="http://schemas.android.com/apk/res/android">
>  
> +    <include layout="@layout/home_page_empty"/>

Do we really need to have a <include> inside a <merge> ??
This is a performance problem.
Why not have the entire layout here? Is there any other layout that uses this layout file, but doesn't need home_page_empty.xml? Even in that case, wouldn't this home_page_empty be INVISIBLE by default?

@@ +9,2 @@
>  
> +    <LinearLayout android:id="@+id/home_layout_content"

This is an extra layout. It helps in code to show/hide a block. But from performance point of view, an extra layer is a regression. Could you please move the empty screen views inside the <merge> itself? The code should show/hide views based on it.

::: mobile/android/base/resources/layout/home_page_empty.xml
@@ +8,5 @@
> +    <LinearLayout android:id="@+id/home_layout_empty"
> +                  android:layout_width="fill_parent"
> +                  android:layout_height="fill_parent"
> +                  android:gravity="center"
> +                  android:orientation="vertical"

Move this line above. It's better to have it with layout_height. OCD :D

@@ +13,5 @@
> +                  android:paddingLeft="50dp"
> +                  android:paddingRight="50dp"
> +                  android:visibility="gone">
> +
> +        <ImageView android:id="@+id/home_empty_img"

Something tells me that this image can be merged with the TextView using CompoundDrawables approach.
Only one catch though, it would take the inherit size -- in which case my idea of scaling down bitmap wouldn't work.
We can enforce the size in code (and we do that with our custom menu).
Could you try using compound-drawables? ("android:drawableTop").

::: mobile/android/base/resources/values-v16/styles.xml
@@ +5,5 @@
>  
>  <resources xmlns:android="http://schemas.android.com/apk/res/android">
>  
> +    <style name="TextAppearance.EmptyMessage" parent="TextAppearance.Large">
> +        <item name="android:textColor">#222222</item>

Same comment as in values/styles.xml (below).

::: mobile/android/base/resources/values/styles.xml
@@ +249,5 @@
>          <item name="android:textColorLink">?android:attr/textColorLink</item>
>      </style>
>  
> +    <style name="TextAppearance.EmptyMessage" parent="TextAppearance.Large">
> +        <item name="android:textColor">#222222</item>

I believe Large uses primary text color, which is #222222. This line is not needed. Please have it only if its not #222222.
Attachment #782700 - Flags: review?(sriram) → review-
Comment on attachment 782701 [details] [diff] [review]
Part 3: Incorporate empty hint page for history v3

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

Requires re-structuring. But based on previous comment, this patch might not even be needed (by using an emptyView).

::: mobile/android/base/home/MostRecentPage.java
@@ +213,5 @@
>          return MostRecentSection.OLDER;
>      }
>  
>      private void loadMostRecentSections(Cursor c) {
> +       if (c != null && c.moveToFirst()) {

That's a big if-else indentation. I would like this to be restructured as:

if (c == null || !c.moveToFirst()) {
    displayEmptyHint();
    return;
}

// rest stays there happppily. :D
Attachment #782701 - Flags: review?(sriram) → review-
Good suggestions - I'll try out these changes, but I'm currently finishing up things for 25, so this will be a slightly lower priority.
Using nodpi with scaled ImageView didn't work so well. Images were either not scaled at all, or became miniscule.

CompoundDrawables also didn't work so well, because there isn't a way to fine-tune the positioning of the drawable - it didn't obey the gravity of the TextView or the LinearLayout parent and would always appears up at the top of the layout. Couldn't add padding because that messed up landscape view.

Kept the LinearLayout so we can control the gravity of the Image/Text inside the screen. Removed the LinearLayout for the other stuff. It's kind of cheating, because the list title is still in the view, just hidden behind the bottom bar, but it's less code. This will have to change a little for ReadingList, since there is not bottom bar there. Possibly will also have to change for tabs, because there's a "show tabs from last time" button in that view.

Fixed styles.xml and layout nits.
Attachment #786601 - Flags: review?(sriram)
Attached patch Part 2: icons v2Splinter Review
Renamed icons.
Attachment #782515 - Attachment is obsolete: true
Attachment #782700 - Attachment is obsolete: true
Attachment #782515 - Flags: review?(sriram)
Attachment #786604 - Flags: review?(sriram)
Comment on attachment 786601 [details] [diff] [review]
Part 1: xml for empty home screens v4

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

Looks good. Thanks :)

::: mobile/android/base/resources/layout/home_list_with_title.xml
@@ +13,5 @@
> +                  android:paddingLeft="50dp"
> +                  android:paddingRight="50dp"
> +                  android:visibility="gone">
> +
> +        <ImageView android:id="@+id/home_empty_img"

I would recommend using "home_empty_image".
Attachment #786601 - Flags: review?(sriram) → review+
Use view.setEmptyView now. There's occasionally a flash of the image, and if you clear history while you're on the history you won't see the image until you navigate back to the history page again, but this is so much cleaner, less jank and hiding/showing views and if checks in ListView handling code.
Attachment #782701 - Attachment is obsolete: true
Attachment #786606 - Flags: review?(sriram)
Comment on attachment 786606 [details] [diff] [review]
Part 3: Incorporate empty hint page for history v4

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

r+ with removing the private variable.

::: mobile/android/base/home/MostRecentPage.java
@@ +106,5 @@
>              }
>          });
>  
> +        // Set empty page view.
> +        if (emptyView == null) {

The reason for the image not showing up could be due to this. Basically when the fragment re-attaches, the View is still pointing to something else. I don't think we need to hold on to a reference here. It's better to just find the id and set the values. I don't get the context of this function, but I guess this will be inside onViewCreated(). In that case, you would have to set the values, every time a view is created.
Attachment #786606 - Flags: review?(sriram) → review+
Attachment #786604 - Flags: review?(sriram) → review+
Landed on fig: https://hg.mozilla.org/projects/fig/rev/e979b2ee0d42
Whiteboard: abouthome-hackathon → abouthome-hackathon, fixed-fig
WHOOPS forgot to qref the private emptyView change.

Landed fo' realz: https://hg.mozilla.org/projects/fig/rev/97996b685d4f
Attachment #782703 - Flags: review?(ibarlow) → review+
Blocks: 902507
With the tablet mode from bug 894077, the empty screens start to look a little weird because there is a lotlotlot of whitespace, and look like they're floating/off-center.

Ian, I know you just cleared the needinfo on this, but now that I've this IRL, any feedback on these screenshots and what we should do about tablet mode?
Flags: needinfo?(ibarlow)
Portrait empty screen on tablet mode.
Attachment #787777 - Flags: review?(ibarlow)
Attachment #787776 - Flags: review?(ibarlow)
Blocks: 903158
Blocks: 903160
Hm, I see your point. I think we can reposition these a bit so they look better. I made a couple of mockups: 

Landscape http://cl.ly/image/0G1a1J1E1W0X
Portrait http://cl.ly/image/282f172I0O3y

Note that I also included a single list rule above the icon and message, just to give them something to visually center against.
Flags: needinfo?(ibarlow)
https://hg.mozilla.org/mozilla-central/rev/97996b685d4f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Attachment #787776 - Flags: review?(ibarlow)
Attachment #787777 - Flags: review?(ibarlow)
You need to log in before you can comment on or make changes to this bug.