(Analytics) implementing ViewPager2 #1427

albertoazinar · 2022-10-23T13:48:45Z

The tab names are appearing.
But the images are not loading on the screen.

google-cla · 2022-10-23T13:48:49Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

thatfiredev

I haven't yet reviewed the Java and Kotlin files. I'll do it once the XML is fixed:

analytics/build.gradle

analytics/.gitignore

analytics/app/src/main/res/layout/activity_main.xml

analytics/.gitignore

analytics/app/src/main/res/layout/activity_main.xml

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/java/MainActivity.java

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/java/MainActivity.java

saketkumar · 2022-10-29T20:22:55Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/java/MainActivity.java

-        mViewPager = binding.viewPager;
+        mViewPager =  (ViewPager2)findViewById(R.id.viewPager);
        mViewPager.setAdapter(mImagePagerAdapter);
+        mViewPager.setOrientation(ViewPager2.ORIENTATION_HORIZONTAL);


Do we need to set the orientation to Horizontal?
I think the default orientation is Horizontal only.

analytics/app/src/main/res/layout/activity_main.xml

…android

analytics/.gitignore

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/java/MainActivity.java

thatfiredev · 2022-11-09T14:51:28Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

+        //binding.viewPager.adapter = imagePagerAdapter
+        val viewPager: ViewPager2 = findViewById(R.id.viewPager)
+        viewPager.adapter = imagePagerAdapter

-        // Workaround for AppCompat issue not showing ViewPager titles
-        val params = binding.pagerTabStrip.layoutParams as ViewPager.LayoutParams
-        params.isDecor = true
-
-        // When the visible image changes, send a screen view hit.
-        binding.viewPager.addOnPageChangeListener(object : ViewPager.SimpleOnPageChangeListener() {
-            override fun onPageSelected(position: Int) {
-                recordImageView()
-                recordScreenView()
-            }
-        })
+        val tabLayout: TabLayout = findViewById(R.id.tab_layout)


Let's keep viewbinding.

should we implement OnPageChangeCallback instead of addOnPageChangeListener
Because I'm getting an warning on addOnPageChangeListener what makes me think it is not recognized anymore.

Oh, yes, let's use OnPageChangeCallback. I believe the method is registerOnPageChangeCallback()

thatfiredev · 2022-11-09T14:51:48Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

     */
    private fun getCurrentImageId(): String {
-        val position = binding.viewPager.currentItem
+        val position = 0//binding.viewPager.currentItem


Suggested change

val position = 0//binding.viewPager.currentItem

val position = binding.viewPager.currentItem

thatfiredev · 2022-11-09T14:52:03Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

    @SuppressLint("WrongConstant")
    inner class ImagePagerAdapter(
-        fm: FragmentManager,
+        fm: FragmentActivity,


Let's use the same constructor we used on Java.

thatfiredev · 2022-11-09T14:52:08Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

+     * A [FragmentStateAdapter] that returns a fragment corresponding to
     * one of the sections/tabs/pages.
     */
    @SuppressLint("WrongConstant")


Suggested change

@SuppressLint("WrongConstant")

thatfiredev · 2022-11-09T14:52:43Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

+        override fun getItemCount(): Int {
+            return infos.size
+        }


nit: this can actually become a single line:

Suggested change

override fun getItemCount(): Int {

return infos.size

}

override fun getItemCount(): Int = infos.size

thatfiredev

@AlbertoYabeda Thanks for patiently working on each and every review comment here.
We're 99% done 😬 Just a few more comments

thatfiredev · 2022-11-23T18:17:49Z

analytics/.gitignore

 /local.properties
 .DS_Store
 build/
-google-services.json


This line shouldn't be removed. 😬

analytics/app/src/main/AndroidManifest.xml

thatfiredev · 2022-11-23T18:19:06Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/java/MainActivity.java

     */
    private String getCurrentImageId() {
-        int position = mViewPager.getCurrentItem();
+        int position = mViewPager.getCurrentItem();;


Suggested change

int position = mViewPager.getCurrentItem();;

int position = mViewPager.getCurrentItem();

thatfiredev · 2022-11-23T18:20:29Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

+        val viewPager: ViewPager2 = findViewById(R.id.viewPager)
+        viewPager.adapter = imagePagerAdapter


Let's stick to ViewBinding:

Suggested change

val viewPager: ViewPager2 = findViewById(R.id.viewPager)

viewPager.adapter = imagePagerAdapter

thatfiredev · 2022-11-23T18:21:00Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

+
+        binding.viewPager.registerOnPageChangeCallback(pageChangedCallback)
+
+        val tabLayout: TabLayout = findViewById(R.id.tab_layout)


Let's stick to ViewBinding:

Suggested change

val tabLayout: TabLayout = findViewById(R.id.tab_layout)

val tabLayout: TabLayout = binding.tabLayout

thatfiredev · 2022-11-23T18:21:55Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

+        lc: Lifecycle
+    ) : FragmentStateAdapter(fm, lc) {


nit: maybe we should make the variable name consistent with the java sample:

Suggested change

lc: Lifecycle

) : FragmentStateAdapter(fm, lc) {

lifecycle: Lifecycle

) : FragmentStateAdapter(fm, lifecycle) {

albertoazinar · 2022-11-23T20:16:55Z

Thanks for your patience reviewing my PR. 😄

thatfiredev · 2022-11-28T22:01:08Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

+        val viewPager: ViewPager2 = binding.viewPager
+        viewPager.adapter = imagePagerAdapter


This code is simply repeating the line above it.

Suggested change

val viewPager: ViewPager2 = binding.viewPager

viewPager.adapter = imagePagerAdapter

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

thatfiredev · 2022-11-28T22:02:32Z

analytics/app/src/main/java/com/google/firebase/quickstart/analytics/kotlin/MainActivity.kt

+        binding.viewPager.registerOnPageChangeCallback(pageChangedCallback)
+
+        val tabLayout: TabLayout = binding.tabLayout
+        TabLayoutMediator(tabLayout, viewPager) { tab, position ->


If the code repetition comment gets applied:

Suggested change

TabLayoutMediator(tabLayout, viewPager) { tab, position ->

TabLayoutMediator(tabLayout, binding.viewPager) { tab, position ->

thatfiredev · 2022-11-28T22:07:23Z

analytics/app/src/main/res/layout/activity_main.xml

+        app:layout_constraintBottom_toTopOf="@+id/viewPager"
+        app:layout_constraintEnd_toEndOf="@id/viewPager"


Let's not attach this to the viewPager. (note that this is my personal preference, I don't think there's specific guidance on that.)

Suggested change

app:layout_constraintBottom_toTopOf="@+id/viewPager"

app:layout_constraintEnd_toEndOf="@id/viewPager"

app:layout_constraintEnd_toEndOf="parent"

I'll set the end to EndOf "parent"

but I think the Bottom of the tabLayout should be at the top of ViewPager.

Correct me if I'm wrong.

thatfiredev · 2022-11-28T22:08:48Z

analytics/app/src/main/res/layout/activity_main.xml

+        android:layout_width="match_parent"
+        android:layout_height="0dp"
+        app:layout_constraintBottom_toBottomOf="parent"
+        app:layout_constraintTop_toBottomOf="@+id/tab_layout" />


This layout seems to be missing horizontal constraints. Can we have some constraints at the Start and End? That will allow us to set the width to "0dp".

Suggested change

android:layout_width="match_parent"

android:layout_height="0dp"

app:layout_constraintBottom_toBottomOf="parent"

app:layout_constraintTop_toBottomOf="@+id/tab_layout" />

android:layout_width="0dp"

android:layout_height="0dp"

app:layout_constraintStart_toStartOf="parent"

app:layout_constraintEnd_toEndOf="parent"

app:layout_constraintBottom_toBottomOf="parent"

app:layout_constraintTop_toBottomOf="@+id/tab_layout" />

Ohh Yes. that would be awesome.

…d imports

thatfiredev

Thank you so much @AlbertoYabeda !

albertoazinar · 2022-11-29T13:55:05Z

Welcome
and thanks for your support @thatfiredev 👌

(Analytics) implementing ViewPager2

a252153

thatfiredev requested changes Oct 24, 2022

View reviewed changes

albertoazinar and others added 5 commits October 24, 2022 23:22

(quickstart-android [analytics]) - setting new layout widgets

94a01ca

(quickstart-android [analytics]) - implem - ViewPager2 java

d859f60

(quickstart-android [analytics]) - implem - ViewPager2 kotlin

87aef7a

(quickstart-android [analytics]) - refact - update on ViewPager2 id

809fa4a

Merge branch 'master' into master

a2804ef

albertoazinar requested a review from thatfiredev October 25, 2022 09:04

thatfiredev requested changes Oct 25, 2022

View reviewed changes

saketkumar reviewed Oct 29, 2022

View reviewed changes

albertoazinar added 3 commits November 8, 2022 23:41

(Analytics) - Tablayout and viewPager attributes fix

2b57dd3

(Analytics) - cede refact. and improvements

3bcda77

Merge branch 'master' of https://github.com/AlbertoYabeda/quickstart-…

b7eaa67

…android

thatfiredev requested changes Nov 9, 2022

View reviewed changes

albertoazinar added 4 commits November 23, 2022 19:18

(firebase-analytics) code refact - removed blank spaces

8b579d1

(firebase-analytics) code refactor - updated methods

619a95f

(firebase-analytics) code refactor - updated deprecated method

4d61fd2

(firebase-analytics) - updated .gitignore

30f0a5e

thatfiredev reviewed Nov 23, 2022

View reviewed changes

albertoazinar added 4 commits November 23, 2022 22:06

(firebase-analytics) - fixed wrong file to ignore

5df23e9

(firebase-analytics) code refact - removing unnecessary permissions

bbed2d3

(firebase-analytics) code refact - removing unnecessary spaces

da66b3d

(firebase-analytics) code refact - fixing nit

3bb877f

(firebase-analytics) code refact

853f398

thatfiredev reviewed Nov 28, 2022

View reviewed changes

albertoazinar added 2 commits November 29, 2022 12:20

(firebase-analytics) code fix - layout nits

6427791

(firebase-analytics) code refact - removing code repetition and unuse…

7cf0bff

…d imports

thatfiredev approved these changes Nov 29, 2022

View reviewed changes

thatfiredev merged commit 93d08e9 into firebase:master Nov 29, 2022

thatfiredev mentioned this pull request Nov 30, 2022

FragmentAdapter is deprecated #1416

Closed

	val position = 0//binding.viewPager.currentItem
	val position = binding.viewPager.currentItem

	int position = mViewPager.getCurrentItem();;
	int position = mViewPager.getCurrentItem();

		val viewPager: ViewPager2 = findViewById(R.id.viewPager)
		viewPager.adapter = imagePagerAdapter


		binding.viewPager.registerOnPageChangeCallback(pageChangedCallback)

		val tabLayout: TabLayout = findViewById(R.id.tab_layout)

	val tabLayout: TabLayout = findViewById(R.id.tab_layout)
	val tabLayout: TabLayout = binding.tabLayout

		val viewPager: ViewPager2 = binding.viewPager
		viewPager.adapter = imagePagerAdapter

	TabLayoutMediator(tabLayout, viewPager) { tab, position ->
	TabLayoutMediator(tabLayout, binding.viewPager) { tab, position ->

		app:layout_constraintBottom_toTopOf="@+id/viewPager"
		app:layout_constraintEnd_toEndOf="@id/viewPager"

	app:layout_constraintBottom_toTopOf="@+id/viewPager"
	app:layout_constraintEnd_toEndOf="@id/viewPager"
	app:layout_constraintEnd_toEndOf="parent"

(Analytics) implementing ViewPager2 #1427

(Analytics) implementing ViewPager2 #1427

Uh oh!

Conversation

albertoazinar commented Oct 23, 2022

Uh oh!

google-cla bot commented Oct 23, 2022

Uh oh!

thatfiredev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thatfiredev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertoazinar commented Nov 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment