Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async media: Scroll to failed post from error notifications #6424

Merged
merged 9 commits into from
Jul 31, 2017
Merged

Async media: Scroll to failed post from error notifications #6424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e70fc2a
Pass the post when opening post list from error notification
aforcier Jul 28, 2017
3bf6a80
Scroll list to failed post when opening error notification
aforcier Jul 28, 2017
d108595
Remove post list error dialog (triggered from error notifications)
aforcier Jul 28, 2017
56d8d25
Scroll target post to the top of the list
aforcier Jul 28, 2017
13de017
Merge branch 'issue/6386-async-design-tweaks' into feature/async-erro…
aforcier Jul 31, 2017
08c6954
Smooth scroll to failed post
aforcier Jul 31, 2017
711f21e
Offset scroll-to-post to show part of above post
aforcier Jul 31, 2017
3e0b6f6
Rename target post id constant
aforcier Jul 31, 2017
fece2bf
Show toast when tapping error notification for unknown post
aforcier Jul 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 15 additions & 31 deletions WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListActivity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
package org.wordpress.android.ui.posts;

import android.app.AlertDialog;
import android.content.Intent;
import android.os.Bundle;
import android.support.v7.app.ActionBar;
import android.support.v7.app.AppCompatActivity;
import android.support.v7.widget.Toolbar;
import android.text.TextUtils;
import android.view.MenuItem;

import org.wordpress.android.R;
import org.wordpress.android.WordPress;
import org.wordpress.android.fluxc.model.PostModel;
import org.wordpress.android.fluxc.model.SiteModel;
import org.wordpress.android.fluxc.store.PostStore;
import org.wordpress.android.fluxc.store.SiteStore;
import org.wordpress.android.ui.ActivityId;
import org.wordpress.android.ui.RequestCodes;
Expand All @@ -21,13 +21,14 @@

public class PostsListActivity extends AppCompatActivity {
public static final String EXTRA_VIEW_PAGES = "viewPages";
public static final String EXTRA_ERROR_MSG = "errorMessage";
public static final String EXTRA_TARGET_POST_LOCAL_ID = "targetPostLocalId";

private boolean mIsPage = false;
private PostsListFragment mPostList;
private SiteModel mSite;

@Inject SiteStore mSiteStore;
@Inject PostStore mPostStore;

@Override
public void onCreate(Bundle savedInstanceState) {
Expand Down Expand Up @@ -89,10 +90,19 @@ private void handleIntent(Intent intent) {
actionBar.setDisplayHomeAsUpEnabled(true);
}

PostModel targetPost = null;
int targetPostId = intent.getIntExtra(EXTRA_TARGET_POST_LOCAL_ID, 0);
if (targetPostId > 0) {
targetPost = mPostStore.getPostByLocalPostId(intent.getIntExtra(EXTRA_TARGET_POST_LOCAL_ID, 0));
if (targetPost == null) {
ToastUtils.showToast(this, R.string.error_post_does_not_exist);
}
}

mPostList = (PostsListFragment) getFragmentManager().findFragmentByTag(PostsListFragment.TAG);
if (mPostList == null || siteHasChanged || pageHasChanged) {
if (mPostList == null || siteHasChanged || pageHasChanged || targetPost != null) {
PostsListFragment oldFragment = mPostList;
mPostList = PostsListFragment.newInstance(mSite, mIsPage);
mPostList = PostsListFragment.newInstance(mSite, mIsPage, targetPost);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PostsListFragment.newInstance() only uses targetPost to obtain it's id and set a parcelable argument. Later on that id will be properly used to retrieve the actual PostModel from the db when it is actually needed.
Also, the only reason why PostStore is imported and injected here is to be able to get the PostModel.

Why don't we just remove the use of mPostStore in PostsListActivity altogether, and pass the ID here as well?

This would also be in line with the decision taken on trying to avoid this kind of PostModel passing around because of potential OOM problems, like in this PR here #5856

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we're passing the PostModel directly to a method, it's not being parceled, so #5856 shouldn't apply.

I decided to use the model for two reasons:

  1. We won't create a new instance of the PostsListFragment if the post has since been deleted (since it would have a non-0 id, but the PostModel obtained from the store would be null)
  2. This avoids PostsListFragment.newInstance() taking an int id argument, which is error-prone and the main case for using models over ids

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we're passing the PostModel directly to a method, it's not being parceled, so #5856 shouldn't apply.

Forgive me, I wasn't clear: I was trying to acknowledge how the same treatment to this case is given within the fragment by using the ID instead of the PostModel. I should have said something like "it's great to see we're using this in the same way here".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having discussed these 2 points in Slack, and acknowledging point 1 is better kept as is as per the advantage of keeping fragments, and leaving point 2 for a longer discussion elsewhere, here's one minor thing users could benefit from:

If the user taps on an error notification for which the referred PostModel doesn’t exist anymore (i.e. has been deleted) we should do something else then, maybe show a Toast or something, indicating the Post wasn’t found.

To test:

  1. start draft
  2. make it fail
  3. delete draft
  4. tap on notification

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added toast in fece2bf.

if (oldFragment == null) {
getFragmentManager().beginTransaction()
.add(R.id.post_list_container, mPostList, PostsListFragment.TAG)
Expand All @@ -103,8 +113,6 @@ private void handleIntent(Intent intent) {
.commit();
}
}

showErrorDialogIfNeeded(intent.getExtras());
}

@Override
Expand All @@ -122,30 +130,6 @@ protected void onActivityResult(int requestCode, int resultCode, Intent data) {
}
}

/**
* intent extras will contain error info if this activity was started from an
* upload error notification
*/
private void showErrorDialogIfNeeded(Bundle extras) {
if (extras == null || !extras.containsKey(EXTRA_ERROR_MSG) || isFinishing()) {
return;
}

final String errorMessage = extras.getString(EXTRA_ERROR_MSG);

if (TextUtils.isEmpty(errorMessage)) {
return;
}

AlertDialog.Builder builder = new AlertDialog.Builder(this);
builder.setTitle(getResources().getText(R.string.error))
.setMessage(errorMessage)
.setPositiveButton(android.R.string.ok, null)
.setCancelable(true);

builder.create().show();
}

public boolean isRefreshing() {
return mPostList.isRefreshing();
}
Expand Down
47 changes: 44 additions & 3 deletions WordPress/src/main/java/org/wordpress/android/ui/posts/PostsListFragment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@
import android.support.design.widget.Snackbar;
import android.support.v7.app.AlertDialog;
import android.support.v7.widget.LinearLayoutManager;
import android.support.v7.widget.LinearSmoothScroller;
import android.support.v7.widget.RecyclerView;
import android.util.TypedValue;
import android.view.LayoutInflater;
import android.view.View;
import android.view.ViewGroup;
Expand Down Expand Up @@ -85,6 +87,7 @@ public class PostsListFragment extends Fragment

private boolean mCanLoadMorePosts = true;
private boolean mIsPage;
private PostModel mTargetPost;
private boolean mIsFetchingPosts;
private boolean mShouldCancelPendingDraftNotification = false;
private int mPostIdForPostToBeDeleted = 0;
Expand All @@ -97,11 +100,14 @@ public class PostsListFragment extends Fragment
@Inject PostStore mPostStore;
@Inject Dispatcher mDispatcher;

public static PostsListFragment newInstance(SiteModel site, boolean isPage) {
public static PostsListFragment newInstance(SiteModel site, boolean isPage, @Nullable PostModel targetPost) {
PostsListFragment fragment = new PostsListFragment();
Bundle bundle = new Bundle();
bundle.putSerializable(WordPress.SITE, site);
bundle.putBoolean(PostsListActivity.EXTRA_VIEW_PAGES, isPage);
if (targetPost != null) {
bundle.putInt(PostsListActivity.EXTRA_TARGET_POST_LOCAL_ID, targetPost.getId());
}
fragment.setArguments(bundle);
return fragment;
}
Expand Down Expand Up @@ -130,18 +136,23 @@ private void updateSiteOrFinishActivity(Bundle savedInstanceState) {
if (getArguments() != null) {
mSite = (SiteModel) getArguments().getSerializable(WordPress.SITE);
mIsPage = getArguments().getBoolean(PostsListActivity.EXTRA_VIEW_PAGES);
mTargetPost = mPostStore.getPostByLocalPostId(
getArguments().getInt(PostsListActivity.EXTRA_TARGET_POST_LOCAL_ID));
} else {
mSite = (SiteModel) getActivity().getIntent().getSerializableExtra(WordPress.SITE);
mIsPage = getActivity().getIntent().getBooleanExtra(PostsListActivity.EXTRA_VIEW_PAGES, false);
mTargetPost = mPostStore.getPostByLocalPostId(
getActivity().getIntent().getIntExtra(PostsListActivity.EXTRA_TARGET_POST_LOCAL_ID, 0));
}
} else {
mSite = (SiteModel) savedInstanceState.getSerializable(WordPress.SITE);
mIsPage = savedInstanceState.getBoolean(PostsListActivity.EXTRA_VIEW_PAGES);
mTargetPost = mPostStore.getPostByLocalPostId(
savedInstanceState.getInt(PostsListActivity.EXTRA_TARGET_POST_LOCAL_ID));
}

if (mSite == null) {
ToastUtils.showToast(getActivity(), R.string.blog_not_found,
ToastUtils.Duration.SHORT);
ToastUtils.showToast(getActivity(), R.string.blog_not_found, ToastUtils.Duration.SHORT);
getActivity().finish();
}
}
Expand Down Expand Up @@ -494,6 +505,36 @@ public void onPostsLoaded(int postCount) {
} else if (postCount > 0) {
hideEmptyView();
}

// If the activity was given a target post, and this is the first time posts are loaded, scroll to that post
if (mTargetPost != null) {
if (mPostsListAdapter != null) {
final int position = mPostsListAdapter.getPositionForPost(mTargetPost);
if (position > -1) {
RecyclerView.SmoothScroller smoothScroller = new LinearSmoothScroller(getActivity()) {
private static final int SCROLL_OFFSET_DP = 23;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment here explaining why we are moving this offset? maybe something like // we're showing just a bit of the item above to hint the user they can navigate up (feel free to re-write to something better worded than this)


@Override
protected int getVerticalSnapPreference() {
return LinearSmoothScroller.SNAP_TO_START;
}

@Override
public int calculateDtToFit(int viewStart, int viewEnd, int boxStart, int boxEnd,
int snapPreference) {
// Assume SNAP_TO_START, and offset the scroll, so the bottom of the above post shows
int offsetPx = (int) TypedValue.applyDimension(
TypedValue.COMPLEX_UNIT_DIP, SCROLL_OFFSET_DP, getResources().getDisplayMetrics());
return boxStart - viewStart + offsetPx;
}
};

smoothScroller.setTargetPosition(position);
mRecyclerView.getLayoutManager().startSmoothScroll(smoothScroller);
}
}
mTargetPost = null;
}
}

/*
Expand Down
8 changes: 6 additions & 2 deletions WordPress/src/main/java/org/wordpress/android/ui/posts/adapters/PostsListAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,10 @@ public void onAnimationEnd(Animator animation) {
animOut.start();
}

public int getPositionForPost(PostModel post) {
return PostUtils.indexOfPostInList(post, mPosts);
}

public void loadPosts(LoadMode mode) {
if (mIsLoadingPosts) {
AppLog.d(AppLog.T.POSTS, "post adapter > already loading posts");
Expand All @@ -642,7 +646,7 @@ public void loadPosts(LoadMode mode) {

public void updateProgressForPost(@NonNull PostModel post) {
if (mRecyclerView != null) {
int position = PostUtils.indexOfPostInList(post, mPosts);
int position = getPositionForPost(post);
if (position > -1) {
RecyclerView.ViewHolder viewHolder = mRecyclerView.findViewHolderForAdapterPosition(position);
if (viewHolder instanceof PostViewHolder) {
Expand All @@ -661,7 +665,7 @@ public void updateProgressForPost(@NonNull PostModel post) {
public void hidePost(PostModel post) {
mHiddenPosts.add(post);

int position = PostUtils.indexOfPostInList(post, mPosts);
int position = getPositionForPost(post);
if (position > -1) {
mPosts.remove(position);
if (mPosts.size() > 0) {
Expand Down
2 changes: 1 addition & 1 deletion WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadNotifier.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ void updateNotificationError(PostModel post, SiteModel site, String errorMessage
notificationIntent.addFlags(Intent.FLAG_ACTIVITY_NEW_TASK);
notificationIntent.putExtra(WordPress.SITE, site);
notificationIntent.putExtra(PostsListActivity.EXTRA_VIEW_PAGES, post.isPage());
notificationIntent.putExtra(PostsListActivity.EXTRA_ERROR_MSG, errorMessage);
notificationIntent.putExtra(PostsListActivity.EXTRA_TARGET_POST_LOCAL_ID, post.getId());
notificationIntent.setAction(String.valueOf(notificationId));

PendingIntent pendingIntent = PendingIntent.getActivity(mContext,
Expand Down
1 change: 1 addition & 0 deletions WordPress/src/main/res/values/strings.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1163,6 +1163,7 @@
<string name="error_media_save">Unable to save media</string>
<string name="error_media_canceled">Uploading media were canceled</string>
<string name="error_media_recover">We were unable to upload this post\'s media. Please edit the post to try again.</string>
<string name="error_post_does_not_exist">This post no longer exists</string>
<string name="error_blog_hidden">This blog is hidden and couldn\'t be loaded. Enable it again in settings and try again.</string>
<string name="fatal_db_error">An error occurred while creating the app database. Try reinstalling the app.</string>
<string name="error_copy_to_clipboard">An error occurred while copying text to clipboard</string>
Expand Down