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: DNotitlebarWindowHelper can't be removed from mapped #270

Merged
merged 1 commit into from
Jan 9, 2025

Conversation

18202781743
Copy link
Contributor

qobject_cast<QWindow*>(window) is returned nullptr if window is destroyed in qt6, and we can see it on tst_QWindow::qobject_castOnDestruction() in tst_qwindow.cpp.

pms: BUG-299239

qobject_cast<QWindow*>(window) is returned nullptr if window is destroyed in qt6, and we can see it on tst_QWindow::qobject_castOnDestruction() in tst_qwindow.cpp.

pms: BUG-299239
@18202781743 18202781743 requested a review from mhduiy January 9, 2025 02:01
@deepin-ci-robot
Copy link
Contributor

deepin pr auto review

代码审查意见:

  1. 类型转换的改进:

    • dnotitlebarwindowhelper_wl.cppdnotitlebarwindowhelper.cpp文件中,将qobject_cast<QWindow*>(parent())更改为static_cast<QWindow*>(parent())。这种更改提高了代码的清晰度和性能,因为static_cast在编译时进行类型检查,而qobject_cast在运行时进行类型检查。如果parent()不是QWindow类型,static_cast会抛出编译错误,而qobject_cast会返回nullptr
  2. 注释的改进:

    • dnotitlebarwindowhelper.cpp文件中,注释// TODO应该被移除或替换为具体的待办事项描述。目前,它没有提供任何有用的信息,可能会对未来的维护者造成混淆。
  3. 代码风格:

    • 虽然代码风格没有明显的错误,但建议保持代码风格的一致性。例如,在dnotitlebarwindowhelper.cpp文件中,Utility::clearWindowProperty函数的调用应该遵循项目中的命名约定,例如使用小驼峰命名法。
  4. 错误处理:

    • dnotitlebarwindowhelper.cpp文件中,当m_window->handle()true时,应该添加错误处理逻辑。如果m_window->handle()true,但Utility::clearWindowProperty函数调用失败,应该记录错误或抛出异常,以便于调试和问题追踪。
  5. 代码可读性:

    • 虽然代码改动较小,但建议在dnotitlebarwindowhelper_wl.cppdnotitlebarwindowhelper.cpp文件中添加一些注释,解释为什么需要进行类型转换,以及mapped.remove函数的用途。这有助于提高代码的可读性和可维护性。

综上所述,代码的改动是合理的,但建议在注释、代码风格和错误处理方面进行一些改进,以提高代码的整体质量和可维护性。

deepin-ci-robot added a commit to linuxdeepin/qt6platform-plugins that referenced this pull request Jan 9, 2025
Synchronize source files from linuxdeepin/qt5platform-plugins.

Source-pull-request: linuxdeepin/qt5platform-plugins#270
@deepin-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 18202781743, mhduiy

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

@18202781743 18202781743 merged commit 259373e into linuxdeepin:master Jan 9, 2025
18 of 20 checks passed
18202781743 pushed a commit to linuxdeepin/qt6platform-plugins that referenced this pull request Jan 9, 2025
Synchronize source files from linuxdeepin/qt5platform-plugins.

Source-pull-request: linuxdeepin/qt5platform-plugins#270
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