Skip to content
This repository has been archived by the owner on Jul 18, 2018. It is now read-only.

Commit

Permalink
📰 Merge the more button and progress item
Browse files Browse the repository at this point in the history 
Makes handling the state of these items easier, in
preparation for a more visible treatment of the loading
state.

The new design also fixes an issue where the progress
indicator was appearing below the fold, thus being invisible
to the user.

Preview: https://photos.app.goo.gl/SbEYohBYPHMcWooh2
Bug: 663376,762932
Change-Id: I0fd7f74ea2a45eec782cce8f52933f40d8d928de
Reviewed-on: https://chromium-review.googlesource.com/667103
Commit-Queue: Nicolas Dossou-Gbété <dgn@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Reviewed-by: Michael van Ouwerkerk <mvanouwerkerk@chromium.org>
Reviewed-by: Peter Conn <peconn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503125}
  • Loading branch information
Nicolas Dossou-gbete authored and Commit Bot committed Sep 20, 2017
1 parent a07a41c commit 07ea99d
Show file tree
Hide file tree
Showing 8 changed files with 144 additions and 97 deletions.
24 changes: 14 additions & 10 deletions chrome/android/java/res/layout/content_suggestions_action_card_modern.xml
View file
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:orientation="vertical"
android:layout_width="match_parent"
android:layout_height="wrap_content"
style="@style/SuggestionCardModern"
android:background="@null"
android:foreground="@null"
android:padding="8dp">
<FrameLayout
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:tools="http://schemas.android.com/tools"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:padding="8dp">

<Button
style="@style/SuggestionCardActionModern"
Expand All @@ -18,4 +15,11 @@
android:layout_margin="0dp"
android:text="@string/more"
tools:text="Action" />
</LinearLayout>

<org.chromium.chrome.browser.ntp.cards.ProgressIndicatorView
android:id="@+id/progress_indicator"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_gravity="center_vertical" />

</FrameLayout>
36 changes: 20 additions & 16 deletions chrome/android/java/res/layout/new_tab_page_action_card.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,32 @@
bigger one related to the peeking behaviour. This card will only have
buttons, that have themselves some padding. We just stay within the
material specifications. -->
<LinearLayout
<FrameLayout
xmlns:android="http://schemas.android.com/apk/res/android"
xmlns:chrome="http://schemas.android.com/apk/res-auto"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:orientation="vertical"
android:padding="8dp"
android:background="@drawable/card_single">


<!-- Using some standard metrics (minWidth, paddings) to override the
special ones used in ButtonCompatBorderless -->
<Button
android:id="@+id/action_button"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:gravity="center_vertical|center_horizontal"
android:minWidth="0dp"
android:paddingEnd="16dp"
android:paddingStart="16dp"
android:text="@string/more"
android:textAllCaps="true"
android:textColor="@color/light_active_color"
style="@style/ButtonCompatBorderless" />
</LinearLayout>
android:id="@+id/action_button"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:gravity="center_vertical"
android:minWidth="0dp"
android:paddingEnd="16dp"
android:paddingStart="16dp"
android:text="@string/more"
android:textAllCaps="true"
android:textColor="@color/light_active_color"
style="@style/ButtonCompatBorderless" />

<org.chromium.chrome.browser.ntp.cards.ProgressIndicatorView
android:id="@+id/progress_indicator"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_gravity="center_vertical"
/>
</FrameLayout>
114 changes: 81 additions & 33 deletions chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ActionItem.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@

package org.chromium.chrome.browser.ntp.cards;

import android.support.annotation.IntDef;
import android.support.annotation.LayoutRes;
import android.view.View;
import android.widget.Button;

import org.chromium.base.VisibleForTesting;
import org.chromium.chrome.R;
Expand All @@ -19,25 +21,35 @@
import org.chromium.chrome.browser.util.FeatureUtilities;
import org.chromium.chrome.browser.widget.displaystyle.UiConfig;

import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;

/**
* Item that allows the user to perform an action on the NTP.
* Item that allows the user to perform an action on the NTP. Depending on its state, it can also
* show a progress indicator over the same space. See {@link State}.
*/
public class ActionItem extends OptionalLeaf {
@Retention(RetentionPolicy.SOURCE)
@IntDef({State.HIDDEN, State.BUTTON, State.LOADING})
public @interface State {
int HIDDEN = 0;
int BUTTON = 1;
int LOADING = 2;
}

private final SuggestionsCategoryInfo mCategoryInfo;
private final SuggestionsSection mParentSection;
private final SuggestionsRanker mSuggestionsRanker;

private boolean mImpressionTracked;
private int mPerSectionRank = -1;
private boolean mEnabled;
private @State int mState = State.HIDDEN;

public ActionItem(SuggestionsSection section, SuggestionsRanker ranker) {
mCategoryInfo = section.getCategoryInfo();
mParentSection = section;
mSuggestionsRanker = ranker;
mEnabled = true;
setVisibilityInternal(
mCategoryInfo.getAdditionalAction() != ContentSuggestionsAdditionalAction.NONE);
updateState(State.BUTTON); // Also updates the visibility of the item.
}

@Override
Expand All @@ -53,7 +65,18 @@ protected void onBindViewHolder(NewTabPageViewHolder holder) {

@Override
public void visitOptionalItem(NodeVisitor visitor) {
visitor.visitActionItem(mCategoryInfo.getAdditionalAction());
switch (mState) {
case State.BUTTON:
visitor.visitActionItem(mCategoryInfo.getAdditionalAction());
break;
case State.LOADING:
visitor.visitProgressItem();
break;
case State.HIDDEN:
// If state is HIDDEN, itemCount should be 0 and this method should not be called.
default:
throw new IllegalStateException();
}
}

@CategoryInt
Expand All @@ -69,9 +92,30 @@ public int getPerSectionRank() {
return mPerSectionRank;
}

public void updateState(@State int newState) {
if (newState == State.BUTTON
&& mCategoryInfo.getAdditionalAction() == ContentSuggestionsAdditionalAction.NONE) {
newState = State.HIDDEN;
}

if (mState == newState) return;
mState = newState;

boolean newVisibility = (newState != State.HIDDEN);
if (isVisible() != newVisibility) {
setVisibilityInternal(newVisibility);
} else {
notifyItemChanged(0, (viewHolder) -> ((ViewHolder) viewHolder).setState(mState));
}
}

public @State int getState() {
return mState;
}

@VisibleForTesting
void performAction(SuggestionsUiDelegate uiDelegate) {
if (!mEnabled) return;
assert mState == State.BUTTON;

uiDelegate.getEventReporter().onMoreButtonClicked(this);

Expand All @@ -95,46 +139,33 @@ void performAction(SuggestionsUiDelegate uiDelegate) {
}
}

/** Used to enable/disable the action of this item. */
public void setEnabled(boolean enabled) {
mEnabled = enabled;
}

public void setVisible(boolean visible) {
setVisibilityInternal(visible);
}

/** ViewHolder associated to {@link ItemViewType#ACTION}. */
public static class ViewHolder extends CardViewHolder implements ContextMenuManager.Delegate {
private ActionItem mActionListItem;
private final ProgressIndicatorView mProgressIndicator;
private final Button mButton;

public ViewHolder(final SuggestionsRecyclerView recyclerView,
ContextMenuManager contextMenuManager, final SuggestionsUiDelegate uiDelegate,
public ViewHolder(SuggestionsRecyclerView recyclerView,
ContextMenuManager contextMenuManager, SuggestionsUiDelegate uiDelegate,
UiConfig uiConfig) {
super(getLayout(), recyclerView, uiConfig, contextMenuManager);

itemView.findViewById(R.id.action_button)
.setOnClickListener(new View.OnClickListener() {
@Override
public void onClick(View v) {
mActionListItem.performAction(uiDelegate);
}
});

new ImpressionTracker(itemView, new ImpressionTracker.Listener() {
@Override
public void onImpression() {
if (mActionListItem != null && !mActionListItem.mImpressionTracked) {
mActionListItem.mImpressionTracked = true;
uiDelegate.getEventReporter().onMoreButtonShown(mActionListItem);
}
mProgressIndicator = itemView.findViewById(R.id.progress_indicator);
mButton = itemView.findViewById(R.id.action_button);
mButton.setOnClickListener(v -> mActionListItem.performAction(uiDelegate));

new ImpressionTracker(itemView, () -> {
if (mActionListItem != null && !mActionListItem.mImpressionTracked) {
mActionListItem.mImpressionTracked = true;
uiDelegate.getEventReporter().onMoreButtonShown(mActionListItem);
}
});
}

public void onBindViewHolder(ActionItem item) {
super.onBindViewHolder();
mActionListItem = item;
setState(item.mState);
}

@LayoutRes
Expand All @@ -143,5 +174,22 @@ private static int getLayout() {
? R.layout.content_suggestions_action_card_modern
: R.layout.new_tab_page_action_card;
}

private void setState(@State int state) {
assert state != State.HIDDEN;

// When hiding children, we keep them invisible rather than GONE to make sure the
// overall height of view does not change, to make transitions look better.
if (state == State.BUTTON) {
mButton.setVisibility(View.VISIBLE);
mProgressIndicator.hide(/* keepSpace = */ true);
} else if (state == State.LOADING) {
mButton.setVisibility(View.INVISIBLE);
mProgressIndicator.show();
} else {
// Not even HIDDEN is supported as the item should not be able to receive updates.
assert false : "ActionViewHolder got notified of an unsupported state: " + state;
}
}
}
}
17 changes: 9 additions & 8 deletions chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/ProgressIndicatorView.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,9 @@ public class ProgressIndicatorView extends ImageView {
public ProgressIndicatorView(Context context, AttributeSet attrs) {
super(context, attrs);

mShowSpinnerRunnable = new Runnable() {
@Override
public void run() {
mPostedCallback = false;
show();
}
mShowSpinnerRunnable = () -> {
mPostedCallback = false;
show();
};

mProgressDrawable = new MaterialProgressDrawable(getContext(), this);
Expand All @@ -50,7 +47,7 @@ public void run() {
mProgressDrawable.setAlpha(255);
mProgressDrawable.setColorSchemeColors(
ApiCompatibilityUtils.getColor(resources, R.color.light_active_color));
mProgressDrawable.updateSizes(MaterialProgressDrawable.LARGE);
mProgressDrawable.updateSizes(MaterialProgressDrawable.DEFAULT);
setImageDrawable(mProgressDrawable);

hide();
Expand All @@ -76,10 +73,14 @@ public void show() {
}

public void hide() {
hide(false);
}

public void hide(boolean keepSpace) {
mProgressDrawable.stop();
removeCallbacks(mShowSpinnerRunnable);
mPostedCallback = false;
setVisibility(View.GONE);
setVisibility(keepSpace ? View.INVISIBLE : View.GONE);
}

public void showDelayed() {
Expand Down
26 changes: 6 additions & 20 deletions chrome/android/java/src/org/chromium/chrome/browser/ntp/cards/SuggestionsSection.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ public class SuggestionsSection extends InnerNode {
private final SuggestionsList mSuggestionsList;
private final StatusItem mStatus;
private final ActionItem mMoreButton;
private final ProgressItem mProgressIndicator;

/**
* Keeps track of how many suggestions have been seen by the user so that we replace only
Expand Down Expand Up @@ -105,11 +104,10 @@ public SuggestionsSection(Delegate delegate, SuggestionsUiDelegate uiDelegate,
mStatus = StatusItem.createNoSuggestionsItem(info);
}
mMoreButton = new ActionItem(this, ranker);
mProgressIndicator = new ProgressItem();
if (useModern) {
addChildren(mHeader, mSuggestionsList, mMoreButton, mProgressIndicator);
addChildren(mHeader, mSuggestionsList, mMoreButton);
} else {
addChildren(mHeader, mSuggestionsList, mStatus, mMoreButton, mProgressIndicator);
addChildren(mHeader, mSuggestionsList, mStatus, mMoreButton);
}

mOfflineModelObserver = new OfflineModelObserver(offlinePageBridge);
Expand Down Expand Up @@ -511,23 +509,14 @@ private boolean canUpdateSuggestions() {
}
/** Fetches additional suggestions only for this section. */
public void fetchSuggestions() {
// We want to disable the action item while we are fetching suggestions in order to
// avoid fetching the same suggestions twice. See crbug.com/739648.
mMoreButton.setEnabled(false);
mMoreButton.setVisible(false);
mMoreButton.updateState(ActionItem.State.LOADING);
mSuggestionsSource.fetchSuggestions(
mCategoryInfo.getCategory(), getDisplayedSuggestionIds(), additionalSuggestions -> {
if (!isAttached()) return; // The section has been dismissed.

mProgressIndicator.setVisible(false);

appendSuggestions(additionalSuggestions, /* keepSectionSize = */ false);

mMoreButton.setEnabled(true);
mMoreButton.setVisible(true);
mMoreButton.updateState(ActionItem.State.BUTTON);
});

mProgressIndicator.setVisible(true);
}

/**
Expand All @@ -545,7 +534,8 @@ public void setStatus(@CategoryStatus int status) {
Log.d(TAG, "setStatus: unavailable status, cleared suggestions.");
}

mProgressIndicator.setVisible(SnippetsBridge.isCategoryLoading(status));
mMoreButton.updateState(SnippetsBridge.isCategoryLoading(status) ? ActionItem.State.LOADING
: ActionItem.State.BUTTON);
}

/** Clears the suggestions and related data, resetting the state of the section. */
Expand Down Expand Up @@ -600,10 +590,6 @@ public String getHeaderText() {
return mHeader.getHeaderText();
}

ProgressItem getProgressItemForTesting() {
return mProgressIndicator;
}

ActionItem getActionItemForTesting() {
return mMoreButton;
}
Expand Down
Loading

0 comments on commit 07ea99d

Please sign in to comment.