#596: support copy image #601
Conversation
Reviewer's Guide by SourceryThis pull request adds support for copying images to the clipboard. The changes include adding a new 'Copy Image' menu item to the image context menu in DialogBase.cpp, connecting this menu item to a new OnCopyImage callback function, and ensuring proper cleanup of the menu widget. Additionally, the OnCopyImage method is declared in the DialogBase class in DialogBase.h. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @lidaobing - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| gtk_widget_show_all(menu); | ||
|
|
||
| gtk_menu_popup_at_pointer(GTK_MENU(menu), (GdkEvent*)&event); | ||
| g_signal_connect(menu, "hide", G_CALLBACK(gtk_widget_destroy), menu); |
There was a problem hiding this comment.
issue (bug_risk): Potential double-free or use-after-free issue.
Connecting the 'hide' signal to gtk_widget_destroy might lead to a double-free or use-after-free if the menu is already being managed elsewhere. Ensure that this widget is not being destroyed elsewhere in the code.
| @@ -910,4 +915,15 @@ void DialogBase::OnSaveImage(GtkImage* image) { | |||
| gtk_widget_destroy(dialog); | |||
| } | |||
|
|
|||
| void DialogBase::OnCopyImage(GtkImage* image) { | |||
| GdkPixbuf* pixbuf = gtk_image_get_pixbuf(image); | |||
| if (!pixbuf) { | |||
There was a problem hiding this comment.
suggestion: Consider providing more context in the error log.
The error log 'Failed to create pixbuf from image.' could be more informative. Consider including details about the image or the context in which this failure occurred to aid in debugging.
| if (!pixbuf) { | |
| if (!pixbuf) { | |
| LOG_ERROR("Failed to create pixbuf from image. Image pointer: %p", image); | |
| return; |
| return; | ||
| } | ||
|
|
||
| GtkClipboard* clipboard = gtk_clipboard_get(GDK_SELECTION_CLIPBOARD); |
There was a problem hiding this comment.
suggestion: Check for null clipboard.
Consider adding a null check for the clipboard to handle cases where gtk_clipboard_get might fail.
| GtkClipboard* clipboard = gtk_clipboard_get(GDK_SELECTION_CLIPBOARD); | |
| GtkClipboard* clipboard = gtk_clipboard_get(GDK_SELECTION_CLIPBOARD); | |
| if (clipboard == nullptr) { | |
| return; | |
| } |
| @@ -90,6 +90,7 @@ class DialogBase : public SessionAbstract, public sigc::trackable { | |||
| GtkTextChildAnchor* anchor, | |||
| GtkTextBuffer* buffer); | |||
| static void OnSaveImage(GtkImage* self); | |||
| static void OnCopyImage(GtkImage* self); | |||
There was a problem hiding this comment.
suggestion: Consider renaming the parameter for consistency.
The parameter name 'self' is typically used in Python. Consider renaming it to 'image' for consistency with the implementation and other function signatures.
| static void OnCopyImage(GtkImage* self); | |
| static void OnCopyImage(GtkImage* image); |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #601 +/- ##
==========================================
- Coverage 51.44% 51.36% -0.08%
==========================================
Files 64 64
Lines 8333 8344 +11
==========================================
- Hits 4287 4286 -1
- Misses 4046 4058 +12 ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
This pull request introduces a new feature that allows users to copy images to the clipboard directly from the image context menu in the application.