Fix permissions formatting logging 9989925042154523083#113
Fix permissions formatting logging 9989925042154523083#113harrydbarnes wants to merge 13 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.
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 introduces a comprehensive modernization of the Android application's architecture and UI. It transitions the entire user interface to Jetpack Compose, migrates all Java code to Kotlin, and establishes a robust data management layer using Hilt for dependency injection, Room for local persistence, and a clean repository pattern. These changes aim to enhance the application's maintainability, scalability, and overall developer experience by adopting contemporary Android development 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
|
There was a problem hiding this comment.
Code Review
This pull request is a significant and impressive modernization of the application. It successfully refactors the codebase from a traditional Android View-based architecture with Java to a modern, more maintainable stack using Jetpack Compose for the UI, Kotlin, Hilt for dependency injection, Room for local persistence, and a repository pattern for data handling. The changes are extensive, removing legacy code and introducing best practices like reactive UI with StateFlow and structured data layers. My review focuses on a few areas to further enhance correctness, maintainability, and adherence to best practices, such as improving error handling, dependency management, and ensuring type safety in reactive streams.
| ) { args -> | ||
| val weatherData = args[0] as? WeatherResponse | ||
| @Suppress("UNCHECKED_CAST") | ||
| val newsData = args[1] as List<Article> | ||
| @Suppress("UNCHECKED_CAST") | ||
| val calendarData = args[2] as List<CalendarEvent> | ||
| val funFactData = args[3] as? String | ||
| val locationName = args[4] as String | ||
| val userName = args[5] as String | ||
| val tempUnit = args[6] as String | ||
| val isLoading = args[7] as Boolean |
There was a problem hiding this comment.
The combine operator provides type-safe, individual parameters to its lambda, not an array of Any?. Using an args array with manual casting and @Suppress("UNCHECKED_CAST") is incorrect and unsafe, bypassing the type system and potentially leading to runtime errors if the flow order changes. You should receive the values as separate, typed parameters in the lambda signature for type safety and improved readability.
Here's how you can correct the combine block:
val uiState: StateFlow<MainUiState> = combine(
weather,
news,
events,
_funFact,
_locationName,
_userName,
_tempUnit,
_isLoading
) { weatherData, newsData, calendarData, funFactData, locationName, userName, tempUnit, isLoading ->
val summaryText = if (weatherData != null) {
SummaryUtils.generateSummary(
context,
weatherData,
calendarData,
calendarData.size,
newsData.firstOrNull(),
userName,
false
)
} else null
MainUiState(
weather = weatherData,
newsArticles = newsData,
calendarEvents = calendarData,
funFact = funFactData,
summary = summaryText,
locationName = locationName,
userGreeting = "${SummaryUtils.getTimeBasedGreeting()}, $userName",
tempUnit = tempUnit,
isLoading = isLoading
)
}.onEach { state ->
// ...| implementation "com.google.dagger:hilt-android:2.51.1" | ||
| kapt "com.google.dagger:hilt-compiler:2.51.1" | ||
| implementation 'androidx.hilt:hilt-navigation-compose:1.2.0' | ||
| implementation 'androidx.hilt:hilt-work:1.2.0' | ||
| kapt 'androidx.hilt:hilt-compiler:1.2.0' |
There was a problem hiding this comment.
For better maintainability and to avoid version mismatches, consider defining Hilt versions in variables, similar to how room_version is defined. This makes it easier to update Hilt-related dependencies together and ensures consistency.
// Hilt
def hilt_version = "2.51.1"
def androidx_hilt_version = "1.2.0"
implementation "com.google.dagger:hilt-android:$hilt_version"
kapt "com.google.dagger:hilt-compiler:$hilt_version"
implementation "androidx.hilt:hilt-navigation-compose:$androidx_hilt_version"
implementation "androidx.hilt:hilt-work:$androidx_hilt_version"
kapt "androidx.hilt:hilt-compiler:$androidx_hilt_version"| views.setTextViewText(R.id.widget_temp, "%.0f°".format(current.temperature)) | ||
| views.setImageViewResource(R.id.widget_weather_icon, AppUtils.getWeatherIconResource(current.weatherCode)) | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() |
There was a problem hiding this comment.
Using e.printStackTrace() is not recommended in production code as its output is not sent to Logcat by default, making debugging difficult. It's better to use android.util.Log.e() to log exceptions, which ensures the error message appears in Logcat and can be filtered by a tag.
| e.printStackTrace() | |
| android.util.Log.e("DayAheadWidget", "Error parsing weather JSON", e) |
| val ownerIdx = | ||
| cursor.getColumnIndexOrThrow(CalendarContract.Events.CALENDAR_DISPLAY_NAME) | ||
|
|
||
| while (cursor.moveToNext() && events.size < 5) { // Limit to 5 |
There was a problem hiding this comment.
The number 5 used to limit the number of fetched calendar events is a magic number. It's better to extract it into a named constant to improve readability and make it easier to modify in the future. You can define it in the companion object.
companion object {
private const val TAG = "CalendarRepository"
private const val MAX_EVENTS_TO_FETCH = 5
}
// ... in refreshEvents()
while (cursor.moveToNext() && events.size < MAX_EVENTS_TO_FETCH) { ... }| news.business?.let { allArticles.addAll(it.map { a -> a.toEntity("Business") }) } | ||
| news.entertainment?.let { allArticles.addAll(it.map { a -> a.toEntity("Entertainment") }) } | ||
| news.health?.let { allArticles.addAll(it.map { a -> a.toEntity("Health") }) } | ||
| news.science?.let { allArticles.addAll(it.map { a -> a.toEntity("Science") }) } | ||
| news.sports?.let { allArticles.addAll(it.map { a -> a.toEntity("Sports") }) } | ||
| news.technology?.let { allArticles.addAll(it.map { a -> a.toEntity("Technology") }) } | ||
| news.us?.let { allArticles.addAll(it.map { a -> a.toEntity("US") }) } | ||
| news.world?.let { allArticles.addAll(it.map { a -> a.toEntity("World") }) } |
There was a problem hiding this comment.
The Article.toEntity() function defaults a null title to an empty string, which can lead to storing and displaying articles without a title. To improve data quality, it's better to filter out articles that have a null or blank title before mapping them to entities.
For example, for business news, you could do:
news.business
?.filter { !it.title.isNullOrBlank() }
?.map { it.toEntity("Business") }
?.let { allArticles.addAll(it) }This should be applied to all news categories.
No description provided.