-
Notifications
You must be signed in to change notification settings - Fork 473
live update permission toggle #376
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
…n settings Change-Id: I822f4d1d8ad1b347ed53420fe3c8404958261d49
Change-Id: Iaf5acbaffa2d098f1833a99a35e9052cb5f5b769
Summary of ChangesHello @alabiaga, 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 implements a feature that allows users to manage "Live Updates" notification permissions more easily. It introduces a new UI component in the 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully adds a user-facing message and a toggle to enable live update permissions, guiding them to the system settings. The implementation is solid, including updates to the AndroidManifest and the creation of a new Composable to handle the permission check. I've included a few suggestions to enhance the code, focusing on improving user navigation behavior and adhering to common Kotlin and Compose coding conventions.
| val isPostPromotionsEnabled = remember { mutableStateOf(SnackbarNotificationManager.isPostPromotionsEnabled()) } | ||
| LifecycleEventEffect(Lifecycle.Event.ON_RESUME) { | ||
| isPostPromotionsEnabled.value = SnackbarNotificationManager.isPostPromotionsEnabled() | ||
| } | ||
| if (!isPostPromotionsEnabled.value) { |
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.
For better readability and to follow idiomatic Compose conventions, you can use a property delegate (by) for your MutableState. This will allow you to access and modify the value directly without needing to use .value.
To use this, you may need to add import androidx.compose.runtime.getValue and import androidx.compose.runtime.setValue.
| val isPostPromotionsEnabled = remember { mutableStateOf(SnackbarNotificationManager.isPostPromotionsEnabled()) } | |
| LifecycleEventEffect(Lifecycle.Event.ON_RESUME) { | |
| isPostPromotionsEnabled.value = SnackbarNotificationManager.isPostPromotionsEnabled() | |
| } | |
| if (!isPostPromotionsEnabled.value) { | |
| var isPostPromotionsEnabled by remember { mutableStateOf(SnackbarNotificationManager.isPostPromotionsEnabled()) } | |
| LifecycleEventEffect(Lifecycle.Event.ON_RESUME) { | |
| isPostPromotionsEnabled = SnackbarNotificationManager.isPostPromotionsEnabled() | |
| } | |
| if (!isPostPromotionsEnabled) { |
| Button( | ||
| onClick = { | ||
| val intent = Intent(Settings.ACTION_APP_NOTIFICATION_PROMOTION_SETTINGS).apply { | ||
| flags = Intent.FLAG_ACTIVITY_NEW_TASK |
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.
Using Intent.FLAG_ACTIVITY_NEW_TASK is not ideal here. Since LocalContext.current within a Composable hosted by an Activity will typically be that Activity's context, starting the settings activity in a new task can disrupt the user's navigation flow. When they press the back button from the settings screen, they might not return to your app as expected. It's better to launch the settings activity in the same task by removing this flag.
| fun isPostPromotionsEnabled(): Boolean { | ||
| return notificationManager.canPostPromotedNotifications() | ||
| } |
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.
The code inside this function is not correctly indented. Following standard Kotlin formatting guidelines improves code readability and maintainability.
| fun isPostPromotionsEnabled(): Boolean { | |
| return notificationManager.canPostPromotedNotifications() | |
| } | |
| fun isPostPromotionsEnabled(): Boolean { | |
| return notificationManager.canPostPromotedNotifications() | |
| } |
Change-Id: I2db246e697aa54aebba466ae96cb1b5321b7ec72
Add message/toggle to allow user to enable live update permission in settings