Fix permissions formatting logging 9989925042154523083#114
Fix permissions formatting logging 9989925042154523083#114harrydbarnes wants to merge 14 commits intomainfrom
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Implemented permission result handling in MainActivity to refresh data or show feedback. - Replaced hardcoded SimpleDateFormat with DateUtils in CalendarCard to respect user time format preference. - Added TODO in MainViewModel regarding hardcoded news category. - Replaced e.printStackTrace() with Log.e() in repositories for better error tracking. - Added TAG constants to repositories.
- Implemented permission result handling in MainActivity. - Replaced hardcoded date format with DateUtils in CalendarCard. - Re-implemented dynamic news category selection in MainViewModel. - Added isLoading state to MainViewModel. - Replaced e.printStackTrace() with Log.e() in repositories. - Updated repositories to return Boolean success/failure status. - Implemented transactional replaceAll in ArticleDao/NewsRepository to prevent data loss. - Implemented retry logic in WidgetUpdateWorker based on refresh success. - Created missing colors.xml to fix build failure. - Implemented settings navigation in HomeScreen.
- Implemented permission result handling in MainActivity. - Replaced hardcoded date format with DateUtils in CalendarCard. - Re-implemented dynamic news category selection in MainViewModel. - Added isLoading state to MainViewModel. - Fixed combine function signature in MainViewModel to handle 8 flows. - Replaced e.printStackTrace() with Log.e() in repositories. - Updated repositories to return Boolean success/failure status. - Implemented transactional replaceAll in ArticleDao/NewsRepository to prevent data loss. - Implemented retry logic in WidgetUpdateWorker based on refresh success. - Created missing colors.xml to fix build failure. - Implemented settings navigation in HomeScreen.
…Repository.kt Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Implemented permission result handling in MainActivity. - Replaced hardcoded date format with DateUtils in CalendarCard. - Re-implemented dynamic news category selection in MainViewModel. - Added isLoading state to MainViewModel. - Fixed combine function signature in MainViewModel to handle 8 flows. - Replaced e.printStackTrace() with Log.e() in repositories. - Updated repositories to return Boolean success/failure status. - Implemented transactional replaceAll in ArticleDao/NewsRepository to prevent data loss. - Implemented retry logic in WidgetUpdateWorker based on refresh success. - Created missing colors.xml to fix build failure. - Implemented settings navigation in HomeScreen. - Fixed syntax error in CalendarRepository.
- Implemented permission result handling in MainActivity. - Replaced hardcoded date format with DateUtils in CalendarCard. - Re-implemented dynamic news category selection in MainViewModel. - Added isLoading state to MainViewModel. - Refactored MainViewModel combine logic to use chained calls for type safety. - Replaced e.printStackTrace() with Log.e() in repositories and DayAheadWidget. - Updated repositories to return Boolean success/failure status. - Implemented transactional replaceAll in ArticleDao/NewsRepository. - Implemented retry logic in WidgetUpdateWorker. - Created missing colors.xml to fix build failure. - Implemented settings navigation in HomeScreen. - Fixed CalendarRepository syntax error and extracted magic number. - Added @JvmStatic to AppUtils for Java interop. - Extracted Hilt versions in build.gradle. - Added null/blank title filtering in NewsRepository.
Summary of ChangesHello @harrydbarnes, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant architectural overhaul and modernization of the application. The entire codebase has been migrated from Java to Kotlin, and the user interface has been completely rewritten using Jetpack Compose, replacing the traditional Android View system. This refactoring introduces a robust data layer with Room Persistence Library for local caching and a new repository pattern, alongside Hilt for streamlined dependency injection. These changes aim to improve code maintainability, scalability, and developer experience, while also bringing the UI up to modern Android standards. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request represents a significant and well-executed migration of the application from a legacy Java and XML-based architecture to a modern Kotlin stack utilizing Jetpack Compose, Hilt, Room, and Coroutines. The adoption of a repository pattern, reactive UI with StateFlow, and dependency injection greatly improves the codebase's maintainability, testability, and scalability.
My review highlights a critical regression in location fetching that needs to be addressed to restore core weather functionality. I've also provided a few suggestions to further improve code clarity, adhere to current API best practices, and reduce code duplication. Overall, this is an excellent modernization effort.
| val latStr = prefs.getString(AppConstants.KEY_LATITUDE, AppConstants.DEFAULT_LATITUDE.toString()) | ||
| val lonStr = prefs.getString(AppConstants.KEY_LONGITUDE, AppConstants.DEFAULT_LONGITUDE.toString()) | ||
|
|
||
| val lat = try { latStr?.toDouble() ?: AppConstants.DEFAULT_LATITUDE } catch (e: Exception) { AppConstants.DEFAULT_LATITUDE } | ||
| val lon = try { lonStr?.toDouble() ?: AppConstants.DEFAULT_LONGITUDE } catch (e: Exception) { AppConstants.DEFAULT_LONGITUDE } |
There was a problem hiding this comment.
The application's location fetching logic appears to have been lost in the refactoring. The ViewModel now reads latitude and longitude from SharedPreferences, but there's no mechanism to update these values with the device's current location. This will cause the app to show weather for a stale or default location, which is a critical issue.
You should re-implement location fetching, for example, by using the FusedLocationProviderClient in the MainActivity and passing the location to the ViewModel, or preferably by creating a LocationRepository that provides location updates as a Flow for the ViewModel to consume.
| @Suppress("DEPRECATION") | ||
| val addresses = geocoder.getFromLocation(lat, lon, 1) |
There was a problem hiding this comment.
The getFromLocation method with latitude and longitude is deprecated on API 33 and above. While you've suppressed the warning, it's recommended to migrate to the new asynchronous version which takes a GeocodeListener. This avoids potential IOExceptions and aligns with modern Android practices. You can wrap the callback-based API using suspendCancellableCoroutine to make it fit nicely within your suspend function.
| android.Manifest.permission.READ_CALENDAR | ||
| ) != android.content.pm.PackageManager.PERMISSION_GRANTED | ||
| ) { | ||
| return@withContext true |
There was a problem hiding this comment.
The refreshEvents function returns true when READ_CALENDAR permission is not granted. This can be misleading to the caller, as it implies the operation was successful when in fact no events were fetched. You should return false to indicate that the operation could not be completed due to missing permissions. This allows the caller to handle this state more explicitly.
| return@withContext true | |
| return@withContext false |
| news.business?.filter { !it.title.isNullOrBlank() }?.let { allArticles.addAll(it.map { a -> a.toEntity("Business") }) } | ||
| news.entertainment?.filter { !it.title.isNullOrBlank() }?.let { allArticles.addAll(it.map { a -> a.toEntity("Entertainment") }) } | ||
| news.health?.filter { !it.title.isNullOrBlank() }?.let { allArticles.addAll(it.map { a -> a.toEntity("Health") }) } | ||
| news.science?.filter { !it.title.isNullOrBlank() }?.let { allArticles.addAll(it.map { a -> a.toEntity("Science") }) } | ||
| news.sports?.filter { !it.title.isNullOrBlank() }?.let { allArticles.addAll(it.map { a -> a.toEntity("Sports") }) } | ||
| news.technology?.filter { !it.title.isNullOrBlank() }?.let { allArticles.addAll(it.map { a -> a.toEntity("Technology") }) } | ||
| news.us?.filter { !it.title.isNullOrBlank() }?.let { allArticles.addAll(it.map { a -> a.toEntity("US") }) } | ||
| news.world?.filter { !it.title.isNullOrBlank() }?.let { allArticles.addAll(it.map { a -> a.toEntity("World") }) } |
There was a problem hiding this comment.
The logic for filtering and mapping articles for each category is duplicated. This can be extracted into a private helper function to improve readability and maintainability.
fun addArticles(category: String, articles: List<Article>?) {
articles?.filter { !it.title.isNullOrBlank() }
?.map { it.toEntity(category) }
?.let { allArticles.addAll(it) }
}
addArticles("Business", news.business)
addArticles("Entertainment", news.entertainment)
addArticles("Health", news.health)
addArticles("Science", news.science)
addArticles("Sports", news.sports)
addArticles("Technology", news.technology)
addArticles("US", news.us)
addArticles("World", news.world)
1 participant
No description provided.