Skip to content

Conversation

@tsic404
Copy link
Contributor

@tsic404 tsic404 commented Dec 10, 2024

Use Qt's new retainWhileLoading to solve the problem of Image loading flickering instead of using a fixed size to scale to the current size

log: as title

Use Qt's new retainWhileLoading to solve the problem of
Image loading flickering instead of using a fixed size
to scale to the current size

log: as title
@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. 变量命名

    • iconScale 更名为 iconSize 可能会引起混淆,因为 iconScale 通常表示图标的大小比例,而 iconSize 更直接地表示图标的大小。建议保持一致的命名规范。
  2. 重复计算

    • WindowIndicatordotWidthdotHeight 计算中,iconSize / 16iconSize / 3 被重复计算了多次。可以考虑将这些计算结果存储在变量中,以避免重复计算。
  3. 绑定表达式

    • windowIndicatoranchors.topMarginanchors.bottomMarginanchors.leftMarginanchors.rightMargin 中使用了 Qt.binding,这可能会导致性能问题。如果这些值不经常变化,可以考虑直接计算并赋值。
  4. 动画中的比例问题

    • 在动画中,iconScale 被用于计算动画的起始和结束值。但是,在动画结束后,iconscale 被设置为 1.0,这可能会导致图标大小不一致。建议在动画结束后,根据 iconSize 动态调整 iconscale
  5. 注释

    • launchAnimation 的计算中,注释 // todo: use icon.height * iconScale is not good 可能表明存在更好的计算方法,但未提供替代方案。建议进一步优化这部分代码。
  6. 性能优化

    • rect 的动画中,originSize 被重复计算。可以考虑在动画开始前计算一次,并在动画过程中使用。
  7. 代码清理

    • onTriggered 中的 taskmanager.Applet.setAppItemWindowIconGeometry 调用可能需要参数验证,以确保传递给函数的值是有效的。
  8. 未使用的代码

    • onIconScaleChanged 事件处理程序被删除,但相关的注释和代码仍然保留。建议清理未使用的代码和注释,以保持代码的整洁。

总体来说,代码的修改提高了可读性和性能,但需要注意变量命名的一致性,以及潜在的重复计算和性能问题。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, 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

@tsic404
Copy link
Contributor Author

tsic404 commented Dec 12, 2024

/forcemerge

@deepin-bot
Copy link

deepin-bot bot commented Dec 12, 2024

This pr force merged! (status: unknown)

@deepin-bot deepin-bot bot merged commit 6798319 into linuxdeepin:master Dec 12, 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.

4 participants