Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors informational and warning dialog helpers to use non-blocking GTK dialog handling by showing the dialog and destroying it on response instead of using gtk_dialog_run. Sequence diagram for nonblocking info and warning dialogssequenceDiagram
actor User
participant Application
participant UiHelper
participant GTKDialog
Application->>UiHelper: pop_info(parent, format, ...)
activate UiHelper
UiHelper->>GTKDialog: create GtkMessageDialog(parent, message)
UiHelper->>GTKDialog: gtk_message_dialog_set_markup(msg)
UiHelper->>GTKDialog: gtk_window_set_title("Information")
UiHelper->>GTKDialog: g_signal_connect("response", gtk_widget_destroy)
UiHelper->>GTKDialog: gtk_widget_show()
deactivate UiHelper
note right of Application: Application continues
User->>GTKDialog: Clicks response button
GTKDialog-->>GTKDialog: emit response signal
GTKDialog-->>GTKDialog: gtk_widget_destroy(dialog)
destroy GTKDialog
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- If these dialogs are expected to remain modal like they were with
gtk_dialog_run, consider explicitly setting them as modal/transient for the parent (e.g.,gtk_window_set_modal/gtk_window_set_transient_for) to preserve the original interaction behavior. - If the dialog contains additional child widgets (buttons, labels, etc.), you may want to use
gtk_widget_show_all(dialog)instead ofgtk_widget_show(dialog)to ensure all content becomes visible in non-blocking mode.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If these dialogs are expected to remain modal like they were with `gtk_dialog_run`, consider explicitly setting them as modal/transient for the parent (e.g., `gtk_window_set_modal` / `gtk_window_set_transient_for`) to preserve the original interaction behavior.
- If the dialog contains additional child widgets (buttons, labels, etc.), you may want to use `gtk_widget_show_all(dialog)` instead of `gtk_widget_show(dialog)` to ensure all content becomes visible in non-blocking mode.
## Individual Comments
### Comment 1
<location> `src/iptux/UiHelper.cpp:189` </location>
<code_context>
gtk_window_set_title(GTK_WINDOW(dialog), _("Information"));
- gtk_dialog_run(GTK_DIALOG(dialog));
- gtk_widget_destroy(dialog);
+ g_signal_connect(dialog, "response", G_CALLBACK(gtk_widget_destroy), NULL);
+ gtk_widget_show(dialog);
}
</code_context>
<issue_to_address>
**issue (bug_risk):** The response handler signature no longer matches gtk_widget_destroy.
"response" handlers receive (GtkDialog*, gint), but gtk_widget_destroy takes only a GtkWidget*. With this direct connection the gint will be treated as a pointer, causing undefined behavior or crashes. Instead, either use g_signal_connect_swapped with the dialog as user data, or create a wrapper callback matching the response signature that calls gtk_widget_destroy(dialog).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| gtk_window_set_title(GTK_WINDOW(dialog), _("Information")); | ||
| gtk_dialog_run(GTK_DIALOG(dialog)); | ||
| gtk_widget_destroy(dialog); | ||
| g_signal_connect(dialog, "response", G_CALLBACK(gtk_widget_destroy), NULL); |
There was a problem hiding this comment.
issue (bug_risk): The response handler signature no longer matches gtk_widget_destroy.
"response" handlers receive (GtkDialog*, gint), but gtk_widget_destroy takes only a GtkWidget*. With this direct connection the gint will be treated as a pointer, causing undefined behavior or crashes. Instead, either use g_signal_connect_swapped with the dialog as user data, or create a wrapper callback matching the response signature that calls gtk_widget_destroy(dialog).
There was a problem hiding this comment.
Pull request overview
This PR removes the blocking gtk_dialog_run() usage from the pop_info() and pop_warning() helper dialogs, switching them to a signal-driven, non-blocking display pattern.
Changes:
- Replace
gtk_dialog_run()+ immediate destroy with a"response"handler that destroys the dialog. - Show the dialog via
gtk_widget_show()instead of entering a nested modal loop.
| g_signal_connect(dialog, "response", G_CALLBACK(gtk_widget_destroy), NULL); | ||
| gtk_widget_show(dialog); |
There was a problem hiding this comment.
The "response" signal has the signature (GtkDialog*, gint response_id, gpointer), but gtk_widget_destroy takes only a single GtkWidget*. Connecting it directly relies on undefined behavior/calling convention quirks. Use g_signal_connect_swapped(dialog, "response", G_CALLBACK(gtk_widget_destroy), dialog) or a small wrapper callback with the proper signature that destroys the dialog.
| g_signal_connect(dialog, "response", G_CALLBACK(gtk_widget_destroy), NULL); | ||
| gtk_widget_show(dialog); |
There was a problem hiding this comment.
Removing gtk_dialog_run makes this dialog non-blocking; any callers that expect the warning to be shown/acknowledged before continuing will break. For example, Application::onActivate (src/iptux/Application.cpp:221-226) calls pop_warning() and then immediately quits the application, so the warning dialog may never be presented. Either keep pop_warning synchronous (e.g., run a local main loop until response) or refactor callers to perform follow-up actions (like quitting) from the dialog's response handler.
| g_signal_connect(dialog, "response", G_CALLBACK(gtk_widget_destroy), NULL); | |
| gtk_widget_show(dialog); | |
| gtk_dialog_run(GTK_DIALOG(dialog)); | |
| gtk_widget_destroy(dialog); |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #715 +/- ##
=======================================
Coverage 51.75% 51.75%
=======================================
Files 64 64
Lines 8704 8704
=======================================
Hits 4505 4505
Misses 4199 4199 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary by Sourcery
Enhancements: