i.e. show some helpful message when you have nothing in your history

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

This needs some padding, wrap, and font tweaks.

This layout will be reusable for all the other empty screens. Opening the soft keyboard will resize the content, which looks kind of strange.

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/

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.

Sriram: I'm not sure how to get text style to be Light, as Ian suggested. This isn't italics, right?

This uses styles instead of a ton of android attrs.

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.

Why don't we use this? http://developer.android.com/reference/android/widget/AdapterView.html#getEmptyView%28%29

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

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.

Renamed icons.

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".

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.

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.

Landed on fig: https://hg.mozilla.org/projects/fig/rev/e979b2ee0d42

WHOOPS forgot to qref the private emptyView change. Landed fo' realz: https://hg.mozilla.org/projects/fig/rev/97996b685d4f

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?

Portrait empty screen on tablet mode.

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.