Skip to content

fix: Changing screen layout might make dock exclusion zone incorrect #960

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.

Sign up for GitHub

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

Jump to bottom
Merged
deepin-bot merged 1 commit into from
Dec 13, 2024
Merged

fix: Changing screen layout might make dock exclusion zone incorrect #960

deepin-bot merged 1 commit into from
Dec 13, 2024

Conversation

@ArchieMeng
Copy link
Contributor

@ArchieMeng ArchieMeng commented Dec 12, 2024

Changing screen layout may not change the geometry of the screen binded to dock. (For example, the screen stay on top or left without resolution changed) So, connect slot to all screens instead.

pms: BUG-292677
Log: Changing screen layout might make dock exclusion zone incorrect

@deepin-ci-robot
Copy link

deepin-ci-robot commented Dec 13, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ArchieMeng, tsic404

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

@ArchieMeng
Copy link
Contributor Author

ArchieMeng commented Dec 13, 2024

/merge

@deepin-bot
Copy link

deepin-bot bot commented Dec 13, 2024

This pr cannot be merged! (status: behind)

@ArchieMeng
fix: Changing screen layout might make dock exclusion zone incorrect
5817801 
Changing screen layout may not change the geometry of the screen binded
to dock. (For example, the screen stay on top or left without resolution
changed) So, connect slot to all screens instead.

pms: BUG-292677
Log: Changing screen layout might make dock exclusion zone incorrect
@ArchieMeng
Copy link
Contributor Author

ArchieMeng commented Dec 13, 2024

/forcemerge

@deepin-ci-robot
Copy link

deepin-ci-robot commented Dec 13, 2024

deepin pr auto review

代码审查意见：

  1. 代码重复：在connect语句中，m_positionChangedTimer和m_exclusionZoneChangedTimer的连接逻辑几乎完全相同，可以考虑将这部分逻辑抽象成一个函数，减少代码重复。

  2. 使用static_cast：在连接信号和槽时使用static_cast将QTimer::start转换为void (QTimer::*)()类型，这种做法是正确的，但可以简化为直接使用&QTimer::start，因为QTimer::start已经是void (QTimer::*)()类型。

  3. 定时器间隔：定时器间隔设置为100毫秒，这个值看起来比较合适，但需要确认这个时间间隔是否满足实际需求，以及是否会对性能产生负面影响。

  4. 信号连接的顺序：在screenChanged槽函数中，先启动了两个定时器，然后又调用了onPositionChanged和onExclusionZoneChanged方法。这种顺序可能会导致定时器启动后立即触发槽函数，如果这不是预期的行为，需要重新考虑信号和槽的连接顺序。

  5. 内存管理：在screenChanged槽函数中，通过disconnect断开了所有屏幕的连接，然后重新连接了当前屏幕的连接。这种做法可能会导致内存泄漏，因为如果nowScreen是之前连接的屏幕，那么之前的连接就不会被断开。应该先检查连接是否已经存在，然后再进行连接和断开操作。

  6. 代码可读性：在screenChanged槽函数中，使用了Lambda表达式来连接screenAdded信号，这种方式可以增加代码的可读性，但需要确保Lambda表达式中的代码逻辑是清晰的。

  7. 注释：代码中缺少对TODO注释的解释，应该添加注释说明为什么需要使用EventFilter来更新位置，以及这个功能尚未实现的原因。

  8. 未使用的连接：代码中注释掉了keyboardInteractivityChanged信号的连接，如果这个功能不再需要，应该完全移除相关的代码。

综上所述，代码在逻辑上基本正确，但存在一些可以改进的地方，以提高代码的可维护性和可读性。

@deepin-bot
Copy link

deepin-bot bot commented Dec 13, 2024

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit debb9a7 into linuxdeepin:master Dec 13, 2024
7 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tsic404 tsic404 tsic404 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ArchieMeng @deepin-ci-robot @tsic404