-
-
Notifications
You must be signed in to change notification settings - Fork 45
Solved crash for older Android versions #3032
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.
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
Conversation
📝 WalkthroughWalkthroughThe change updates the implementation of the Suggested reviewers
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (1)
464-483: Consider using ThreeTenABP as a more complete solution.While the current implementation correctly fixes the crash, for more extensive usage of Java 8 date/time APIs in the future, consider using the ThreeTenABP library. It provides backported functionality of the java.time package for Android versions before API 26.
#!/bin/bash # Check if the app already uses ThreeTenABP grep -r "com.jakewharton.threetenabp" .
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java(2 hunks)
🔇 Additional comments (3)
app/src/org/commcare/android/database/connect/models/ConnectJobRecord.java (3)
16-16: Added import for compatibility implementation.The import for
SimpleDateFormatis necessary for the backward compatibility implementation for older Android versions.
465-472: Good API level check for java.time package.The implementation correctly checks for API level 26 (Android O) which is when the java.time package became available without requiring additional libraries. The DateTimeFormatter approach for Android 8.0+ devices is maintained as it's more efficient.
473-479: Proper fallback implementation for older Android versions.This fallback implementation correctly addresses the crash on Android 6 and 7 by using
SimpleDateFormatandDateinstead of the Java 8 time API. The format patterns match between both implementations, ensuring consistent output.The comment about removing this code when the minSdk changes to 26 is a good practice for future maintenance.
Technical Summary
https://dimagi.atlassian.net/browse/CCCT-971
Feature Flag
Bug fix
Safety Assurance
Safety story
Tested on Android 6 and 7
QA Plan
To test on Android 6 and 7
Labels and Review