Skip to content

Conversation

@yixinshark
Copy link
Contributor

…sks plugin can not hide

as title

Log: as title
Bug: https://pms.uniontech.com/bug-view-286871.html

@yixinshark yixinshark requested a review from tsic404 November 28, 2024 07:31
@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. 变量初始化

    • MultiTaskView类的构造函数中,m_kWinEffect变量被初始化为true,但在visible函数中,它被用来决定MultiTaskView是否可见。如果m_kWinEffect的初始值不正确,可能会导致MultiTaskView在启动时不可见。建议在visible函数中添加注释说明m_kWinEffect的用途。
  2. 连接信号和槽

    • 在构造函数中,m_kWinCompositingConfigvalueChanged信号与一个lambda表达式连接。这个lambda表达式在windowEffectTypeKey的值发生变化时更新m_kWinEffect并发射visibleChanged信号。这个逻辑是正确的,但建议在连接信号和槽之前,先检查m_kWinCompositingConfig是否为nullptr,以避免潜在的空指针解引用问题。
  3. 代码可读性

    • visible函数中,m_kWinEffect变量被用来决定MultiTaskView是否可见。这个逻辑可以通过重命名变量来提高代码的可读性,例如将m_kWinEffect重命名为isKWinEffectEnabled
  4. 资源管理

    • m_kWinCompositingConfig是一个指向DConfig的指针,应该在适当的时候进行资源释放。建议在MultiTaskView的析构函数中添加对m_kWinCompositingConfig的删除操作,以避免内存泄漏。
  5. 常量命名

    • KWinOptimalPerformance常量应该使用全大写字母和下划线分隔,以符合C++的命名约定。例如,可以将其命名为KWIN_OPTIMAL_PERFORMANCE
  6. 代码重复

    • visible函数中,m_kWinEffectm_visible的检查顺序可能会影响逻辑。建议先检查m_visible,然后再检查m_kWinEffect,以避免在m_visiblefalse时执行不必要的检查。

综上所述,建议对代码进行如下修改:

  • MultiTaskView类的构造函数中,检查m_kWinCompositingConfig是否为nullptr
  • visible函数中,先检查m_visible,然后再检查m_kWinEffect
  • m_kWinEffect重命名为isKWinEffectEnabled以提高代码可读性。
  • MultiTaskView的析构函数中添加对m_kWinCompositingConfig的删除操作。
  • KWinOptimalPerformance常量命名为KWIN_OPTIMAL_PERFORMANCE,以符合C++的命名约定。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

@yixinshark
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Nov 29, 2024

This pr force merged! (status: blocked)

@deepin-bot deepin-bot bot merged commit 2087226 into linuxdeepin:master Nov 29, 2024
7 of 10 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.

3 participants