Skip to content

Conversation

@BLumia
Copy link
Member

@BLumia BLumia commented Dec 23, 2024

修复拖拽任务栏上的应用图标时,图标拖拽位置和实际位置可能错位的问题。

@BLumia BLumia requested a review from tsic404 December 23, 2024 05:20
修复拖拽任务栏上的应用图标时,图标拖拽位置和实际位置可能错位的问题。

PMS: BUG-296961
Log:
@BLumia BLumia force-pushed the dock-taskmanager-dnd branch from 34e4e78 to e556acf Compare December 23, 2024 07:05
@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. 代码重复

    • implicitWidthimplicitHeight的计算中,visualModel.cellWidth被重复使用。建议将visualModel.cellWidth的计算提取到一个单独的函数中,以避免重复代码。
  2. 硬编码比例

    • 注释中提到的“1:4 the distance between app : dock height; get width/height≈0.8”是一个硬编码的比例。建议将这个比例作为一个可配置的属性,以便于后续的调整和维护。
  3. 性能优化

    • onPositionChangedonDropped函数中,Screen.devicePixelRatio被多次调用。如果这个值在短时间内不会改变,建议将其缓存起来,避免重复计算。
  4. 逻辑清晰度

    • onEntered函数中的逻辑可以进一步优化,以减少对visualModel.items.move的调用次数。例如,可以在拖动开始时记录下拖动的起始位置,然后在拖动过程中进行比较,只有在位置发生变化时才调用move函数。
  5. 注释清晰度

    • 注释中的“1:4 the distance between app : dock height; get width/height≈0.8”可能不够清晰,建议提供更多的上下文信息,例如这个比例是如何得出的,以及它的具体用途。
  6. 代码风格

    • 代码中存在一些不一致的地方,例如在onPositionChangedonDropped函数中,let关键字的使用不一致。建议在整个项目中保持一致的代码风格。
  7. 错误处理

    • onPositionChangedonDropped函数中,没有对taskmanager.Applet.dataModel.moveTo的返回值进行检查。建议添加错误处理逻辑,以处理可能出现的异常情况。
  8. 国际化

    • 注释中的“1:4 the distance between app : dock height; get width/height≈0.8”可能需要根据不同的语言进行翻译,以提供更好的用户体验。

以上是针对代码的审查意见,希望能够对您有所帮助。

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

@BLumia BLumia merged commit cda9b5e into linuxdeepin:master Dec 23, 2024
7 of 10 checks passed
@BLumia BLumia deleted the dock-taskmanager-dnd branch December 23, 2024 07:37
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