Skip to content

fix(shot): suppress XRecord relay on normal path to fix selection fli…#852

Closed
dengzhongyuan365-dev wants to merge 0 commit into
linuxdeepin:masterfrom
dengzhongyuan365-dev:bug-fix-5-28
Closed

fix(shot): suppress XRecord relay on normal path to fix selection fli…#852
dengzhongyuan365-dev wants to merge 0 commit into
linuxdeepin:masterfrom
dengzhongyuan365-dev:bug-fix-5-28

Conversation

@dengzhongyuan365-dev

Copy link
Copy Markdown
Member

…cker on HiDPI

The XRecord relay introduced in BUG-330895 uses coordinate comparison (m_currentCursor vs rootX/rootY) to deduplicate events when Qt already handled them via the native path. However on HiDPI screens the two coordinates live in different spaces — mouseEvent->pos() is a widget-local logical coordinate scaled by pixelRatio, while rootX/rootY are physical screen pixels — so the comparison always evaluates to unequal. Every mouse move during drag triggers two repaint() calls (one from Qt native, one from XRecord relay), causing the dashed selection border to flicker.

Replace the coordinate-based dedup with a timestamp approach:

  • mouseMoveEF records m_lastNativeMouseMoveMs on every native MouseMove (relayed events are gated by m_isRelayedEvent flag to avoid polluting)
  • onMouseDrag/onMouseMove only relay when no native event arrived in >50ms

At ~60fps, native events land every ~16ms, so the 50ms threshold completely suppresses relay under normal operation. When a Dock Grab blocks Qt input, the gap exceeds 50ms and relay kicks in automatically.

Log: 修复高分屏截图框选虚线闪烁问题

PMS: BUG-330895

Influence: 高分屏拖拽框选不再闪烁;Dock Grab 场景下 XRecord 补发仍正常工作

@deepin-ci-robot

Copy link
Copy Markdown

deepin pr auto review

你好!我是CodeGeeX。我已经仔细审查了你提供的Git Diff。这次代码变更的核心目的是优化XRecord与Qt原生事件通路的冲突,将原本基于“坐标去重”的补发逻辑,优化为基于“时间阈值(Staleness)”的补发逻辑,以解决Dock Grab阻断原生事件时的问题,并避免正常情况下的双重触发。

整体来看,这个思路非常清晰且有效,但在语法逻辑、代码质量、性能和安全性方面,我有一些改进建议:

1. 语法与逻辑

  • 线程安全问题(严重)
    m_lastNativeMouseMoveMsmouseMoveEF(Qt GUI线程)中更新,而在 onMouseDrag / onMouseMove(可能是XRecord的监听线程,通常非GUI线程)中读取。对 qint64 的非原子读写在不同编译器和架构下可能不是线程安全的,可能导致读到撕裂的数据。
    改进意见:使用 std::atomic<qint64>QAtomicInt(如果精度要求不高),或者确保XRecord回调通过信号槽(队列连接)转发到GUI线程再处理。
  • m_isRelayedEvent 的状态维护风险
    sendSimulatedMouseEvent 中,通过 m_isRelayedEvent = true; ... m_isRelayedEvent = false; 来标记转发事件。如果在 QApplication::sendEvent 执行过程中抛出异常或触发某个槽函数导致提前返回(虽然Qt中不太常见),这个标志位将永远停留在 true,导致 m_lastNativeMouseMoveMs 不再更新。
    改进意见:使用 RAII 机制来管理这个标志位,确保异常安全。

2. 代码质量

  • 时间戳获取方式不统一
    QDateTime::currentMSecsSinceEpoch() 属于 Qt 的时间工具,而此处用于测量极短的时间间隔(50ms阈值)。更好的做法是使用单调时钟,避免系统时间被用户或NTP同步导致逻辑错乱。
    改进意见:使用 QElapsedTimer 或 C++11的 std::chrono::steady_clock
  • 魔法数字提取为常量(已部分完成)
    你已经将 50 提取为了 RELAY_STALENESS_MS,这很好。但 m_isRelayedEventm_lastNativeMouseMoveMs 作为类的成员变量,直接定义在头文件中,缺乏注释说明其生命周期和线程约束。
  • RAII 守卫类:为了解决上述的状态维护风险,建议实现一个简单的守卫类。

3. 代码性能

  • 高频调用下的时间获取开销
    鼠标移动事件是极高频的(每秒可能上百次)。QDateTime::currentMSecsSinceEpoch() 内部需要查询系统时间,开销相对较大。
    改进意见:如果使用 QElapsedTimer 并作为类成员缓存,调用 elapsed()restart() 的开销比查询系统时间要低得多。

4. 代码安全

  • 事件重放攻击/死循环风险
    当前逻辑是:XRecord发现超时 -> 调用 sendSimulatedMouseEvent -> 触发 mouseMoveEF -> 不更新时间戳。
    如果由于某种未知Bug,Qt原生事件完全卡死,XRecord会以极高频率(远超正常60fps)不断补发事件,可能导致CPU占用飙升。
    改进意见:可以考虑在 sendSimulatedMouseEvent 中加入最小发送间隔限制(如 16ms,即约60fps),防止事件风暴。

改进后的代码建议

1. 头文件 main_window.h 修改

#include <QElapsedTimer>
#include <atomic>

// ... 在类定义中 ...

public slots:
    /**
     * @brief 当前光标的位置
     */
    QPoint m_currentCursor;
    
    // 标记当前事件是否为转发事件,避免更新原生时间戳
    bool m_isRelayedEvent = false; 
    
    // 记录上一次原生鼠标移动的时间(使用单调时钟避免系统时间跳变影响)
    // 注意:如果XRecord回调不在GUI线程,需改为 std::atomic<qint64>
    QElapsedTimer m_lastNativeMouseMoveTimer; 
    
    // 如果确认XRecord在独立线程,请使用:
    // std::atomic<qint64> m_lastNativeMouseMoveMs{0};

2. 源文件 main_window.cpp 修改

a) 初始化时间计时器(在构造函数中):

MainWindow::MainWindow(QWidget *parent) 
    : QWidget(parent)
{
    // ...
    m_lastNativeMouseMoveTimer.start();
    // ...
}

b) 异常安全的转发事件函数:

void MainWindow::sendSimulatedMouseEvent(QEvent::Type type, const QPoint &pos,
                                         Qt::MouseButton button, Qt::MouseButtons buttons)
{
    // RAII 守卫,确保即使 sendEvent 内部发生异常或提前返回,标志位也能正确还原
    struct RelayGuard {
        bool& flag;
        RelayGuard(bool& f) : flag(f) { flag = true; }
        ~RelayGuard() { flag = false; }
    };
    RelayGuard guard(m_isRelayedEvent);

    QScopedPointer<QMouseEvent> event(new QMouseEvent(type, pos, button, buttons, Qt::NoModifier));
    QApplication::sendEvent(this, event.data());
}

c) 优化 mouseMoveEF 中的时间戳记录:

int MainWindow::mouseMoveEF(QMouseEvent *mouseEvent, bool &needRepaint)
{
    m_currentCursor = mouseEvent->pos();
    
    // 仅原生事件更新时间戳,转发事件不更新(避免污染判断)
    if (!m_isRelayedEvent) {
        // 使用 restart() 记录当前时间,后续通过 elapsed() 判断间隔
        m_lastNativeMouseMoveTimer.restart(); 
        
        // 如果使用 std::atomic<qint64>,则这样更新:
        // m_lastNativeMouseMoveMs.store(QDateTime::currentMSecsSinceEpoch(), std::memory_order_relaxed);
    }
    
    // ... 后续逻辑不变 ...
}

d) 优化 onMouseDragonMouseMove 中的判断逻辑:

void MainWindow::onMouseDrag(int x, int y)
{
    // ... 前置逻辑不变 ...

    if (status::shot == m_functionType && isFirstPressButton && !isFirstReleaseButton) {
        // 判断是否超过 50ms 阈值
        if (m_lastNativeMouseMoveTimer.elapsed() > RELAY_STALENESS_MS) {
        // 如果使用 std::atomic<qint64>:
        // if (QDateTime::currentMSecsSinceEpoch() - m_lastNativeMouseMoveMs.load(std::memory_order_relaxed) > RELAY_STALENESS_MS) {
            sendSimulatedMouseEvent(QEvent::MouseMove, QPoint(x, y), Qt::NoButton, Qt::LeftButton);
        }
    }
}

void MainWindow::onMouseMove(int x, int y)
{
    // ... 前置逻辑不变 ...

    bool inShotHoverPhase = status::shot == m_functionType && !isFirstPressButton;
    if (!isFirstMove || inShotHoverPhase) {
        if (m_lastNativeMouseMoveTimer.elapsed() > RELAY_STALENESS_MS) {
            sendSimulatedMouseEvent(QEvent::MouseMove, QPoint(x, y), Qt::NoButton, Qt::NoButton);
        }
    }
}

总结

你的核心逻辑从“空间(坐标)去重”升级为“时间(阈值)去重”,非常符合底层事件处理的最佳实践。通过采纳上述建议,可以消除潜在的线程安全隐患、防止系统时间回拨导致的逻辑失效、避免标志位状态泄漏,并提升高频调用下的性能表现

@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengzhongyuan365-dev, lzwind

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

1 similar comment
@deepin-ci-robot

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dengzhongyuan365-dev, lzwind

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants