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: some plugin icon issue #608

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

rb-union
Copy link
Contributor

fix: use dci icons replace svg icons

As title.

Log: Use dci icons.
Bug: https://pms.uniontech.com/bug-view-298681.html

fix: unnecessary scaling adjustments

remove unnecessary scaling adjustments.

Log: Fix icon error on High-DPI.
Bug: https://pms.uniontech.com/bug-view-298677.html

remove unnecessary scaling adjustments.

Log: Fix icon error on High-DPI.
Bug: https://pms.uniontech.com/bug-view-298677.html
@deepin-ci-robot
Copy link

deepin pr auto review

代码审查意见:

  1. 代码注释

    • iconwidget.cpprecordiconwidget.cpp中的loadSvg函数中,注释// High-DPI pixmap always support on Qt6可能需要更详细地解释为什么在高DPI下不需要调整比例。
  2. 代码重复

    • iconwidget.cpprecordiconwidget.cpp中的loadSvg函数代码几乎完全相同,可以考虑将这部分代码提取到一个公共函数中,以减少代码重复。
  3. 资源文件路径

    • shotstart.proshotstartrecord.pro中,资源文件的路径被硬编码为/usr/share/dde-dock/icons/dcc-setting/,这可能会导致在不同系统上安装路径不一致的问题。建议使用相对路径或配置文件来管理这些路径。
  4. 资源文件类型

    • 新增的.dci文件类型在res.qrc中未说明其用途,建议在res.qrc文件中添加注释说明。
  5. 安装路径

    • shotstart.proshotstartrecord.pro中,INSTALLS变量包含了shoticon文件,但未说明这些文件的具体用途。建议在INSTALLS变量中添加注释说明。
  6. 文件权限

    • 新增的.dci文件在shotstartrecord.proshotstart.pro中使用了100644权限,这通常表示文件是只读的。如果这些文件需要执行权限,应该使用100755权限。
  7. Qt版本兼容性

    • 注释// High-DPI pixmap always support on Qt6表明代码在高DPI支持上依赖于Qt6。如果项目需要支持Qt5,这部分代码可能需要调整。
  8. 代码风格

    • loadSvg函数中,注释掉的代码// pixmapSize = size* ratio;应该被删除,以保持代码的整洁。

综上所述,代码在功能上没有问题,但存在一些可改进的地方,特别是在代码重复、资源管理、注释和代码风格方面。

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.

+1

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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
Copy link
Contributor Author

/forcemerge

@deepin-bot
Copy link
Contributor

deepin-bot bot commented Jan 10, 2025

This pr force merged! (status: unstable)

@deepin-bot deepin-bot bot merged commit 5b71e9b into linuxdeepin:develop/snipe Jan 10, 2025
8 of 9 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