Conversation
Summary of ChangesHello, 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 enhances the user experience by making the application's restart dialog more informative. Instead of a simple confirmation, the dialog now explicitly lists which changed preferences necessitate an application restart. This change provides greater transparency to the user about the impact of their preference modifications, particularly for platform-specific settings like V-Sync on macOS. Highlights
Changelog
Activity
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 refactors the restart confirmation dialog to dynamically list which preferences require an application restart, which is a good improvement over hardcoded logic. The change from tracing::error! to tracing::info! for application restarts is also appropriate. I have one minor suggestion to make the code for collecting restart-requiring preferences slightly more direct.
| let mut requiring_restart = Vec::new(); | ||
| if self.disable_ui_acceleration != other.disable_ui_acceleration { | ||
| requiring_restart.push("Disable UI Acceleration"); | ||
| } | ||
| #[cfg(target_os = "macos")] | ||
| if self.vsync != other.vsync { | ||
| requiring_restart.push("Enable V-Sync"); | ||
| } | ||
| requiring_restart.into_iter().map(String::from).collect() |
There was a problem hiding this comment.
This implementation can be made more direct by creating a Vec<String> from the beginning and pushing allocated strings, avoiding the final conversion step. This is slightly more efficient and arguably clearer.
| let mut requiring_restart = Vec::new(); | |
| if self.disable_ui_acceleration != other.disable_ui_acceleration { | |
| requiring_restart.push("Disable UI Acceleration"); | |
| } | |
| #[cfg(target_os = "macos")] | |
| if self.vsync != other.vsync { | |
| requiring_restart.push("Enable V-Sync"); | |
| } | |
| requiring_restart.into_iter().map(String::from).collect() | |
| let mut requiring_restart = Vec::new(); | |
| if self.disable_ui_acceleration != other.disable_ui_acceleration { | |
| requiring_restart.push("Disable UI Acceleration".to_string()); | |
| } | |
| #[cfg(target_os = "macos")] | |
| if self.vsync != other.vsync { | |
| requiring_restart.push("Enable V-Sync".to_string()); | |
| } | |
| requiring_restart |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 6 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="editor/src/messages/dialog/preferences_dialog/preferences_dialog_message_handler.rs">
<violation number="1" location="editor/src/messages/dialog/preferences_dialog/preferences_dialog_message_handler.rs:31">
P2: This restart-setting list is computed from a stale baseline because `unmodified_preferences` is never reset when the Preferences dialog is reopened. After a previous visit captured a snapshot, later confirmations can show the wrong settings as requiring restart.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| PreferencesDialogMessage::Confirm => { | ||
| if let Some(unmodified_preferences) = &self.unmodified_preferences | ||
| && unmodified_preferences.needs_restart(preferences) | ||
| && let preferences_requiring_restart = unmodified_preferences.preferences_requiring_restart(preferences) |
There was a problem hiding this comment.
P2: This restart-setting list is computed from a stale baseline because unmodified_preferences is never reset when the Preferences dialog is reopened. After a previous visit captured a snapshot, later confirmations can show the wrong settings as requiring restart.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At editor/src/messages/dialog/preferences_dialog/preferences_dialog_message_handler.rs, line 31:
<comment>This restart-setting list is computed from a stale baseline because `unmodified_preferences` is never reset when the Preferences dialog is reopened. After a previous visit captured a snapshot, later confirmations can show the wrong settings as requiring restart.</comment>
<file context>
@@ -28,9 +28,10 @@ impl MessageHandler<PreferencesDialogMessage, PreferencesDialogMessageContext<'_
PreferencesDialogMessage::Confirm => {
if let Some(unmodified_preferences) = &self.unmodified_preferences
- && unmodified_preferences.needs_restart(preferences)
+ && let preferences_requiring_restart = unmodified_preferences.preferences_requiring_restart(preferences)
+ && !preferences_requiring_restart.is_empty()
{
</file context>
1 participant
No description provided.