#604: cop/save image not work under Linux #607
Conversation
Reviewer's Guide by SourceryThis pull request refactors the image context menu handling in the DialogBase class to fix the issue of saving images not working under Linux. The changes include introducing a new method, afterWindowCreated, to initialize the image context menu and updating the OnImageButtonPress, OnSaveImage, and OnCopyImage methods to use class members for the menu and active image. Additionally, the afterWindowCreated method is called in the CreateMainWindow methods of DialogGroup and DialogPeer to ensure proper initialization. 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: 8 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.
| } | ||
|
|
||
| gboolean DialogBase::OnImageButtonPress(DialogBase* self, | ||
| GdkEventButton* event, |
There was a problem hiding this comment.
issue (bug_risk): Pointer dereference without null check
Ensure that event is not null before dereferencing it. Although it is likely that event will always be valid, adding a null check can prevent potential crashes in edge cases.
| GtkEventBox* event_box) { | ||
| if (event.type != GDK_BUTTON_PRESS || event.button != 3) { | ||
| if (event->type != GDK_BUTTON_PRESS || event->button != 3) { |
There was a problem hiding this comment.
suggestion: Magic number usage
Consider defining a constant for the button number 3 to improve code readability and maintainability.
| if (event->type != GDK_BUTTON_PRESS || event->button != 3) { | |
| #define RIGHT_MOUSE_BUTTON 3 | |
| if (event->type != GDK_BUTTON_PRESS || event->button != RIGHT_MOUSE_BUTTON) { |
| @@ -824,20 +842,10 @@ gboolean DialogBase::OnImageButtonPress(DialogBase*, | |||
| LOG_ERROR("image not found in event box."); | |||
| return FALSE; | |||
| } | |||
| self->m_activeImage = GTK_IMAGE(image); | |||
There was a problem hiding this comment.
issue (bug_risk): Potential type safety issue
Ensure that image is indeed a GtkImage before casting. If image can be of different types, consider adding a type check.
|
|
||
| gtk_menu_popup_at_pointer(GTK_MENU(menu), (GdkEvent*)&event); | ||
| g_signal_connect(menu, "hide", G_CALLBACK(gtk_widget_destroy), menu); | ||
| gtk_widget_show_all(GTK_WIDGET(self->m_imagePopupMenu)); |
There was a problem hiding this comment.
issue (bug_risk): Potential null pointer dereference
Ensure that self->m_imagePopupMenu is not null before calling gtk_widget_show_all. Although it is likely initialized, adding a null check can prevent potential crashes.
| void DialogBase::OnSaveImage(GtkImage* image) { | ||
| void DialogBase::OnSaveImage(DialogBase* self) { | ||
| GtkImage* image = self->m_activeImage; | ||
| g_return_if_fail(!!image); |
There was a problem hiding this comment.
suggestion: Double negation usage
Using double negation !!image is unconventional and might be confusing. Consider using image != NULL for clarity.
| g_return_if_fail(!!image); | |
| g_return_if_fail(image != NULL); |
| @@ -109,6 +111,10 @@ class DialogBase : public SessionAbstract, public sigc::trackable { | |||
| int64_t totalsendsize; // 总计待发送大小(包括已发送) | |||
| struct timeval lasktime; // 上一次更新UI的时间 | |||
| guint timersend; // 发送文件界面更新计时器ID | |||
|
|
|||
| private: | |||
| GtkMenu* m_imagePopupMenu = 0; | |||
There was a problem hiding this comment.
suggestion: Use nullptr instead of 0 for pointer initialization
Consider using nullptr instead of 0 for pointer initialization to improve code clarity and type safety.
| GtkMenu* m_imagePopupMenu = 0; | |
| GtkMenu* m_imagePopupMenu = nullptr; |
|
|
||
| private: | ||
| GtkMenu* m_imagePopupMenu = 0; | ||
| GtkImage* m_activeImage = 0; |
There was a problem hiding this comment.
suggestion: Use nullptr instead of 0 for pointer initialization
Consider using nullptr instead of 0 for pointer initialization to improve code clarity and type safety.
| GtkImage* m_activeImage = 0; | |
| GtkImage* m_activeImage = nullptr; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #607 +/- ##
==========================================
+ Coverage 51.37% 51.47% +0.10%
==========================================
Files 64 64
Lines 8345 8354 +9
==========================================
+ Hits 4287 4300 +13
+ Misses 4058 4054 -4 ☔ View full report in Codecov by Sentry. |
Summary by Sourcery
This pull request addresses the issue with the 'Save Image' functionality not working under Linux by properly managing the image popup menu. Additionally, it refactors the image context menu creation to be initialized once after the window is created, enhancing code maintainability.