#610 cant send image in group chat #637
Conversation
Reviewer's Guide by SourceryThis pull request addresses the issue of sending images in group chats. The changes primarily involve refactoring the 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 found some issues that need to be addressed.
Blocking issues:
Here's what I looked at during the review
- 🔴 General issues: 2 blocking issues, 6 other issues
- 🟢 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.
| auto res = | ||
| shared_ptr<ChipData>(new ChipData(MessageContentType::PICTURE, text)); | ||
| res->deleteFileAfterSent = deleteFileAfterSent; |
There was a problem hiding this comment.
suggestion: Consider using make_shared for shared_ptr creation
Using make_shared is generally more efficient and safer than using new with shared_ptr. It performs a single memory allocation for both the control block and the object.
| auto res = | |
| shared_ptr<ChipData>(new ChipData(MessageContentType::PICTURE, text)); | |
| res->deleteFileAfterSent = deleteFileAfterSent; | |
| auto res = make_shared<ChipData>(MessageContentType::PICTURE, text); | |
| res->deleteFileAfterSent = deleteFileAfterSent; |
| @@ -425,7 +426,7 @@ void DialogGroup::BroadcastEnclosureMsg(const vector<FileInfo*>& files) { | |||
| * 向选中的好友广播文本消息. | |||
| * @param msg 文本消息 | |||
| */ | |||
| void DialogGroup::BroadcastTextMsg(const gchar* msg) { | |||
| void DialogGroup::broadcastTextMsg(shared_ptr<MsgPara> para) { | |||
There was a problem hiding this comment.
nitpick: Inconsistent naming convention
The method name broadcastTextMsg does not follow the camelCase convention used elsewhere in the codebase. Consider renaming it to BroadcastTextMsg for consistency.
| gtk_text_buffer_get_bounds(buffer, &start, &end); | ||
| if (gtk_text_iter_equal(&start, &end)) | ||
| gtk_widget_grab_focus(GTK_WIDGET(inputTextviewWidget)); // 为下一次任务做准备 | ||
| if (grpinf->isInputEmpty()) { |
There was a problem hiding this comment.
issue (bug_risk): Potential null pointer dereference
Ensure that grpinf is not null before calling isInputEmpty to avoid potential null pointer dereference.
| /** | ||
| * 封装消息. | ||
| * @param dtlist 数据链表 | ||
| * @return 消息封装包 | ||
| */ | ||
| MsgPara* DialogPeer::PackageMsg(const std::vector<ChipData>& dtlist) { | ||
| MsgPara* DialogPeer::PackageMsg(const vector<shared_ptr<ChipData>>& dtlist) { |
There was a problem hiding this comment.
suggestion (performance): Consider using const reference for parameter
Consider passing const vector<shared_ptr<ChipData>>& dtlist as a const reference to avoid unnecessary copying.
| MsgPara* DialogPeer::PackageMsg(const vector<shared_ptr<ChipData>>& dtlist) { | |
| MsgPara* DialogPeer::PackageMsg(const std::vector<std::shared_ptr<ChipData>>& dtlist) { |
| auto ptr = chipData.data.c_str(); | ||
| switch (chipData.type) { | ||
| bool CoreThread::SendMessage(CPPalInfo pal, shared_ptr<ChipData> chipData) { | ||
| switch (chipData->type) { |
There was a problem hiding this comment.
issue (bug_risk): Null pointer check for chipData
Ensure that chipData is not null before dereferencing it to avoid potential null pointer dereference.
| Command(*this).SendSublayer(sock, pal, IPTUX_MSGPICOPT, | ||
| chipData->data.c_str()); |
There was a problem hiding this comment.
issue (bug_risk): Potential resource leak
Ensure that the socket is closed in case of an exception to avoid potential resource leaks.
| */ | ||
| void DialogBase::FeedbackMsg(const gchar* msg) { | ||
| MsgPara para(this->app->getMe()); | ||
| void DialogBase::FeedbackMsg(shared_ptr<MsgPara> msgPara) { |
There was a problem hiding this comment.
nitpick: Inconsistent naming convention
The method name FeedbackMsg does not follow the camelCase convention used elsewhere in the codebase. Consider renaming it to feedbackMsg for consistency.
| @@ -221,8 +221,7 @@ void TcpData::RecvMsgPic(PalInfo* pal, const char* path) { | |||
| /* 构建消息封装包 */ | |||
| para.stype = MessageSourceType::PAL; | |||
| para.btype = GROUP_BELONG_TYPE_REGULAR; | |||
| ChipData chip(MESSAGE_CONTENT_TYPE_PICTURE, path); | |||
| para.dtlist.push_back(chip); | |||
| para.dtlist.push_back(ChipData::newImgMsg(path, false)); | |||
There was a problem hiding this comment.
suggestion (performance): Consider using emplace_back
Consider using emplace_back instead of push_back to construct the object in place and avoid an unnecessary copy.
| para.dtlist.push_back(ChipData::newImgMsg(path, false)); | |
| para.dtlist.emplace_back(ChipData::newImgMsg(path, false)); |
Summary by Sourcery
This pull request introduces the ability to send image messages in group chats by adding new methods to the ChipData class and refactoring message handling to use shared pointers. It also includes bug fixes for image message sending and updates to the test suite to cover the new functionality.