jenkins-bot has submitted this change and it was merged.

Change subject: Iron out issues in Read More component.
......................................................................


Iron out issues in Read More component.

- Dynamically size the component based on the number of page suggestions
  retrieved.
- Fix layout issues / inconsistencies.
- Will follow up with patch to enable scrolling when swiping the list
  items.

Change-Id: Ie5b02c0afca51f44ff444f1759847945fb72aba6
---
M wikipedia/res/layout/fragment_page.xml
M wikipedia/src/main/java/org/wikipedia/page/PageViewFragmentInternal.java
M 
wikipedia/src/main/java/org/wikipedia/page/bottomcontent/BottomContentHandler.java
M wikipedia/src/main/java/org/wikipedia/views/ObservableWebView.java
4 files changed, 143 insertions(+), 74 deletions(-)

Approvals:
  BearND: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/wikipedia/res/layout/fragment_page.xml 
b/wikipedia/res/layout/fragment_page.xml
index 402ca01..362e285 100644
--- a/wikipedia/res/layout/fragment_page.xml
+++ b/wikipedia/res/layout/fragment_page.xml
@@ -75,14 +75,14 @@
                 </FrameLayout>
             </FrameLayout>
 
-            <FrameLayout
+            <LinearLayout
                 android:id="@+id/bottom_content_container"
                 android:layout_width="match_parent"
                 android:layout_height="wrap_content"
-                android:orientation="vertical"
-                android:background="?attr/page_background_color"
+                android:layout_marginBottom="-400dp"
                 android:layout_gravity="bottom"
-                android:visibility="invisible">
+                android:orientation="vertical"
+                android:visibility="gone">
                 <LinearLayout
                     android:id="@+id/read_more_container"
                     android:layout_width="match_parent"
@@ -92,38 +92,35 @@
                         style="?android:textAppearanceLarge"
                         android:layout_width="wrap_content"
                         android:layout_height="wrap_content"
+                        android:paddingLeft="@dimen/activity_horizontal_margin"
+                        
android:paddingRight="@dimen/activity_horizontal_margin"
                         android:paddingBottom="16dp"
-                        android:paddingLeft="16dp"
-                        android:paddingRight="16dp"
                         android:fontFamily="serif"
                         android:textSize="24sp"
-                        android:text="@string/read_more_section"
-                        />
+                        android:text="@string/read_more_section"/>
                     <ListView
                         android:id="@+id/read_more_list"
                         android:layout_width="match_parent"
-                        android:layout_height="320dp">
-                    </ListView>
+                        android:layout_height="0dp"/>
                 </LinearLayout>
                 <LinearLayout
                     android:layout_width="match_parent"
                     android:layout_height="wrap_content"
                     android:orientation="vertical"
-                    android:layout_gravity="bottom"
+                    android:paddingBottom="16dp"
                     android:background="?attr/subtle_gray_color">
                     <View
                         android:layout_width="match_parent"
                         android:layout_height="8dp"
-                        android:layout_gravity="top"
                         android:background="@drawable/toolbar_bottom_shadow"/>
 
                     <TextView
                         android:id="@+id/page_last_updated_text"
                         android:layout_width="match_parent"
                         android:layout_height="wrap_content"
+                        android:paddingTop="16dp"
                         style="?android:textAppearanceSmall"
                         android:textColorLink="?attr/link_color"
-                        android:paddingTop="16dp"
                         android:gravity="center"/>
                     <TextView
                         android:id="@+id/page_license_text"
@@ -131,10 +128,9 @@
                         android:layout_height="wrap_content"
                         style="?android:textAppearanceSmall"
                         android:textColorLink="?attr/link_color"
-                        android:paddingBottom="16dp"
                         android:gravity="center"/>
                 </LinearLayout>
-            </FrameLayout>
+            </LinearLayout>
 
         </FrameLayout>
 
diff --git 
a/wikipedia/src/main/java/org/wikipedia/page/PageViewFragmentInternal.java 
b/wikipedia/src/main/java/org/wikipedia/page/PageViewFragmentInternal.java
index b6156fd..522c86b 100644
--- a/wikipedia/src/main/java/org/wikipedia/page/PageViewFragmentInternal.java
+++ b/wikipedia/src/main/java/org/wikipedia/page/PageViewFragmentInternal.java
@@ -113,6 +113,7 @@
     private PageTitle titleOriginal;
     private ViewGroup imagesContainer;
     private LeadImagesHandler leadImagesHandler;
+    private BottomContentHandler bottomContentHandler;
     private ObservableWebView webView;
     private SwipeRefreshLayoutWithScroll refreshView;
     private View networkError;
@@ -414,6 +415,11 @@
         imagesContainer = (ViewGroup) 
parentFragment.getView().findViewById(R.id.page_images_container);
         leadImagesHandler = new LeadImagesHandler(parentFragment, bridge, 
webView, imagesContainer);
 
+        bottomContentHandler = new BottomContentHandler(parentFragment, bridge,
+                webView, linkHandler,
+                (ViewGroup) 
parentFragment.getView().findViewById(R.id.bottom_content_container),
+                title);
+
         //is this page in cache??
         if (PAGE_CACHE.has(titleOriginal)) {
             Log.d(TAG, "Using page from cache: " + 
titleOriginal.getDisplayText());
@@ -481,11 +487,8 @@
                 // Do any other stuff that should happen upon page load 
completion...
                 getActivity().updateProgressBar(false, true, 0);
 
-                // create bottom content for this page...
-                new BottomContentHandler(parentFragment, bridge, webView, 
linkHandler,
-                        (ViewGroup) 
parentFragment.getView().findViewById(R.id.bottom_content_container),
-                        title, page.getPageProperties().isMainPage());
-
+                // trigger layout of the bottom content
+                bottomContentHandler.beginLayout();
             }
         });
     }
@@ -502,6 +505,8 @@
             webView.setVisibility(View.GONE);
             // and the lead image
             leadImagesHandler.hide();
+            // and the bottom content
+            bottomContentHandler.hide();
 
             // and reload the page...
             setState(STATE_NO_FETCH);
@@ -888,6 +893,7 @@
     private void showNetworkError() {
         // Check for the source of the error and have different things turn up
         leadImagesHandler.hide();
+        bottomContentHandler.hide();
         webView.setVisibility(View.INVISIBLE);
         ViewAnimations.fadeIn(networkError);
     }
@@ -1004,6 +1010,8 @@
         webView.setVisibility(View.GONE);
         // and the lead image
         leadImagesHandler.hide();
+        // and the bottom content
+        bottomContentHandler.hide();
         curEntry = new HistoryEntry(title, HistoryEntry.SOURCE_HISTORY);
         setState(STATE_NO_FETCH);
         performActionForState(state);
diff --git 
a/wikipedia/src/main/java/org/wikipedia/page/bottomcontent/BottomContentHandler.java
 
b/wikipedia/src/main/java/org/wikipedia/page/bottomcontent/BottomContentHandler.java
index 86b1180..2ab97a7 100644
--- 
a/wikipedia/src/main/java/org/wikipedia/page/bottomcontent/BottomContentHandler.java
+++ 
b/wikipedia/src/main/java/org/wikipedia/page/bottomcontent/BottomContentHandler.java
@@ -1,9 +1,8 @@
 package org.wikipedia.page.bottomcontent;
 
-import android.graphics.Point;
-import android.os.Build;
 import android.text.Html;
 import android.text.TextUtils;
+import android.util.Log;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
@@ -12,6 +11,7 @@
 import android.widget.BaseAdapter;
 import android.widget.FrameLayout;
 import android.widget.ImageView;
+import android.widget.ListAdapter;
 import android.widget.ListView;
 import android.widget.TextView;
 
@@ -40,7 +40,9 @@
 import java.util.List;
 import java.util.Map;
 
-public class BottomContentHandler implements 
ObservableWebView.OnScrollChangeListener {
+public class BottomContentHandler implements 
ObservableWebView.OnScrollChangeListener,
+        ObservableWebView.OnContentHeightChangedListener {
+    private static final String TAG = "BottomContentHandler";
     private final PageViewFragment parentFragment;
     private final CommunicationBridge bridge;
     private final WebView webView;
@@ -48,17 +50,20 @@
     private final PageTitle pageTitle;
     private final PageActivity activity;
     private final WikipediaApp app;
-
-    private int displayHeight;
     private float displayDensity;
+    private boolean firstTimeShown = false;
 
     private View bottomContentContainer;
     private TextView pageLastUpdatedText;
     private TextView pageLicenseText;
+    private ListView readMoreList;
+
+    private SuggestedPagesFunnel funnel;
+    private FullSearchArticlesTask.FullSearchResults readMoreItems;
 
     public BottomContentHandler(final PageViewFragment parentFragment, 
CommunicationBridge bridge,
-                                ObservableWebView webview, LinkHandler 
linkHandler, ViewGroup hidingView,
-                                PageTitle pageTitle, boolean isMainPage) {
+                                ObservableWebView webview, LinkHandler 
linkHandler,
+                                ViewGroup hidingView, PageTitle pageTitle) {
         this.parentFragment = parentFragment;
         this.bridge = bridge;
         this.webView = webview;
@@ -70,77 +75,115 @@
 
         bottomContentContainer = hidingView;
         webview.addOnScrollChangeListener(this);
+        webview.addOnContentHeightChangedListener(this);
 
         pageLastUpdatedText = 
(TextView)bottomContentContainer.findViewById(R.id.page_last_updated_text);
         pageLicenseText = 
(TextView)bottomContentContainer.findViewById(R.id.page_license_text);
+        readMoreList = 
(ListView)bottomContentContainer.findViewById(R.id.read_more_list);
+
+        funnel = new SuggestedPagesFunnel(app, pageTitle.getSite());
 
         // preload the display density, since it will be used in a lot of 
places
         displayDensity = 
parentFragment.getResources().getDisplayMetrics().density;
-
-        // get the screen height, using correct methods for different API 
versions
-        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.HONEYCOMB_MR2) {
-            Point size = new Point();
-            
parentFragment.getActivity().getWindowManager().getDefaultDisplay().getSize(size);
-            displayHeight = (int)(size.y / displayDensity);
-        } else {
-            displayHeight = (int)(parentFragment.getActivity()
-                    .getWindowManager().getDefaultDisplay().getHeight() / 
displayDensity);
-        }
-
-        if (isMainPage) {
-            
bottomContentContainer.findViewById(R.id.read_more_container).setVisibility(View.GONE);
-        } else {
-            requestReadMoreItems(activity.getLayoutInflater());
-        }
-
-        setupAttribution();
-
-        // give it a chance to redraw, and then see if it fits
-        bottomContentContainer.post(new Runnable() {
-            @Override
-            public void run() {
-                if (!parentFragment.isAdded()) {
-                    return;
-                }
-                layoutContent();
-            }
-        });
+        // hide ourselves by default
+        hide();
     }
 
     @Override
     public void onScrollChanged(int oldScrollY, int scrollY) {
+        if (bottomContentContainer.getVisibility() == View.GONE) {
+            return;
+        }
         int contentHeight = (int)(webView.getContentHeight() * displayDensity);
-        int screenHeight = (int)(displayHeight * displayDensity);
-        final int bottomOffsetExtra = 25;
-        int bottomOffset = contentHeight - scrollY - screenHeight
-                + (int)(bottomOffsetExtra * displayDensity);
+        int bottomOffset = contentHeight - scrollY - webView.getHeight();
         int bottomHeight = bottomContentContainer.getHeight();
         FrameLayout.LayoutParams params = (FrameLayout.LayoutParams) 
bottomContentContainer.getLayoutParams();
         if (bottomOffset > bottomHeight) {
             if (params.bottomMargin != -bottomHeight) {
                 params.bottomMargin = -bottomHeight;
                 bottomContentContainer.setLayoutParams(params);
+                bottomContentContainer.setVisibility(View.INVISIBLE);
             }
         } else {
             params.bottomMargin = -bottomOffset;
             bottomContentContainer.setLayoutParams(params);
+            if (bottomContentContainer.getVisibility() != View.VISIBLE) {
+                bottomContentContainer.setVisibility(View.VISIBLE);
+            }
+            if (!firstTimeShown && readMoreItems != null) {
+                firstTimeShown = true;
+                funnel.logSuggestionsShown(pageTitle, 
readMoreItems.getPageTitles());
+            }
+        }
+    }
+
+    @Override
+    public void onContentHeightChanged(int contentHeight) {
+        if (bottomContentContainer.getVisibility() != View.VISIBLE) {
+            return;
+        }
+        // trigger a manual scroll event to update our position
+        onScrollChanged(webView.getScrollY(), webView.getScrollY());
+    }
+
+    /**
+     * Hide the bottom content entirely.
+     * It can only be shown again by calling layoutContent()
+     */
+    public void hide() {
+        bottomContentContainer.setVisibility(View.GONE);
+    }
+
+    public void beginLayout() {
+        setupAttribution();
+        if 
(parentFragment.getFragment().getPage().getPageProperties().isMainPage()) {
+            
bottomContentContainer.findViewById(R.id.read_more_container).setVisibility(View.GONE);
+            layoutContent();
+        } else {
+            requestReadMoreItems(activity.getLayoutInflater());
         }
     }
 
     private void layoutContent() {
+        if (!parentFragment.isAdded()) {
+            return;
+        }
+        bottomContentContainer.setVisibility(View.INVISIBLE);
+        // keep trying until our layout has a height...
+        if (bottomContentContainer.getHeight() == 0) {
+            final int postDelay = 50;
+            bottomContentContainer.postDelayed(new Runnable() {
+                @Override
+                public void run() {
+                    layoutContent();
+                }
+            }, postDelay);
+            return;
+        }
+
+        // calculate the height of the listview, based on the number of items 
inside it.
+        ListAdapter adapter = readMoreList.getAdapter();
+        int listHeight = 0;
+        if (adapter != null && adapter.getCount() > 0) {
+            ViewGroup.LayoutParams params = readMoreList.getLayoutParams();
+            final int itemHeight = 
(int)parentFragment.getResources().getDimension(R.dimen.defaultListItemSize);
+            listHeight = adapter.getCount() * itemHeight
+                    + (readMoreList.getDividerHeight() * (adapter.getCount() - 
1));
+            params.height = listHeight;
+            readMoreList.setLayoutParams(params);
+        }
+
         // pad the bottom of the webview, to make room for ourselves
+        int totalHeight = bottomContentContainer.getHeight() + listHeight;
         JSONObject payload = new JSONObject();
         try {
-            payload.put("paddingBottom", 
(int)(bottomContentContainer.getHeight() / displayDensity));
+            payload.put("paddingBottom", (int)(totalHeight / displayDensity));
         } catch (JSONException e) {
             throw new RuntimeException(e);
         }
         bridge.sendMessage("setPaddingBottom", payload);
-
-        // and make it visible!
-        bottomContentContainer.setVisibility(View.VISIBLE);
-        // trigger a manual scroll event to update our position
-        onScrollChanged(webView.getScrollY(), webView.getScrollY());
+        // ^ sending the padding event will guarantee a ContentHeightChanged 
event to be triggered,
+        // which will update our margin based on the scroll offset, so we 
don't need to do it here.
     }
 
     private void setupAttribution() {
@@ -160,23 +203,28 @@
                 pageTitle.getPrefixedText()) {
             @Override
             public void onFinish(FullSearchResults results) {
-                setupReadMoreSection(bottomContentContainer, layoutInflater, 
results);
+                readMoreItems = results;
+                if (readMoreItems.getResults().size() > 0) {
+                    setupReadMoreSection(layoutInflater, readMoreItems);
+                }
+                layoutContent();
             }
 
             @Override
             public void onCatch(Throwable caught) {
-                super.onCatch(caught);
+                // Read More titles are expendable.
+                Log.w(TAG, "Error while fetching Read More titles.", caught);
+                // but lay out the bottom content anyway:
+                layoutContent();
             }
         }.execute();
     }
 
-    private void setupReadMoreSection(View parentView, LayoutInflater 
layoutInflater,
+    private void setupReadMoreSection(LayoutInflater layoutInflater,
                                       final 
FullSearchArticlesTask.FullSearchResults results) {
-        final SuggestedPagesFunnel funnel = new SuggestedPagesFunnel(app, 
pageTitle.getSite());
         final ReadMoreAdapter adapter = new ReadMoreAdapter(layoutInflater, 
results.getResults());
-        ListView list = (ListView) 
parentView.findViewById(R.id.read_more_list);
-        list.setAdapter(adapter);
-        list.setOnItemClickListener(new AdapterView.OnItemClickListener() {
+        readMoreList.setAdapter(adapter);
+        readMoreList.setOnItemClickListener(new 
AdapterView.OnItemClickListener() {
             @Override
             public void onItemClick(AdapterView<?> parent, View view, int 
position, long id) {
                 PageTitle title = (PageTitle) adapter.getItem(position);
diff --git a/wikipedia/src/main/java/org/wikipedia/views/ObservableWebView.java 
b/wikipedia/src/main/java/org/wikipedia/views/ObservableWebView.java
index cb5c118..817e9d8 100644
--- a/wikipedia/src/main/java/org/wikipedia/views/ObservableWebView.java
+++ b/wikipedia/src/main/java/org/wikipedia/views/ObservableWebView.java
@@ -14,6 +14,9 @@
     private List<OnScrollChangeListener> onScrollChangeListeners;
     private List<OnDownMotionEventListener> onDownMotionEventListeners;
     private List<OnUpOrCancelMotionEventListener> 
onUpOrCancelMotionEventListeners;
+    private List<OnContentHeightChangedListener> 
onContentHeightChangedListeners;
+
+    private int contentHeight = 0;
 
     public void addOnScrollChangeListener(OnScrollChangeListener 
onScrollChangeListener) {
         onScrollChangeListeners.add(onScrollChangeListener);
@@ -27,6 +30,10 @@
         onUpOrCancelMotionEventListeners.add(onUpOrCancelMotionEventListener);
     }
 
+    public void 
addOnContentHeightChangedListener(OnContentHeightChangedListener 
onContentHeightChangedListener) {
+        onContentHeightChangedListeners.add(onContentHeightChangedListener);
+    }
+
     public interface OnScrollChangeListener {
         void onScrollChanged(int oldScrollY, int scrollY);
     }
@@ -37,6 +44,10 @@
 
     public interface OnUpOrCancelMotionEventListener {
         void onUpOrCancelMotionEvent();
+    }
+
+    public interface OnContentHeightChangedListener {
+        void onContentHeightChanged(int contentHeight);
     }
 
     public ObservableWebView(Context context) {
@@ -63,6 +74,7 @@
         onScrollChangeListeners = new ArrayList<OnScrollChangeListener>();
         onDownMotionEventListeners = new 
ArrayList<OnDownMotionEventListener>();
         onUpOrCancelMotionEventListeners = new 
ArrayList<OnUpOrCancelMotionEventListener>();
+        onContentHeightChangedListeners = new 
ArrayList<OnContentHeightChangedListener>();
     }
 
     @Override
@@ -102,7 +114,12 @@
         if (isInEditMode()) {
             return;
         }
+        if (contentHeight != getContentHeight()) {
+            contentHeight = getContentHeight();
+            for (OnContentHeightChangedListener listener : 
onContentHeightChangedListeners) {
+                listener.onContentHeightChanged(contentHeight);
+            }
+        }
         WikipediaApp.getInstance().getBus().post(new WebViewInvalidateEvent());
     }
-
 }

-- 
To view, visit https://gerrit.wikimedia.org/r/174995
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie5b02c0afca51f44ff444f1759847945fb72aba6
Gerrit-PatchSet: 6
Gerrit-Project: apps/android/wikipedia
Gerrit-Branch: master
Gerrit-Owner: Dbrant <[email protected]>
Gerrit-Reviewer: BearND <[email protected]>
Gerrit-Reviewer: Brion VIBBER <[email protected]>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to