Issue/4727 notifs detail view horizontal navigation #4796

mzorz · 2016-11-17T00:13:01Z

This PR implements 2 ways to horizontally navigate through Notifications detail:

using swipe left/right (by means of a ViewPager control)
touching the left/right arrows now available on the action bar

To test:

log in to the WP Android app with your account
go to the Notifications tab (rightmost tab)
you'll be presented with the notifications list.
tap on any item - you'll be presented with the item's detail screen
swipe left-right to navigate.
Note that you'll be able to navigate left-right items as they are listed top-bottom in the notifications list correspondingly. That means if you tapped on the first item in the list, you won't be able to swipe left on that item (as you can't go any further up while already looking at the top of the list). Same thing for the last item in the list.
Also, if you tap on any of the filters (All, Comments, unread, etc.) you'd be able to navigate the list horizontally accordingly - that is, items are filtered there as well.

See video here https://cloudup.com/cBZOd5sFriD


          first implementation of viewpager for lateral notification detail nav…

…igation


          change action bar title as you navigate the Notes list left and right…

… in the viewpager


          marking as read each note as they are shown into the viewpager curren…

…t item view


          filtered lists work now on horizontal navigation


          Merge branch 'develop' into issue/4727-unotifs-up-down-navigatino-arrows


          fixed filter passing for filtered notification list


          Merge branch 'develop' into issue/4727-unotifs-up-down-navigatino-arrows


          added left/right arrows navigation


          changed getFragmentManager for getChildFragmentManager call to suppor…

…t nested fragments for Comments structure in ViewPager


          removed unused import


          removed unused imports

daniloercoli · 2016-11-17T16:49:09Z

I found an issue where the details screen can be blank upon screen rotation.
Steps to repro:

Go to notifications
Tap on a note and open the details screen
Rotate the device

daniloercoli · 2016-11-17T16:54:00Z

Hey @nbradbury - Since you've worked with the ViewPager in the past :) , would you mind a quick look to this PR and tell us if you see something to double check?

nbradbury · 2016-11-17T18:24:34Z

Howdy @daniloercoli, sure I'll take a look later today. In the meantime, now that I see this I think we should reconsider the left/right icons in the toolbar.

This isn't the only area of the app that uses a ViewPager (the reader being the obvious other place), and I think we'll want a consistent UI for letting the user know they can move between pages. I don't see the left/right icons working well in the reader since there are already a couple of toolbar icons there.

I think what we really want is to simply let the user know they can swipe between pages - once they know they can do it, it's much easier than tapping an icon. I think the USA Today app handles this nicely. The first few times you open an article, a snackbar-ish message animates in at the bottom of the screen letting you know you can swipe. Once you swipe, it animates back out.

What do we think about that approach instead?

daniloercoli · 2016-11-17T21:22:25Z

I agree with you @nbradbury, and the proposed solution looks much nicer.
IMO Left/Right buttons in the toolbar seems off-platform.

Curious to hear what @drw158 thinks about it.

/cc @kwonye

nbradbury · 2016-11-17T22:43:00Z

Hey @nbradbury - Since you've worked with the ViewPager in the past :) , would you mind a quick look to this PR and tell us if you see something to double check?

It looks like this ViewPager adapter is based on the one I coded for the reader a while ago, and that has been pretty reliable. I do recommend adding this change, though, to address this Android bug.

Note, also, that the use of getChildFragmentManager() here will need to be re-thought. That call was added in API 17 and our minSdkVersion is 16.

@nbradbury


          added workaorund for Android bug by suggestion of @nbradbury

mzorz · 2016-11-18T12:15:49Z

Note, also, that the use of getChildFragmentManager() here will need to be re-thought. That call was added in API 17 and our minSdkVersion is 16.

That will be a tough one. I needed to use getChildFragmentManager because otherwise this call here

WordPress-Android/WordPress/src/main/java/org/wordpress/android/ui/comments/CommentDetailFragment.java

Line 1062 in 9289ed1

    
           fragmentTransaction.replace(R.id.comment_content_container, mNotificationsDetailListFragment);

would replace fragments on the first available fragment in the ViewPager adapter, instead of doing the replacement in the correct page (for instance, in the next page as the viewpager creates and pre-loads offscreen), which led to very undesirable (and unpredictable) results such as blank pages or a "next" item actually being displayed (because the content of such item would be replaced on the first View found in the previous object). I actually spent more time trying to figure out why that was happening (and finding getChildFragmentManager addresses exactly that kind of situation) than implementing the feature itself.

That said, I believe a workaround could be using tags for fragments (so we can search for them individually as opposed to what happens with xml ids), but we'd have to take care of fragment states and pages artificially, outside the fragment manager to circumvent this problem.

Any thoughts are appreciated

mzorz · 2016-11-18T12:23:32Z

let's blame the jet lag - looking a bit closer this should be in in the support libraries. Let me get back to this later.

mzorz · 2016-11-18T12:28:48Z

re: not using the left/right arrows on the action bar, and instead using the toast/snackbar to inform the user they have the option to swipe to navigate through notifications, I love that.

I'd definitely go that way unless @kwonye or @drw158 feel differently

mzorz · 2016-11-18T12:35:57Z

Also, if we go the way @nbradbury suggests, maybe we'd like to just keep the EDIT button where it was? (check this #4782) @kwonye @drw158

daniloercoli · 2016-11-18T13:42:02Z

My memory is fuzzy, but I remember we were using ChildFragments in Stats, and switched to using "normal" Android Fragments and tags.

mzorz · 2016-11-18T14:12:38Z

Just opened a new PR for discussing the support library usage and related topics here #4806 .

davewhitley · 2016-11-19T14:44:50Z

Using a notice to inform the user that they can swipe seems like a great idea. No need for the navigation buttons in that case.

Seems more natural for iOS to have the navigation buttons, maybe that's just because it's a standard set by Mail.app. I'm ok that Android and iOS are different in this area.

mzorz · 2016-11-19T14:51:06Z

Thank you @drw158 ! Going that way then. 👍


          assigning a different id to the comment detail list fragment containe…

…r, so we are sure to change the right fragment in fragment transaction without the need to use the ChildFragmentManager which is only available on API level 17+


          took snackbar action off, and now snackbar is not presented anymore o…

…nce user does first use swipe

mzorz · 2016-11-21T22:27:14Z

Thank you @nbradbury ! made these changes here 61b4f76

nbradbury · 2016-11-22T01:25:03Z

WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsDetailActivity.java

+        super.onStart();
+        //if the user hasn't used swipe yet, hint the user they can navigate through notifications detail
+        //using swipe on the ViewPager
+        if (!AppPrefs.isSwipeToNavigateShown()) {


Are there situations where only one note is shown, so there's no swiping? If so, we should make sure not to show this snackbar in those situations.

oops. You're so right! here's the fix ccdb2a9


          fixed showing snackbar for swiping only if swipe will be available

daniloercoli · 2016-11-22T11:21:37Z

WordPress/src/main/java/org/wordpress/android/ui/comments/CommentDetailFragment.java

        getFragmentManager().invalidateOptionsMenu();
    }

+    private void setIdForCommentContainer(){
+        if (mComment != null && mCommentContentLayout != null) {
+            mCommentContentLayout.setId((int)mComment.commentID);


Note that a comment could have a value for the ID field that overflows MAX_INT.
Can we use setTag and getTag to avoid potential issues where 2 comments have IDs > MAX_INT.

A note on using tags vs IDs, the fragment transaction needs the container id where to add the fragment to, and there doesn't seem to be a way to replace the container contents without knowing its id in FragmentManager (though this artifact we're making by having a different ID for each viewpager page's container is handled seamlessly by the manager returned by getChildFragmentManager, which is the main purpose of its existence, i.e. handling nested fragments). Setting a tag to find it by tag later then having to set the id anyways seems dumb, so what I did is I'm generating an id with a base id plus the Note position index in the notes adapter array to make probability of id collision low (while it is theoretically possible, I don't think it'll ever happen in practice, as we'd have to have a list of more than 65535 items in the notifications list...)

BTW there is also an API to generate a unique id on runtime, but it's also only available since API level 17, which is what we were trying to avoid in the first place 😁

Commit addressing all this here 8da2cea

daniloercoli · 2016-11-22T11:21:37Z

WordPress/src/main/java/org/wordpress/android/ui/notifications/NotificationsDetailActivity.java

 import android.support.v7.app.ActionBar;
 import android.support.v7.app.AppCompatActivity;
 import android.text.TextUtils;
 import android.view.MenuItem;
+import android.view.View;


Niptick: Unused.

No shame in nitpicks! 😁

daniloercoli · 2016-11-22T11:21:37Z

WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderViewPagerTransformer.java

@@ -11,16 +11,16 @@
 * zoom & depth are based on examples here, with many fixes and simplifications:
 * http://developer.android.com/training/animation/screen-slide.html#pagetransformer
 */
-class ReaderViewPagerTransformer implements ViewPager.PageTransformer {
-    enum TransformType {
+public class ReaderViewPagerTransformer implements ViewPager.PageTransformer {


We should move this class to Widgets (or Utils) and call it WPViewPagerTransformer.

Addressed in 382808f

daniloercoli · 2016-11-22T11:21:37Z

WordPress/src/main/res/values/strings.xml

@@ -855,6 +855,8 @@
    <string name="follows">Follows</string>
    <string name="notifications_label_new_notifications">New notifications</string>
    <string name="notifications_label_new_notifications_subtitle">Tap to show them</string>
+    <string name="notifications_label_swipe_for_more_snackbar">Swipe for more notifications</string>
+    <string name="notifications_label_swipe_snackbar_dont_show_again">OKAY</string>


Is it used?

addressed in 1ffd1ce


          Make sure the notification object is available when resuming the Frag…

…ment

kwonye · 2016-11-22T15:30:35Z

I'll defer to everyones' judgement here: design's 👌 with me

Thanks @drw158 for the feedback!


          removed unused import


          remove unused string


          added resource id handling so we make sure we use Android ids and not…

… comment IDs which can be > MAX_INT


          refactored ReaderViewPagerTransformer into WPViewPagerTransformer and…

… moved into widgets folder

mzorz · 2016-11-22T20:11:32Z

This is ready for a (I guess final) review @daniloercoli !
🙇


          Merge pull request #4814 from wordpress-mobile/issue/deleted-notif-bl…

…ank-screen-resumed Deleted notification can result in blank screen when resumed

daniloercoli · 2016-11-23T10:54:32Z

WordPress/src/main/java/org/wordpress/android/ui/comments/CommentDetailFragment.java

@@ -420,6 +433,13 @@ private void setNoteWithNoteId(String noteId) {
        setRemoteBlogId(note.getSiteId());
    }

+    private void setIdForFragmentContainer(int id){


What do you think if we remove this method and move the assignment mIdForFragmentContainer = id; in newInstance ? Since it's called only there, and the name of this method is quite similar to setIdForCommentContainer confusing me a bit at the beginning of the review.

uhm not sure what you mean - newInstance is a static method that creates a new instance of CommentDetailFragment, and performs any relevant parameter initialization by using its setter methods (such as setNoteWithNoteId, or setReplyText or setIdForFragmentContainer). Should stay as is FWIW

daniloercoli · 2016-11-23T10:59:42Z

...ytics/WordPressAnalytics/src/main/java/org/wordpress/android/analytics/AnalyticsTracker.java

@@ -124,6 +124,7 @@
        NOTIFICATION_UNLIKED,
        NOTIFICATION_TRASHED,
        NOTIFICATION_FLAGGED_AS_SPAM,
+        NOTIFICATION_SWIPE_PAGE_CHANGED,


We should bump Analytics for this new event in both AnalyticsTrackerMixpanel and AnalyticsTrackerNosara. Please read the documentation about adding new events (or ping me in Slack) before setting a name for this event.

Thanks a lot for your explanation and directions on chat! addressed in 9651cdf - would you take another look please? 🙇


          added proper analytics tracking handling for Mixpanel and Nosara


          Move the code that checks if the note obj is not null in `onResume`. …

…Fix a crash on rotation.

daniloercoli · 2016-11-23T12:33:19Z

I see this following crash upon device rotation:

11-23 13:28:27.985 25674-25674/org.wordpress.android E/AndroidRuntime: FATAL EXCEPTION: main
                                                                       Process: org.wordpress.android, PID: 25674
                                                                       java.lang.RuntimeException: Unable to resume activity {org.wordpress.android/org.wordpress.android.ui.notifications.NotificationsDetailActivity}: java.lang.IllegalArgumentException: Must use non-zero containerViewId
                                                                           at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3400)
                                                                           at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3440)
                                                                           at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2713)
                                                                           at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:4483)
                                                                           at android.app.ActivityThread.-wrap19(ActivityThread.java)
                                                                           at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1466)
                                                                           at android.os.Handler.dispatchMessage(Handler.java:102)
                                                                           at android.os.Looper.loop(Looper.java:154)
                                                                           at android.app.ActivityThread.main(ActivityThread.java:6077)
                                                                           at java.lang.reflect.Method.invoke(Native Method)
                                                                           at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:865)
                                                                           at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:755)
                                                                        Caused by: java.lang.IllegalArgumentException: Must use non-zero containerViewId
                                                                           at android.app.BackStackRecord.replace(BackStackRecord.java:501)
                                                                           at android.app.BackStackRecord.replace(BackStackRecord.java:496)
                                                                           at org.wordpress.android.ui.comments.CommentDetailFragment.showCommentForNote(CommentDetailFragment.java:1132)
                                                                           at org.wordpress.android.ui.comments.CommentDetailFragment.showComment(CommentDetailFragment.java:612)
                                                                           at org.wordpress.android.ui.comments.CommentDetailFragment.setNote(CommentDetailFragment.java:417)
                                                                           at org.wordpress.android.ui.comments.CommentDetailFragment.setNoteWithNoteId(CommentDetailFragment.java:432)
                                                                           at org.wordpress.android.ui.comments.CommentDetailFragment.onResume(CommentDetailFragment.java:347)
                                                                           at android.app.Fragment.performResume(Fragment.java:2398)
                                                                           at android.app.FragmentManagerImpl.moveToState(FragmentManager.java:1032)
                                                                           at android.app.FragmentManagerImpl.moveToState(FragmentManager.java:1171)
                                                                           at android.app.FragmentManagerImpl.moveToState(FragmentManager.java:1153)
                                                                           at android.app.FragmentManagerImpl.dispatchResume(FragmentManager.java:2049)
                                                                           at android.app.FragmentController.dispatchResume(FragmentController.java:198)
                                                                           at android.app.Activity.performResume(Activity.java:6789)
                                                                           at android.app.ActivityThread.performResumeActivity(ActivityThread.java:3377)
                                                                           at android.app.ActivityThread.handleResumeActivity(ActivityThread.java:3440) 
                                                                           at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2713) 
                                                                           at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:4483) 
                                                                           at android.app.ActivityThread.-wrap19(ActivityThread.java) 
                                                                           at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1466) 
                                                                           at android.os.Handler.dispatchMessage(Handler.java:102) 
                                                                           at android.os.Looper.loop(Looper.java:154) 
                                                                           at android.app.ActivityThread.main(ActivityThread.java:6077) 
                                                                           at java.lang.reflect.Method.invoke(Native Method) 
                                                                           at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:865) 
                                                                           at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:755)

mzorz · 2016-11-23T12:36:11Z

gotcha - let me check it out right away.


          fixed id=0 exception - saving state for fragment container id

mzorz · 2016-11-23T14:47:51Z

addressed crash in commit 6210d32


          merged develop and resolved merge conflicts

daniloercoli · 2016-11-23T21:48:21Z

mzorz added 9 commits Nov 15, 2016

first implementation of viewpager for lateral notification detail nav…

Loading status checks…

ea17524

…igation

change action bar title as you navigate the Notes list left and right…

cfb5557

… in the viewpager

marking as read each note as they are shown into the viewpager curren…

Loading status checks…

beafe76

…t item view

filtered lists work now on horizontal navigation

Loading status checks…

c9bb57e

Merge branch 'develop' into issue/4727-unotifs-up-down-navigatino-arrows

7c04018

fixed filter passing for filtered notification list

Loading status checks…

7e4c29f

Merge branch 'develop' into issue/4727-unotifs-up-down-navigatino-arrows

Loading status checks…

fa2b945

added left/right arrows navigation

Loading status checks…

018c4b9

changed getFragmentManager for getChildFragmentManager call to suppor…

Loading status checks…

d77b0b7

…t nested fragments for Comments structure in ViewPager

mzorz assigned daniloercoli Nov 17, 2016

mzorz added 2 commits Nov 17, 2016

removed unused import

Loading status checks…

15862a7

removed unused imports

Loading status checks…

9289ed1

mzorz changed the title ~~Issue/4727 unotifs up down navigatino arrows~~ Nov 18, 2016

mzorz changed the title ~~Issue/4727 notifs up down navigation arrows~~ Nov 18, 2016

added workaorund for Android bug by suggestion of @nbradbury

Loading status checks…

12dc966

mzorz mentioned this pull request Nov 18, 2016

changes use of Fragment for support.v4 fragments to make use of nested fragments in API < 17 #4806

Closed

assigning a different id to the comment detail list fragment containe…

Loading status checks…

98c2884

…r, so we are sure to change the right fragment in fragment transaction without the need to use the ChildFragmentManager which is only available on API level 17+

took snackbar action off, and now snackbar is not presented anymore o…

Loading status checks…

61b4f76

…nce user does first use swipe

nbradbury reviewed Nov 22, 2016

View changes

fixed showing snackbar for swiping only if swipe will be available

Loading status checks…

ccdb2a9

daniloercoli requested changes Nov 22, 2016

View changes

daniloercoli added the Notifications label Nov 22, 2016

Make sure the notification object is available when resuming the Frag…

Loading status checks…

40f4d2d

…ment

kwonye mentioned this pull request Nov 22, 2016

Issue/4768 move edit button #4782

Merged

mzorz added 4 commits Nov 22, 2016

removed unused import

b935380

remove unused string

Loading status checks…

1ffd1ce

added resource id handling so we make sure we use Android ids and not…

8da2cea

… comment IDs which can be > MAX_INT

refactored ReaderViewPagerTransformer into WPViewPagerTransformer and…

Loading status checks…

382808f

… moved into widgets folder

Merge pull request #4814 from wordpress-mobile/issue/deleted-notif-bl…

Loading status checks…

e1165f1

…ank-screen-resumed Deleted notification can result in blank screen when resumed

daniloercoli reviewed Nov 23, 2016

View changes

mzorz and others added 2 commits Nov 23, 2016

added proper analytics tracking handling for Mixpanel and Nosara

Loading status checks…

9651cdf

Move the code that checks if the note obj is not null in `onResume`. …

Loading status checks…

f33da9c

…Fix a crash on rotation.

fixed id=0 exception - saving state for fragment container id

Loading status checks…

6210d32

merged develop and resolved merge conflicts

Loading status checks…

b885699

daniloercoli approved these changes Nov 23, 2016

View changes

daniloercoli merged commit b3d8f8f into develop Nov 23, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

continuous-integration/travis-ci/push The Travis CI build passed
Details

maxme deleted the issue/4727-unotifs-up-down-navigatino-arrows branch Feb 24, 2017

mzorz mentioned this pull request Jan 8, 2019

Stop setting CommentDetailFragment's id manually #8923

Merged

1 of 1 task complete

Aug	SEP	Oct
	28
2019	2020	2021

wordpress-mobile / WordPress-Android

Issue/4727 notifs detail view horizontal navigation #4796

Issue/4727 notifs detail view horizontal navigation #4796

mzorz commented Nov 17, 2016

daniloercoli commented Nov 17, 2016

daniloercoli commented Nov 17, 2016

nbradbury commented Nov 17, 2016

daniloercoli commented Nov 17, 2016 •

edited

nbradbury commented Nov 17, 2016

mzorz commented Nov 18, 2016

mzorz commented Nov 18, 2016

mzorz commented Nov 18, 2016

mzorz commented Nov 18, 2016

daniloercoli commented Nov 18, 2016

mzorz commented Nov 18, 2016

davewhitley commented Nov 19, 2016 •

edited

mzorz commented Nov 19, 2016

mzorz commented Nov 21, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

kwonye commented Nov 22, 2016

mzorz commented Nov 22, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

daniloercoli commented Nov 23, 2016

mzorz commented Nov 23, 2016

mzorz commented Nov 23, 2016

daniloercoli commented Nov 23, 2016

wordpress-mobile / WordPress-Android

Join GitHub today

Issue/4727 notifs detail view horizontal navigation #4796

Issue/4727 notifs detail view horizontal navigation #4796

Conversation

mzorz commented Nov 17, 2016

daniloercoli commented Nov 17, 2016

daniloercoli commented Nov 17, 2016

nbradbury commented Nov 17, 2016

daniloercoli commented Nov 17, 2016 • edited

nbradbury commented Nov 17, 2016

mzorz commented Nov 18, 2016

mzorz commented Nov 18, 2016

mzorz commented Nov 18, 2016

mzorz commented Nov 18, 2016

daniloercoli commented Nov 18, 2016

mzorz commented Nov 18, 2016

davewhitley commented Nov 19, 2016 • edited

mzorz commented Nov 19, 2016

mzorz commented Nov 21, 2016

This comment has been minimized.

nbradbury Nov 22, 2016 Contributor

This comment has been minimized.

mzorz Nov 22, 2016 Author Contributor

This comment has been minimized.

daniloercoli Nov 22, 2016 Contributor

This comment has been minimized.

mzorz Nov 22, 2016 Author Contributor

This comment has been minimized.

daniloercoli Nov 22, 2016 Contributor

This comment has been minimized.

kwonye Nov 22, 2016 Contributor

This comment has been minimized.

mzorz Nov 22, 2016 Author Contributor

This comment has been minimized.

daniloercoli Nov 22, 2016 Contributor

This comment has been minimized.

mzorz Nov 22, 2016 Author Contributor

This comment has been minimized.

daniloercoli Nov 22, 2016 Contributor

This comment has been minimized.

mzorz Nov 22, 2016 Author Contributor

kwonye commented Nov 22, 2016

mzorz commented Nov 22, 2016

This comment has been minimized.

daniloercoli Nov 23, 2016 Contributor

This comment has been minimized.

mzorz Nov 23, 2016 Author Contributor

This comment has been minimized.

daniloercoli Nov 23, 2016 Contributor

This comment has been minimized.

daniloercoli Nov 23, 2016 Contributor

This comment has been minimized.

mzorz Nov 23, 2016 Author Contributor

daniloercoli commented Nov 23, 2016

mzorz commented Nov 23, 2016

mzorz commented Nov 23, 2016

daniloercoli commented Nov 23, 2016

daniloercoli commented Nov 17, 2016 •

edited

davewhitley commented Nov 19, 2016 •

edited