Migrate to ViewPager2 #2873
Migrate to ViewPager2 #2873
Conversation
avazirna
force-pushed
the
viewpager2-poc
branch
from
79eb9be to
5e0945e
Compare
avazirna
changed the title
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive refactoring of the detail view and tab management in an Android application. The changes primarily focus on migrating from older Android UI components to more modern alternatives, specifically replacing The modifications span multiple files across the project, including build configuration, layout resources, and Java source files. Key changes include removing deprecated drawable resources for tab styling, updating the The build configuration now includes a The adapter for detail views has been updated to use the newer Sequence DiagramsequenceDiagram
participant User
participant EntityDetailActivity
participant TabbedDetailView
participant EntityDetailPagerAdapter
participant TabLayout
participant ViewPager2
User->>EntityDetailActivity: Open Detail View
EntityDetailActivity->>TabbedDetailView: Initialize View
TabbedDetailView->>TabLayout: Create Tab Layout
TabbedDetailView->>ViewPager2: Initialize ViewPager2
TabbedDetailView->>EntityDetailPagerAdapter: Create Adapter
EntityDetailPagerAdapter-->>TabbedDetailView: Provide Fragments
TabbedDetailView->>TabLayout: Link with ViewPager2
TabLayout->>ViewPager2: Synchronize Tabs
User->>TabLayout: Select Tab
TabLayout->>ViewPager2: Change Page
ViewPager2->>EntityDetailPagerAdapter: Load Fragment
This sequence diagram illustrates the new flow of tab and view management, highlighting the interactions between the key components in the updated implementation. Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
app/src/org/commcare/adapters/EntityDetailPagerAdapter.java (1)
32-32: Consider annotating additional constructor parameters with@NonNullCurrently, only
fmandlifecycleare annotated with@NonNull. Ifdetail,detailIndex,reference, andmodifierare also expected to be non-null, consider annotating them with@NonNullto enhance null safety and ensure consistency.Apply this diff to add
@NonNullannotations:public EntityDetailPagerAdapter(@NonNull FragmentManager fm, @NonNull Lifecycle lifecycle, - Detail detail, int detailIndex, TreeReference reference, ListItemViewModifier modifier) { + @NonNull Detail detail, int detailIndex, @NonNull TreeReference reference, @NonNull ListItemViewModifier modifier) { super(fm, lifecycle); this.detail = detail; this.detailIndex = detailIndex; this.mEntityReference = reference; this.modifier = modifier; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/build.gradle(1 hunks)app/res/drawable/title_case_tab_vertical.xml(0 hunks)app/res/drawable/title_neutral_tab_vertical.xml(0 hunks)app/res/layout/tabbed_detail_view.xml(1 hunks)app/res/values/styles.xml(1 hunks)app/src/org/commcare/activities/EntityDetailActivity.java(0 hunks)app/src/org/commcare/activities/EntitySelectActivity.java(0 hunks)app/src/org/commcare/adapters/EntityDetailPagerAdapter.java(4 hunks)app/src/org/commcare/fragments/BreadcrumbBarFragment.java(0 hunks)app/src/org/commcare/views/TabbedDetailView.java(4 hunks)
💤 Files with no reviewable changes (5)
- app/res/drawable/title_case_tab_vertical.xml
- app/res/drawable/title_neutral_tab_vertical.xml
- app/src/org/commcare/fragments/BreadcrumbBarFragment.java
- app/src/org/commcare/activities/EntityDetailActivity.java
- app/src/org/commcare/activities/EntitySelectActivity.java
🔇 Additional comments (7)
app/build.gradle (1)
463-463: 🛠️ Refactor suggestion
Consider adding ViewPager2 version to dependencies
Since you're migrating to ViewPager2, ensure you've added the appropriate dependency in the dependencies block.
Add the ViewPager2 dependency if not already present:
dependencies {
+ implementation "androidx.viewpager2:viewpager2:1.0.0"
}app/src/org/commcare/adapters/EntityDetailPagerAdapter.java (3)
24-24: Class extension updated to FragmentStateAdapter appropriately
The class now extends FragmentStateAdapter, which is suitable for use with ViewPager2. This change aligns with the migration from ViewPager to ViewPager2.
41-43: createFragment method correctly overrides and uses @NonNull annotation
The createFragment(int position) method properly replaces getItem(int position), conforms to the FragmentStateAdapter requirements, and uses the @NonNull annotation appropriately.
63-65: getItemCount method correctly implements item count logic
The getItemCount() method replaces getCount() and accurately returns the number of items based on whether the detail is compound.
app/res/layout/tabbed_detail_view.xml (2)
13-27: TabLayout added and configured appropriately
The TabLayout is properly added with appropriate attributes to integrate with ViewPager2. Custom attributes and styles are correctly applied, ensuring consistent tab appearance and behavior.
33-37: ViewPager2 replaces previous ViewPager appropriately
The ViewPager2 is correctly configured with layout_height="0dp" and layout_weight="1" to fill the remaining space. This setup is standard for layouts with dynamic content.
app/res/values/styles.xml (1)
346-348: DetailViewTabStyle added appropriately
The new style DetailViewTabStyle defines android:textColor for tab text, ensuring consistent styling across the application.
app/build.gradle
Outdated
| @@ -461,7 +460,7 @@ android { | |||
| signingConfig signingConfigs.release | |||
| proguardFiles getDefaultProguardFile('proguard-android.txt'), 'proguard.cfg' | |||
| testProguardFiles 'test-proguard.cfg' | |||
|
|
|||
| multiDexKeepProguard file('proguard-multidex.pro') | |||
There was a problem hiding this comment.
💡 Codebase verification
Multidex configuration needs to be properly set up
The codebase has multiDexKeepProguard configured but is missing essential multidex setup:
- The
proguard-multidex.profile is missing - No multidex dependency is included
- No explicit
multiDexEnabledconfiguration
To handle method count in release builds, please:
- Add the multidex dependency
- Enable multidex in defaultConfig
- Create the missing
proguard-multidex.profile with proper keep rules
🔗 Analysis chain
Verify multidex configuration for release builds
Since you're adding multidex ProGuard configuration, ensure that multidex is properly enabled for release builds to handle the method count.
Consider adding explicit multidex configuration in your defaultConfig or release buildType if not already present:
android {
defaultConfig {
+ multiDexEnabled true
}
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for multidex configuration
rg -i "multiDexEnabled" app/build.gradle
Length of output: 40
Script:
#!/bin/bash
# Let's check the entire build.gradle content to understand the configuration better
cat app/build.gradle
# Also check for multidex-related dependencies
rg -i "multidex" app/build.gradle
Length of output: 25477
Script:
#!/bin/bash
# Let's check if there's a proguard-multidex.pro file and its contents
cat proguard-multidex.pro 2>/dev/null || echo "File not found"
# Also check for any multidex-related dependencies
rg -i "implementation.*multidex" app/build.gradle
Length of output: 118
💡 Codebase verification
🛠️ Refactor suggestion
Missing ProGuard configuration file referenced in build.gradle
The build configuration references a non-existent ProGuard configuration file proguard-multidex.pro. This file is required for the multiDexKeepProguard configuration to work properly, and its absence could lead to issues in release builds, especially with ViewPager2 integration.
- The referenced file
proguard-multidex.prois missing from the project - No alternative ProGuard configuration files were found in standard locations
- ViewPager2-specific ProGuard rules are not defined anywhere in the project
🔗 Analysis chain
Verify the ProGuard configuration for ViewPager2
The addition of multiDexKeepProguard configuration is appropriate for handling method count limitations in release builds. However, ensure that the proguard-multidex.pro file includes the necessary keep rules for ViewPager2 to prevent any ProGuard-related issues.
Consider adding these ViewPager2-specific ProGuard rules to proguard-multidex.pro if not already present:
# Add to proguard-multidex.pro
+-keep class androidx.viewpager2.widget.ViewPager2 { *; }
+-keepclassmembers class androidx.viewpager2.widget.ViewPager2$** { *; }Let me verify if there are any other ProGuard configuration files in the project that might contain the ViewPager2 rules.
Let me check for any ProGuard configuration files with different extensions and also look for the main ProGuard configuration in the standard Android project locations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if proguard-multidex.pro exists and contains ViewPager2 rules
if [ -f "proguard-multidex.pro" ]; then
echo "Checking proguard-multidex.pro for ViewPager2 rules..."
rg -i "viewpager2" proguard-multidex.pro
else
echo "proguard-multidex.pro file not found"
fi
Length of output: 117
Script:
#!/bin/bash
# Search for any ProGuard configuration files
echo "Searching for ProGuard configuration files..."
fd -e pro
# Search for ViewPager2 related rules in any text file
echo -e "\nSearching for ViewPager2 ProGuard rules in any file..."
rg -i "viewpager2.*keep"
Length of output: 264
Script:
#!/bin/bash
# Search for ProGuard configurations with various extensions
echo "Searching for ProGuard configuration files..."
fd -e proguard
fd -e pro
fd -e txt -x grep -l "proguard" {}
# Check standard Android ProGuard locations
echo -e "\nChecking standard ProGuard locations..."
for file in proguard-rules.pro proguard.pro app/proguard-rules.pro app/proguard.pro; do
if [ -f "$file" ]; then
echo "Found: $file"
echo "Contents:"
cat "$file"
fi
done
Length of output: 729
| } | ||
|
|
||
| public int getCurrentTab() { | ||
| return mViewPager.getCurrentItem(); | ||
| } | ||
|
|
||
| public int getTabCount() { | ||
| return mViewPager.getAdapter().getCount(); | ||
| return Objects.requireNonNull(mViewPager.getAdapter()).getItemCount(); |
There was a problem hiding this comment.
Handle potential null adapter to avoid NullPointerException
Using Objects.requireNonNull(mViewPager.getAdapter()) can throw a NullPointerException if the adapter is null. Consider checking if the adapter is null and handling it appropriately, or ensure that the adapter is always set before this method is called.
Apply this diff to safely handle the adapter:
public int getTabCount() {
- return Objects.requireNonNull(mViewPager.getAdapter()).getItemCount();
+ if (mViewPager.getAdapter() != null) {
+ return mViewPager.getAdapter().getItemCount();
+ } else {
+ return 0;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return Objects.requireNonNull(mViewPager.getAdapter()).getItemCount(); | |
| if (mViewPager.getAdapter() != null) { | |
| return mViewPager.getAdapter().getItemCount(); | |
| } else { | |
| return 0; | |
| } |
There was a problem hiding this comment.
@avazirna are we sure mViewPager.getAdapter() can't be null if a caller calls getTabCount out of order on this view ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if (detail.isCompound()) { | ||
| new TabLayoutMediator(mTabLayout, mViewPager, | ||
| (tab, position) -> tab.setText(detail.getDetails()[position].getTitle().getText().evaluate())) | ||
| .attach(); |
There was a problem hiding this comment.
Add null checks to prevent potential NullPointerException in TabLayoutMediator
When setting up TabLayoutMediator, ensure that detail.getDetails() and the evaluated titles are not null to prevent a NullPointerException. Consider adding null checks or handling potential null values.
Apply this diff to add null checks:
if (detail.isCompound()) {
+ if (detail.getDetails() != null && detail.getDetails().length > 0) {
new TabLayoutMediator(mTabLayout, mViewPager,
(tab, position) -> {
+ String title = detail.getDetails()[position].getTitle().getText().evaluate();
+ if (title != null) {
tab.setText(title);
+ } else {
+ tab.setText("Tab " + position);
+ }
})
.attach();
+ } else {
+ mTabLayout.setVisibility(GONE);
+ }
} else {
mTabLayout.setVisibility(GONE);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (detail.isCompound()) { | |
| new TabLayoutMediator(mTabLayout, mViewPager, | |
| (tab, position) -> tab.setText(detail.getDetails()[position].getTitle().getText().evaluate())) | |
| .attach(); | |
| if (detail.isCompound()) { | |
| if (detail.getDetails() != null && detail.getDetails().length > 0) { | |
| new TabLayoutMediator(mTabLayout, mViewPager, | |
| (tab, position) -> { | |
| String title = detail.getDetails()[position].getTitle().getText().evaluate(); | |
| if (title != null) { | |
| tab.setText(title); | |
| } else { | |
| tab.setText("Tab " + position); | |
| } | |
| }) | |
| .attach(); | |
| } else { | |
| mTabLayout.setVisibility(GONE); | |
| } | |
| } else { | |
| mTabLayout.setVisibility(GONE); | |
| } |
| } | ||
|
|
||
| public int getCurrentTab() { | ||
| return mViewPager.getCurrentItem(); | ||
| } | ||
|
|
||
| public int getTabCount() { | ||
| return mViewPager.getAdapter().getCount(); | ||
| return Objects.requireNonNull(mViewPager.getAdapter()).getItemCount(); |
There was a problem hiding this comment.
@avazirna are we sure mViewPager.getAdapter() can't be null if a caller calls getTabCount out of order on this view ?
| } | ||
| mMenu.getChildAt(position).setBackgroundDrawable(getResources().getDrawable(R.drawable.title_case_tab_vertical)); |
There was a problem hiding this comment.
I believe the default style is enough, but will make sure it looks fine everywhere
There was a problem hiding this comment.
@shubham1g5 Here's the old style to which those drawables where used:
There was a problem hiding this comment.
and how does it look now with the changes in PR ?
There was a problem hiding this comment.
@shubham1g5 my understanding is that this style was used by the old tab system, which was based on the tabbed_detail_menu linear layout. That system was discontinued when PagerTabStrip was added to CC, this is the PR. So this PR doesn't change the style around the tabs, the current and previous styling is what I shared in this comment.
There was a problem hiding this comment.
I think my question here is more around how does this change not effect Case Detail tabs for example given TabbedDetailView is used there and this line of code seems generic to all TabbedDetailView layouts.
There was a problem hiding this comment.
hannn, in the old tab system, we used to add HorizontalMediaView views to tabbed_detail_menu linear layout and apply this style to them, this was removed here. And because we don't add child views to that linear layout anymore, every time we call mMenu.getChildCount() we get 0 and the execution is interrupted. This to say that we haven't been calling this code for quite some time, it's dead code now.
There was a problem hiding this comment.
got it , thanks for confirming.
@shubham1g5 This logic was not changed as such but I agree that we should null check this, also because |
Summary
Poc to fix: SAAS-16035
Product Description
Removed the usage of Duolingo's rtl viewpager as the repository is archived and no longer holds support.
It was observed that the issue was reproducible only in release mode while working fine in debug mode hence we suspected that it might be because outdated viewpager dependency.
In terms of UX, the behaviour almost the same as it was with RtlViewPager, the most notable difference is that when swiping through the tabs the active page doesn't change, only when a tab is selected:
Case Detail
viewpager2_demo.mov
Case Details in landscape mode on large screens
inline_tile_large_screen.mov
Inline Tile
inline_tile.mov
Safety Assurance
Note for QA
This PR brings changes to the tab system used by case details and inline tiles, see videos above. It's important during QA to see if the tabs are working as expected and that right-to-left (RTL) is still supported.