Fix permissions formatting logging 9989925042154523083#115
Fix permissions formatting logging 9989925042154523083#115harrydbarnes wants to merge 15 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.
- 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. - Implemented location fetching in MainActivity and updated MainViewModel. - Updated Geocoder usage for API 33+.
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 delivers a comprehensive modernization of the Android application's architecture and UI. By migrating to Jetpack Compose, integrating Hilt for dependency injection, and implementing a robust data layer with Room and the Repository pattern, the project now adheres to contemporary Android development standards. These changes aim to enhance the application's scalability, maintainability, and overall developer experience, setting a strong foundation for future feature development. 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 is an impressive and large-scale refactoring of the application. You've successfully migrated from a traditional View-based architecture to a modern one using Jetpack Compose, Kotlin, Hilt for dependency injection, and a repository pattern with Room for data persistence. The new architecture is much cleaner, more maintainable, and follows current Android best practices. I have a couple of suggestions to further improve the code, one related to a potential race condition and another for architectural consistency.
| private suspend fun fetchLocationName(lat: Double, lon: Double) { | ||
| if (Geocoder.isPresent()) { | ||
| withContext(Dispatchers.IO) { | ||
| try { | ||
| val geocoder = Geocoder(context, java.util.Locale.getDefault()) | ||
| if (android.os.Build.VERSION.SDK_INT >= android.os.Build.VERSION_CODES.TIRAMISU) { | ||
| geocoder.getFromLocation(lat, lon, 1) { addresses -> | ||
| if (addresses.isNotEmpty()) { | ||
| val address = addresses[0] | ||
| val name = address.locality ?: address.subAdminArea ?: "Unknown Location" | ||
| _locationName.value = name | ||
| } | ||
| } | ||
| } else { | ||
| @Suppress("DEPRECATION") | ||
| val addresses = geocoder.getFromLocation(lat, lon, 1) | ||
| if (!addresses.isNullOrEmpty()) { | ||
| val address = addresses[0] | ||
| val name = address.locality ?: address.subAdminArea ?: "Unknown Location" | ||
| _locationName.value = name | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| _locationName.value = "Unknown Location" | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a potential race condition in fetchLocationName. The withContext(Dispatchers.IO) block will complete before the asynchronous geocoder.getFromLocation callback is executed on Android Tiramisu (API 33) and above. This can lead to the UI not updating with the correct location name in time because the coroutine job will finish prematurely.
To fix this, you should wrap the callback-based API in a coroutine builder like suspendCancellableCoroutine to make it a truly suspending call that waits for the callback to complete.
| @Inject | ||
| lateinit var weatherDao: WeatherDao | ||
|
|
||
| @Inject | ||
| lateinit var gson: Gson | ||
|
|
||
| override fun onUpdate(context: Context, appWidgetManager: AppWidgetManager, appWidgetIds: IntArray) { | ||
| val pendingResult = goAsync() | ||
|
|
||
| CoroutineScope(Dispatchers.IO).launch { | ||
| try { | ||
| val weatherEntity = weatherDao.getWeather() | ||
| val summary = context.getSharedPreferences(AppConstants.PREFS_NAME, Context.MODE_PRIVATE) | ||
| .getString(AppConstants.KEY_SUMMARY_CACHE, context.getString(R.string.widget_default_summary)) | ||
|
|
||
| for (appWidgetId in appWidgetIds) { | ||
| updateAppWidget(context, appWidgetManager, appWidgetId, weatherEntity?.json, summary, gson) | ||
| } | ||
| } catch (e: Exception) { | ||
| android.util.Log.e("DayAheadWidget", "Error in onUpdate", e) | ||
| } finally { | ||
| pendingResult.finish() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private fun updateAppWidget( | ||
| context: Context, | ||
| appWidgetManager: AppWidgetManager, | ||
| appWidgetId: Int, | ||
| weatherJson: String?, | ||
| summary: String?, | ||
| gson: Gson | ||
| ) { | ||
| val views = RemoteViews(context.packageName, R.layout.widget_day_ahead) | ||
| views.setTextViewText(R.id.widget_summary, summary) | ||
|
|
||
| if (weatherJson != null) { | ||
| try { | ||
| val weather = gson.fromJson(weatherJson, WeatherResponse::class.java) | ||
| val current = weather.current | ||
| views.setTextViewText(R.id.widget_temp, "%.0f°".format(current.temperature)) | ||
| views.setImageViewResource(R.id.widget_weather_icon, AppUtils.getWeatherIconResource(current.weatherCode)) | ||
| } catch (e: Exception) { | ||
| android.util.Log.e("DayAheadWidget", "Error parsing weather JSON", e) | ||
| } | ||
| } | ||
|
|
||
| appWidgetManager.updateAppWidget(appWidgetId, views) | ||
| } | ||
| } |
There was a problem hiding this comment.
This class can be simplified by adhering to the repository pattern you've established elsewhere. By injecting WeatherRepository instead of WeatherDao and Gson, you can remove the data-access and parsing logic from this UI component, making it cleaner and more consistent with the rest of your architecture.
@Inject
lateinit var weatherRepository: WeatherRepository
override fun onUpdate(context: Context, appWidgetManager: AppWidgetManager, appWidgetIds: IntArray) {
val pendingResult = goAsync()
CoroutineScope(Dispatchers.IO).launch {
try {
// Fetch parsed weather data from the repository
val weather = weatherRepository.weatherData.first()
val summary = context.getSharedPreferences(AppConstants.PREFS_NAME, Context.MODE_PRIVATE)
.getString(AppConstants.KEY_SUMMARY_CACHE, context.getString(R.string.widget_default_summary))
for (appWidgetId in appWidgetIds) {
updateAppWidget(context, appWidgetManager, appWidgetId, weather, summary)
}
} catch (e: Exception) {
android.util.Log.e("DayAheadWidget", "Error in onUpdate", e)
} finally {
pendingResult.finish()
}
}
}
private fun updateAppWidget(
context: Context,
appWidgetManager: AppWidgetManager,
appWidgetId: Int,
weather: WeatherResponse?,
summary: String?
) {
val views = RemoteViews(context.packageName, R.layout.widget_day_ahead)
views.setTextViewText(R.id.widget_summary, summary)
if (weather != null) {
try {
val current = weather.current
views.setTextViewText(R.id.widget_temp, "%.0f°".format(current.temperature))
views.setImageViewResource(R.id.widget_weather_icon, AppUtils.getWeatherIconResource(current.weatherCode))
} catch (e: Exception) {
android.util.Log.e("DayAheadWidget", "Error processing weather data", e)
}
}
appWidgetManager.updateAppWidget(appWidgetId, views)
}
No description provided.