Adopt the project with ViewPager2 #4020

ebraminio · 2020-04-08T12:17:37Z

ViewPager2 is a recently added component to fix different issues of ViewPager which is itself built upon RecyclerView (though we haven't used this property of it here).

The specific thing that has motivated me to adopt the project with ViewPager2 is lack of RTL languages support on ViewPager which now is supported by ViewPager2, most annoying thing for me when I use the app with a device that uses Persian for its UI, see this for example,

This patch turns it into: (which is the correct scrolling for touch screens)

So the scrolling won't be confusing for RTL languages users anymore. I've fixed minor things which will explain as comment for the patch.

Thanks!

ebraminio · 2020-04-08T12:21:00Z

app/build.gradle

    implementation "android.arch.work:work-runtime:$workManagerVersion"
-    implementation "com.google.android.material:material:1.0.0"
+    implementation "com.google.android.material:material:1.1.0"


Had to upgrade material version to use TabLayoutMediator which is added in 1.1.0.

ebraminio · 2020-04-08T12:24:37Z

app/src/main/java/de/danoeh/antennapod/view/PagerIndicatorView.java

 import androidx.annotation.Nullable;
-import androidx.vectordrawable.graphics.drawable.ArgbEvaluator;


It didn't build with the change and androidx one is apparently declared for internal use of the package.

ebraminio · 2020-04-08T12:26:31Z

app/src/main/java/de/danoeh/antennapod/view/PagerIndicatorView.java

            @Override
            public void onPageScrolled(int position, float positionOffset, int positionOffsetPixels) {
                PagerIndicatorView.this.position = position + positionOffset;
+                if (ViewCompat.getLayoutDirection(pager) == ViewCompat.LAYOUT_DIRECTION_RTL) {
+                    PagerIndicatorView.this.position = numPages - 1 - PagerIndicatorView.this.position;


Flips the pager dots for RTL languages, it can't be done once for the view as direction isn't specified at that time and should be done when the view is attached.

core/src/main/java/de/danoeh/antennapod/core/storage/PodDBAdapter.java

ByteHamster · 2020-04-08T15:16:01Z

Thank you very much for your pull request! It fixes many bugs that must have been there for a long time.

It would be easier to merge if you separated the changes to multiple pull requests. For example, the SQL changes are independent from ViewPager2 and I would directly merge those without discussion ;)

Some things I noticed while skimming the code:

You fixed some SQL problems in PodDBAdapter. Did you fix all cases where we build sql statements with numbers or just the ones that caused problems in your tests?
In order to be able to swipe the media player up and down on each page, we had to add a hacky workaround for setting the scrolling child of the BottomSheetBehavior. This is probably still an issue, so ViewPagerBottomSheetBehavior needs to be adapted to ViewPager2.

Checkstyle fails because of the following lines:

[ERROR] /home/circleci/AntennaPod/./app/src/main/java/de/danoeh/antennapod/fragment/DownloadsFragment.java:55: switch without "default" clause. [MissingSwitchDefault]
[ERROR] /home/circleci/AntennaPod/./app/src/main/java/de/danoeh/antennapod/fragment/EpisodesFragment.java:59: switch without "default" clause. [MissingSwitchDefault]
[ERROR] /home/circleci/AntennaPod/./app/src/main/java/de/danoeh/antennapod/fragment/preferences/StatisticsFragment.java:50: switch without "default" clause. [MissingSwitchDefault]

Checkstyle only checks changed lines, so that new code follows the rules. You did not change the semantics but because you touched the lines, it complains. Would be great if you could fix the errors there.

The release build test fails because of the update of com.google.android.material. That update adds so many more methods that AntennaPod no longer fits into a single dex file. I was trying to avoid updating that library. Is the TabLayoutMediator necessary to make it work? I am not experienced with ViewPager2, so this might be a stupid question ;)
If it is really necessary, it looks like we have to switch to MultiDex (what I tried to avoid in the past).
Regarding commit d938ed2 (Localize digits in queue episodes and parallel downloads numbers), we should probably stop appending text manually and use a placeholder in the string resource instead. This requires re-translation but I think this would be a lot more clean. This is not really related to the RTL changes, so feel free to skip this.
Regarding the translation changes: We use Transifex for translations. Changes to translated string files will be overridden when pulling from Transifex the next time. Merging the local changes with those from Transifex is a lot of work, so please only modify translations using Transifex.

ebraminio · 2020-04-08T15:31:59Z

Thanks for the quick review! :)

The release build test fails because of the update of com.google.android.material. That update adds so many more methods that AntennaPod no longer fits into a single dex file. I was trying to avoid updating that library. Is the TabLayoutMediator necessary to make it work? I am not experienced with ViewPager2, so this might be a stupid question ;)

Yes, or copying it into the project, guess we have to adopt that version anyway.

Will apply the rest. Just was in hurry to fix things I see while testing :)

ebraminio · 2020-04-08T15:49:33Z

You fixed some SQL problems in PodDBAdapter. Did you fix all cases where we build sql statements with numbers or just the ones that caused problems in your tests?

AFAIK all of them, will provide the patch separately.

In order to be able to swipe the media player up and down on each page, we had to add a hacky workaround for setting the scrolling child of the BottomSheetBehavior. This is probably still an issue, so ViewPagerBottomSheetBehavior needs to be adapted to ViewPager2.

I am not sure where the hack is but viewpager2 has an advantage on being able to resize its height in a better way so guess need some help here.

Checkstyle fails because of the following lines:

Will apply.

If it is really necessary, it looks like we have to switch to MultiDex (what I tried to avoid in the past).

It is understandable that you are trying to avoid, not sure what to suggest :/

Regarding commit d938ed2 (Localize digits in queue episodes and parallel downloads numbers), we should probably stop appending text manually and use a placeholder in the string resource instead. This requires re-translation but I think this would be a lot more clean. This is not really related to the RTL changes, so feel free to skip this.

Would you please handle this if won't take much time? Other I will do of course.

Regarding the translation changes: We use Transifex for translations. Changes to translated string files will be overridden when pulling from Transifex the next time. Merging the local changes with those from Transifex is a lot of work, so please only modify translations using Transifex.

Just started translation there. Thanks!

ByteHamster · 2020-04-08T16:24:42Z

I am not sure where the hack is but viewpager2 has an advantage on being able to resize its height in a better way so guess need some help here.

BottomSheetBehavior only allows one scrolling child but we use a ViewPager with multiple scrolling children (shownotes, chapters). So in order to support that, we subclassed BottomSheetBehavior using an androidx package name to access the package-private field that stores the scrolling child. Whenever the ViewPager is scrolled, we guess its currently active child view by assuming that offscreenPageLimit equals the total number of pages (otherwise, retrieving the currently visible child view requires reflection) and update the scrolling child in BottomSheetBehavior. This is pretty messy, unfortunately...

Have a look at ViewPagerBottomSheetBehavior. That class is pretty short and I hope that it can be adapted to ViewPager2 with only a few changes.

ebraminio · 2020-04-09T13:58:58Z

Dropped the unrelated changes (and added your multidex check to my app :) persian-calendar/persian-calendar@4859aa8)

ebraminio · 2020-04-09T14:36:39Z

Used a local copy of TabLayoutMediator instead upgrading material, guess should work now

ebraminio · 2020-04-09T15:21:17Z

Bots are OK now! PTAL

ebraminio · 2020-04-09T15:27:02Z

Have a look at ViewPagerBottomSheetBehavior. That class is pretty short and I hope that it can be adapted to ViewPager2 with only a few changes.

It looks interesting, will investigate what more can be done about but in theory it should work now just like what was before, let's see.

app/src/main/java/de/danoeh/antennapod/fragment/AudioPlayerFragment.java

ByteHamster · 2020-04-09T19:54:51Z

app/src/main/java/com/google/android/material/tabs/TabLayoutMediator.java

+ * {@link #attach()} call. Changing the ViewPager2 or TabLayout will require a new instantiation of
+ * TabLayoutMediator.
+ */
+public final class TabLayoutMediator {


Hmm. I want to keep the amount of code that we need to take care of as small as possible. At some point, we have to upgrade the dependencies. We already use 64,945 out of 65,536 methods, so we can not go much longer without multidex, anyway.

I will play around with ProGuard to see if we can get a bit more room for new methods without enabling MultiDex. I mean, even big apps like WhatsApp work without MultiDex. Something seems to waste methods...

Hmm. I want to keep the amount of code that we need to take care of as small as possible. At some point, we have to upgrade the dependencies. We already use 64,945 out of 65,536 methods, so we can not go much longer without multidex, anyway.

So what do you suggest? It isn't a fork (except a new line that had added because of local lints), can (and should) be deleted after the upgrade.

I will play around with ProGuard to see if we can get a bit more room for new methods without enabling MultiDex. I mean, even big apps like WhatsApp work without MultiDex. Something seems to waste methods...

They may rely more on native codes I believe. I will also check what has consumed the methods number.

I brought it down to around 44k by removing -keep class androidx.** { *; } from the proguard config. Currently fine-tuning the proguard settings so that the tests pass.

Let me know when you landed it. Thanks!

Done :) #4035 is merged, so we have a bit more room for library updates now

ebraminio · 2020-04-10T09:46:47Z

app/src/main/java/de/danoeh/antennapod/fragment/gpodnet/GpodnetMainFragment.java

-        private static final int NUM_PAGES = 2;
-        private static final int POS_TOPLIST = 0;
-        private static final int POS_TAGS = 1;
-        private static final int POS_SUGGESTIONS = 2;


Not in scope of this change but weird that NUM_PAGES is 2 but we have three pages here. Guess better to fix it after the PR.

@ByteHamster while this may take time if possile please have a look if NUM_PAGES = 2 is really desired and nothing has got hide by mistake if possible thanks.

I created issue #4093 for this.

ebraminio · 2020-04-10T10:08:47Z

Bots are happy and did manual testing also. PTAL

ebraminio · 2020-04-10T10:10:07Z

Have updated trasifex with improved messages, wish to see the update soon also.

app/src/main/java/de/danoeh/antennapod/fragment/AudioPlayerFragment.java

ByteHamster

Thanks for rebasing. I have noticed some bugs while testing this PR.

The fragments store the last-used tab. With this PR, the indicator still shows the saved tab but the page that is actually displayed is always the first one. The displayed highlight is corrected after touching the screen for the first time.

Wrong highlight:

How the log screen actually looks like:

While scrolling, the menu is sometimes duplicated (when showing that page for the first time). In previous versions, this was not the case.

If you have shownotes with a big width (more than the screen width), you can no longer scroll their content horizontally.

app/src/main/java/com/google/android/material/bottomsheet/ViewPagerBottomSheetBehavior.java

ByteHamster · 2020-04-10T15:02:03Z

app/src/main/java/de/danoeh/antennapod/fragment/ItemPagerFragment.java

        // FragmentStatePagerAdapter documentation:
        // > When using FragmentStatePagerAdapter the host ViewPager must have a valid ID set.
        // When opening multiple ItemPagerFragments by clicking "item" -> "visit podcast" -> "item" -> etc,
        // the ID is no longer unique and FragmentStatePagerAdapter does not display any pages.
        int newId = ViewCompat.generateViewId();
        pager.setId(newId);
-        pager.setAdapter(new ItemPagerAdapter());
+        pager.setAdapter(new ItemPagerAdapter(this));
        pager.setCurrentItem(feedItemPos);


When clicking an episode (not the first one in the list), I now see an animation scrolling through the pages of other episodes. Not sure what to do about that.

Done e2ff176

ebraminio · 2020-04-15T13:04:26Z

Reverted ViewPager2 adoption in ItemPagerFragment, I understand if you don't see this as a solution so will investigate how it can be fixed properly, but lack of RTL adoption isn't that issue here as there is not tablayout indicator to make things confusing.

The reason needs cast: https://issuetracker.google.com/issues/144669596

As was requested in AntennaPod#4020 (comment)

As was suggested in AntennaPod#4020 (comment)

As ViewPager2 different lifecycle apparently setHasOptionsMenu needs to be called from onResume() from now on.

Now manages nested child scrolling when the orientation isn't perpendicular

ebraminio · 2020-04-29T12:19:04Z

@ByteHamster (posting again, sorry for the spamming) Played some more time with it and now it looks good AFAICT, feel free to have a look to see if you also feel that way. Thanks :)

ByteHamster

Looks good to me. Thanks a lot for all the work! Ready for merging?

ebraminio · 2020-05-01T10:21:23Z

Looks good to me. Thanks a lot for all the work! Ready for merging?

I believe so. Thanks :)

ebraminio commented Apr 8, 2020

View reviewed changes

core/src/main/java/de/danoeh/antennapod/core/storage/PodDBAdapter.java Outdated Show resolved Hide resolved

ebraminio changed the title ~~Adopt the project ViewPager2~~ Adopt the project with ViewPager2 Apr 8, 2020

ebraminio force-pushed the rtl branch from 089f931 to 3117ca2 Compare April 8, 2020 13:16

ebraminio mentioned this pull request Apr 8, 2020

Fix locale of every String.format number formatter #4021

Merged

ebraminio force-pushed the rtl branch from bcb529e to be3a8c3 Compare April 9, 2020 13:48

ebraminio force-pushed the rtl branch from 905b0cf to 98277da Compare April 9, 2020 15:07

ByteHamster reviewed Apr 9, 2020

View reviewed changes

ebraminio force-pushed the rtl branch 2 times, most recently from ace47d0 to 3b1737e Compare April 10, 2020 09:39

ebraminio commented Apr 10, 2020

View reviewed changes

ebraminio force-pushed the rtl branch from c885431 to 53b51de Compare April 10, 2020 09:56

ebraminio force-pushed the rtl branch from 53b51de to 24fc4c2 Compare April 10, 2020 12:05

ebraminio commented Apr 10, 2020

View reviewed changes

app/src/main/java/de/danoeh/antennapod/fragment/AudioPlayerFragment.java Show resolved Hide resolved

ebraminio force-pushed the rtl branch 3 times, most recently from accc97e to 0416230 Compare April 10, 2020 13:43

ByteHamster reviewed Apr 10, 2020

View reviewed changes

ebraminio force-pushed the rtl branch from a225ec0 to 829b75f Compare April 15, 2020 12:58

ebraminio force-pushed the rtl branch from 829b75f to 0d43d97 Compare April 21, 2020 14:57

ebraminio added 16 commits April 21, 2020 19:30

Port AudioPlayer's viewpager to ViewPager2, improve RTL support

cedfb27

Adopt StatisticsFragment with ViewPager2

417c66d

Port pager_fragment users to ViewPager2

e8166a7

Port ItemPagerFragment to ViewPager2

74ec3b1

Match pager_fragment's pager height with its parent

79e8536

Remove remained references of ViewPager

3a11f28

Add default cases for switches lack them

965fc5c

Use constants instead hardcoded numbers in view pagers adapters

75e9ee9

Bring back AudioPlayerFragment pager's setOffscreenPageLimit

260d7ee

The reason needs cast: https://issuetracker.google.com/issues/144669596

Disable bringing episode animation

292afd7

As was requested in AntennaPod#4020 (comment)

Fix view pager scrolling of bottom sheet

77dc78d

As was suggested in AntennaPod#4020 (comment)

Restore current tab right after downloads and episodes views creation

1b9937c

Disable downloads and episodes view tab position restore animation

c88336f

Fix menu items duplication in episodes and downloads fragment

430e0d4

As ViewPager2 different lifecycle apparently setHasOptionsMenu needs to be called from onResume() from now on.

Use more optimized way to detect direction in PagerIndicatorView

17962b5

Wrap scrollable childs of viewpager in NestedScrollableHost

6f74af7

ebraminio force-pushed the rtl branch 4 times, most recently from cd93435 to 885f333 Compare April 21, 2020 17:05

Modify NestedScrollableHost to suit our need

7bdacf8

Now manages nested child scrolling when the orientation isn't perpendicular

ebraminio force-pushed the rtl branch from 885f333 to 7bdacf8 Compare April 21, 2020 17:10

ByteHamster approved these changes May 1, 2020

View reviewed changes

ByteHamster merged commit b870c23 into AntennaPod:develop May 1, 2020

ebraminio deleted the rtl branch May 1, 2020 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adopt the project with ViewPager2 #4020

Adopt the project with ViewPager2 #4020

ebraminio commented Apr 8, 2020 •

edited

ebraminio Apr 8, 2020

ebraminio Apr 8, 2020

ebraminio Apr 8, 2020

ByteHamster commented Apr 8, 2020

ebraminio commented Apr 8, 2020

ebraminio commented Apr 8, 2020 •

edited

ByteHamster commented Apr 8, 2020

ebraminio commented Apr 9, 2020

ebraminio commented Apr 9, 2020

ebraminio commented Apr 9, 2020

ebraminio commented Apr 9, 2020

ByteHamster Apr 9, 2020

ebraminio Apr 9, 2020 •

edited

ebraminio Apr 9, 2020

ByteHamster Apr 9, 2020

ebraminio Apr 10, 2020

ByteHamster Apr 10, 2020

ebraminio Apr 10, 2020 •

edited

ebraminio Apr 11, 2020 •

edited

ByteHamster May 1, 2020

ebraminio commented Apr 10, 2020 •

edited

ebraminio commented Apr 10, 2020

ByteHamster left a comment

ByteHamster Apr 10, 2020

ebraminio Apr 10, 2020

ebraminio commented Apr 15, 2020

ebraminio commented Apr 29, 2020

ByteHamster left a comment

ebraminio commented May 1, 2020

		import androidx.annotation.Nullable;
		import androidx.vectordrawable.graphics.drawable.ArgbEvaluator;

Adopt the project with ViewPager2 #4020

Adopt the project with ViewPager2 #4020

Conversation

ebraminio commented Apr 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ByteHamster commented Apr 8, 2020

ebraminio commented Apr 8, 2020

ebraminio commented Apr 8, 2020 • edited

ByteHamster commented Apr 8, 2020

ebraminio commented Apr 9, 2020

ebraminio commented Apr 9, 2020

ebraminio commented Apr 9, 2020

ebraminio commented Apr 9, 2020

Choose a reason for hiding this comment

ebraminio Apr 9, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebraminio Apr 10, 2020 • edited

Choose a reason for hiding this comment

ebraminio Apr 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebraminio commented Apr 10, 2020 • edited

ebraminio commented Apr 10, 2020

ByteHamster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebraminio commented Apr 15, 2020

ebraminio commented Apr 29, 2020

ByteHamster left a comment

Choose a reason for hiding this comment

ebraminio commented May 1, 2020

ebraminio commented Apr 8, 2020 •

edited

ebraminio commented Apr 8, 2020 •

edited

ebraminio Apr 9, 2020 •

edited

ebraminio Apr 10, 2020 •

edited

ebraminio Apr 11, 2020 •

edited

ebraminio commented Apr 10, 2020 •

edited