Skip to content

Conversation

@JWWTSL
Copy link

@JWWTSL JWWTSL commented Oct 24, 2025

log: When the character is at the cursor position and within a hidden frame, first clear the cell using the cell background colour before proceeding with subsequent rendering. This ensures no outline from the previous frame remains. Extend the redraw area by 1 pixel on all sides to cover edge pixels, preventing border lines from falling outside the redraw region.

felixonmars and others added 3 commits October 24, 2025 13:42
Credits to Antonio Rojas from Arch Linux.
log: When the character is at the cursor position and within a hidden frame, first clear the cell using the cell background colour before proceeding with subsequent rendering. This ensures no outline from the previous frame remains. Extend the redraw area by 1 pixel on all sides to cover edge pixels, preventing border lines from falling outside the redraw region.

bug: https://pms.uniontech.com/bug-view-336141.html
@deepin-ci-robot
Copy link
Contributor

deepin pr auto review

我来对这段代码变更进行审查:

  1. 逻辑和功能改进:
  • 光标绘制逻辑优化:修改了光标闪烁时的绘制行为,在隐藏帧时使用背景色清除光标区域,避免残留问题
  • 光标区域扩展:通过 adjusted(-1, 0, 1, 0) 扩展光标绘制区域,确保完全覆盖
  • 更新区域优化:在 updateCursor() 中增加了 margin,确保光标区域更新更完整
  1. 代码质量问题:
  • 代码重复:在 drawTextFragment() 中有连续两个相同的 if 判断块,这是明显的代码重复,应该合并
  • 注释风格不统一:有些注释使用 //,有些使用 /* */,建议统一使用 //
  • 变量命名:_cursorBlinking 的布尔值判断逻辑可以更清晰
  1. 性能考虑:
  • 频繁的 save()/restore() 操作可能带来性能开销,建议评估是否可以优化
  • 扩展的更新区域可能会增加重绘范围,需要权衡视觉效果和性能
  1. 安全性:
  • painter.fillRect() 使用 adjusted() 扩展区域时没有边界检查,可能导致越界访问

改进建议:

  1. 代码重复问题修复:
// 合并重复的 if 判断
if (!_hideCursor && (style->rendition & RE_CURSOR) && _cursorBlinking)
{
    drawBackground(painter, rect, backgroundColor, false);
}
  1. 光标绘制逻辑优化:
void TerminalDisplay::drawCursor(QPainter& painter,
                               const QRect& rect,
                               const QColor& foregroundColor,
                               const QColor& backgroundColor,
                               bool& invertCharacterColor)
{
    QRectF cursorRect = rect;
    cursorRect.setHeight(_fontHeight);

    if (_cursorBlinking) {
        // 隐藏帧处理
        painter.save();
        painter.setPen(Qt::NoPen);
        // 添加边界检查
        QRect adjustedRect = cursorRect.adjusted(-1, 0, 1, 0);
        if (adjustedRect.isValid()) {
            painter.fillRect(adjustedRect, backgroundColor);
        }
        painter.restore();
        return;
    }

    // 显示帧处理
    if (_cursorColor.isValid()) {
        painter.setPen(_cursorColor);
    } else {
        painter.setPen(foregroundColor);
    }
    
    if (hasFocus()) {
        // 绘制实心光标
        painter.fillRect(cursorRect, painter.pen().color());
        invertCharacterColor = true;
    } else {
        // 绘制光标轮廓
        painter.drawRect(cursorRect);
    }
}
  1. updateCursor() 优化:
void TerminalDisplay::updateCursor()
{
    QRect cursorRect = imageToWidget(QRect(cursorPosition(), QSize(1,1)));
    const int margin = 1;
    // 添加边界检查
    QRect adjustedRect = cursorRect.adjusted(-margin, -margin, margin, margin);
    if (adjustedRect.isValid()) {
        update(adjustedRect);
    }
}
  1. 统一注释风格:
// 使用统一的注释风格
// 光标隐藏帧处理
// 光标显示帧处理
// 背景清除处理

这些改进可以提高代码的可维护性、性能和安全性,同时保持功能的正确性。建议在实施这些更改后进行充分测试,确保不会引入新的问题。

@JWWTSL JWWTSL changed the title Develop/snipe fix: Cursor Display Anomaly Issue Oct 24, 2025
@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JWWTSL, lzwind

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

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

@JWWTSL
Copy link
Author

JWWTSL commented Oct 24, 2025

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Oct 24, 2025

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit 54f4420 into linuxdeepin:develop/snipe Oct 24, 2025
15 checks passed
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.

4 participants