Skip to content
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

fix: Improve image load policy #136

Merged
merged 1 commit into from
May 8, 2024
Merged

Conversation

rb-union
Copy link
Contributor

@rb-union rb-union commented May 7, 2024

调整图像加载策略,缩略图以实际显示区域动态计算,
而非固定数据长度.
移除无关代码,修改一处创建线程未及时释放.

Log: 修复部分启动问题

@rb-union rb-union force-pushed the fix_perf branch 6 times, most recently from 66f6f5d to 43530ee Compare May 8, 2024 02:59
调整图像加载策略,缩略图以实际显示区域动态计算,
而非固定数据长度.
移除无关代码,修改一处创建线程未及时释放.

Log: 修复部分启动问题
Bug: https://pms.uniontech.com/bug-view-254143.html
@deepin-ci-robot
Copy link

deepin pr auto review

关键摘要:

  • estimatedDisplayCount函数中,使用了Q_UNUSED宏来避免未使用变量的警告,这是一个好的做法,但是应该避免过度使用,只在确实不需要的变量上使用。
  • loadThumbnails函数中,使用了static_cast来确保类型安全,这是一个好的做法,但是应该确保所有使用到的变量都已经进行了适当的类型转换。
  • estimatedDisplayCount函数中,m_estimateDisplayCount的赋值操作被移到了条件判断之外,这样可以避免在统计变化时重复更新变量,这是一个改进。
  • loadThumbnails函数中,index的值被硬编码为-1,这可能是一个逻辑错误,应该检查是否有必要这样做,以及这样做的上下文。
  • loadThumbnails函数中,使用了Q_EMIT来发送信号,这是一个好的做法,但是应该确保信号的发射是安全的,不会因为多个线程的信号发射而产生问题。

是否建议立即修改:

建议修改的地方:

  • estimatedDisplayCount函数中,应该检查m_estimateDisplayCount是否已经足够大,以避免在多次调用时进行不必要的重复计算。
  • loadThumbnails函数中,index的值应该根据实际需要来确定,并且应该有相应的错误处理机制,以确保在index无效时能够正确处理。
  • 应该检查loadThumbnails函数中的appendFunc是否能够正确处理非预期的线程安全性,例如在多线程环境下对共享资源的访问。
  • 考虑在loadThumbnails函数中使用QThreadQRunnable来处理图像加载,以避免潜在的界面卡顿。
  • loadThumbnails函数中,Q_EMIT的使用应该确保在多线程环境下不会因为信号发射顺序而产生问题。

Copy link
Contributor

@myk1343 myk1343 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[是否满足兼容性要求] Y
[是否满足commit提交规范] Y
[是否满足编码规范] Y
[Review结论] Pass
[Fail原因] N/A

@pengfeixx
Copy link
Contributor

[是否满足兼容性要求] Y
[是否满足commit提交规范] Y
[是否满足编码规范] Y
[Review结论] Pass
[Fail原因] N/A

Copy link
Contributor

@starhcq starhcq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/+1

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rb-union, starhcq

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

@rb-union rb-union merged commit 414c1f2 into linuxdeepin:master May 8, 2024
17 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.

5 participants