-
Notifications
You must be signed in to change notification settings - Fork 40
fix: Fix the issue of copying the same image multiple times #239
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fix the issue of copying the same image multiple times Log: Fix the issue of copying the same image multiple times pms: BUG-342451
Reviewer's guide (collapsed on small PRs)Reviewer's GuideSimplifies clipboard image de-duplication logic to always suppress identical consecutive images, and ensures non-image clipboard updates (files/text) reset the last-image state so they don’t interfere with later image deduplication. Sequence diagram for clipboard image deduplication after fixsequenceDiagram
actor User
participant SystemClipboard
participant ClipboardLoader
participant HistoryStore
User->>SystemClipboard: Copy image A
SystemClipboard->>ClipboardLoader: clipboardChanged event
ClipboardLoader->>ClipboardLoader: srcImage != m_lastImage
ClipboardLoader->>HistoryStore: add image A
ClipboardLoader->>ClipboardLoader: m_lastImage = image A
User->>SystemClipboard: Copy same image A again
SystemClipboard->>ClipboardLoader: clipboardChanged event
ClipboardLoader->>ClipboardLoader: srcImage == m_lastImage
ClipboardLoader->>ClipboardLoader: skip duplicate image
User->>SystemClipboard: Copy file selection
SystemClipboard->>ClipboardLoader: clipboardChanged event
ClipboardLoader->>ClipboardLoader: detect File type
ClipboardLoader->>ClipboardLoader: m_lastImage = empty QImage
ClipboardLoader->>HistoryStore: add file entry
User->>SystemClipboard: Copy image A again
SystemClipboard->>ClipboardLoader: clipboardChanged event
ClipboardLoader->>ClipboardLoader: m_lastImage is empty
ClipboardLoader->>ClipboardLoader: srcImage != m_lastImage
ClipboardLoader->>HistoryStore: add image A
ClipboardLoader->>ClipboardLoader: m_lastImage = image A
Class diagram for ClipboardLoader clipboard handling changesclassDiagram
class ClipboardLoader {
QImage m_lastImage
void doWork(int protocolType)
void handleImageData(QImage srcImage)
void handleFileData()
void handleTextData(QString text)
}
class QImage
ClipboardLoader "1" *-- "1" QImage : uses
%% Behavioral constraints represented structurally
class ClipboardImageLogic {
QImage lastImage
void updateOnImage(QImage newImage)
void clearOnFile()
void clearOnText()
}
ClipboardLoader --> ClipboardImageLogic : delegates dedup behavior
class ClipboardInfo {
int m_type
QDateTime m_createTime
}
class QDateTime
ClipboardLoader --> ClipboardInfo
ClipboardInfo --> QDateTime
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review这段代码的修改主要涉及剪贴板图片去重逻辑的优化。以下是对修改内容的详细审查和改进建议: 1. 代码逻辑审查修改点分析:
潜在问题:
2. 代码质量建议
// 仅通过图片内容判断是否重复,简化了逻辑但可能增加剪贴板更新频率
if (srcImage == m_lastImage) {
qDebug() << "system repeat image";
return;
}
// 在函数开始处统一清空非图片类型的m_lastImage
if (info.m_type != Image) {
m_lastImage = QImage();
}3. 代码性能建议
// 使用更高效的图片比较方式
if (srcImage.size() == m_lastImage.size() &&
srcImage.constBits() != m_lastImage.constBits() &&
srcImage == m_lastImage) {
// 先检查尺寸和内存地址,避免不必要的深度比较
qDebug() << "system repeat image";
return;
}
// 将QStringLiteral("wayland")改为静态常量
static const QLatin1String waylandPlatform("wayland");
if (waylandPlatform != qGuiApp->platformName()) {
// ...
}4. 代码安全建议
if (srcImage.isNull() || m_lastImage.isNull()) {
// 处理空图片情况
return;
}
QMutexLocker locker(&m_imageMutex);
if (srcImage == m_lastImage) {
// ...
}5. 综合改进建议void ClipboardLoader::doWork(int protocolType)
{
// ... 其他代码 ...
if (!info.m_variantImage.isNull()) {
// 添加空值检查
if (srcImage.isNull() || m_lastImage.isNull()) {
return;
}
// 优化后的比较逻辑
if (srcImage.size() == m_lastImage.size() &&
srcImage.constBits() != m_lastImage.constBits() &&
srcImage == m_lastImage) {
qDebug() << "system repeat image";
return;
}
m_lastImage = srcImage; // 更新最后图片记录
}
// 统一处理非图片类型
if (info.m_type != Image) {
m_lastImage = QImage();
}
// ... 其他代码 ...
}总结这次修改简化了图片去重逻辑,但可能引入以下问题:
建议保留部分原有逻辑,特别是时间间隔判断,同时优化图片比较性能。 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've left some high level feedback:
- The new
srcImage == m_lastImagecheck now suppresses repeats unconditionally, removing the previous timestamp and platform constraints; consider whether users may legitimately copy the same image twice and, if so, whether a time-based or event-based threshold is still needed to avoid over-filtering. - The logic to clear
m_lastImagewhen handling non-image clipboard types is duplicated in both the File and Text branches; consider extracting this into a small helper or centralizing the reset to keep the state handling easier to reason about.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `srcImage == m_lastImage` check now suppresses repeats unconditionally, removing the previous timestamp and platform constraints; consider whether users may legitimately copy the same image twice and, if so, whether a time-based or event-based threshold is still needed to avoid over-filtering.
- The logic to clear `m_lastImage` when handling non-image clipboard types is duplicated in both the File and Text branches; consider extracting this into a small helper or centralizing the reset to keep the state handling easier to reason about.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, pengfeixx The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/merge |
Fix the issue of copying the same image multiple times
Log: Fix the issue of copying the same image multiple times
pms: BUG-342451
Summary by Sourcery
Prevent duplicate image entries in the clipboard history while ensuring non-image clipboard content does not interfere with image deduplication.
Bug Fixes: