feat: allow bypassing the compositor when fullscreen#844
Open
LiHua000 wants to merge 2 commits intolinuxdeepin:masterfrom
Open
feat: allow bypassing the compositor when fullscreen#844LiHua000 wants to merge 2 commits intolinuxdeepin:masterfrom
LiHua000 wants to merge 2 commits intolinuxdeepin:masterfrom
Conversation
Log: as title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LiHua000 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Reviewer's GuideAdds X11 support for toggling _NET_WM_BYPASS_COMPOSITOR when the main windows enter or exit fullscreen, while ensuring the behavior is disabled on Wayland and wired into both MainWindow and Platform_MainWindow fullscreen flows. Sequence diagram for toggling compositor bypass on fullscreen enter/exitsequenceDiagram
actor User
participant MainWindow
participant Utility
participant X11Server
User->>MainWindow: trigger fullscreen (mipsShowFullScreen)
MainWindow->>MainWindow: start fullscreen animation
MainWindow->>MainWindow: setWindowState(WindowFullScreen)
MainWindow-->>MainWindow: QPropertyAnimation finished
MainWindow->>Utility: setBypassCompositor(winId, true)
Utility->>Utility: check_wayland_env()
alt running_on_X11
Utility->>X11Server: XChangeProperty(_NET_WM_BYPASS_COMPOSITOR = 1)
X11Server-->>Utility: property_set
else running_on_Wayland
Utility-->>MainWindow: return without changes
end
User->>MainWindow: exit fullscreen (requestAction)
MainWindow->>MainWindow: isFullScreen() == true
MainWindow->>Utility: setBypassCompositor(winId, false)
Utility->>Utility: check_wayland_env()
alt running_on_X11
Utility->>X11Server: XChangeProperty(_NET_WM_BYPASS_COMPOSITOR = 0)
X11Server-->>Utility: property_set
else running_on_Wayland
Utility-->>MainWindow: return without changes
end
MainWindow->>MainWindow: clear WindowFullScreen flag
Class diagram for Utility and main window fullscreen integrationclassDiagram
class Utility {
+static QByteArray windowProperty(quint32 WId, xcb_atom_t propAtom, xcb_atom_t typeAtom, quint32 len)
+static QList~xcb_atom_t~ windowNetWMState(quint32 WId)
+static void setWindowProperty(quint32 WId, xcb_atom_t propAtom, xcb_atom_t typeAtom, const void *data, quint32 len, uint8_t format)
+static void setBypassCompositor(quint32 WId, bool bypass)
}
class MainWindow {
+void mipsShowFullScreen()
+void requestAction(ActionFactory::ActionKind actionKind, bool bFromUser)
}
class Platform_MainWindow {
+void mipsShowFullScreen()
+void requestAction(ActionFactory::ActionKind actionKind, bool bFromUser)
}
MainWindow ..> Utility : uses
Platform_MainWindow ..> Utility : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
Utility::setBypassCompositor, usingunsigned longas the property payload for a 32-bitCARDINALcan be architecture-dependent; consider using a fixed-width type likeuint32_t(and adjusting the format parameter if needed) to avoid size mismatches on 64-bit platforms. - The fullscreen compositor-bypass logic is duplicated in both
MainWindowandPlatform_MainWindow; consider extracting a small helper or centralizing this behavior to keep the fullscreen handling consistent and easier to maintain. - In
MainWindow::mipsShowFullScreen, the bypass is set only after the animation finishes, whilePlatform_MainWindow::mipsShowFullScreensets it immediately after callingsetWindowState; verify whether this intentional difference is required or if these code paths should align for predictable behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `Utility::setBypassCompositor`, using `unsigned long` as the property payload for a 32-bit `CARDINAL` can be architecture-dependent; consider using a fixed-width type like `uint32_t` (and adjusting the format parameter if needed) to avoid size mismatches on 64-bit platforms.
- The fullscreen compositor-bypass logic is duplicated in both `MainWindow` and `Platform_MainWindow`; consider extracting a small helper or centralizing this behavior to keep the fullscreen handling consistent and easier to maintain.
- In `MainWindow::mipsShowFullScreen`, the bypass is set only after the animation finishes, while `Platform_MainWindow::mipsShowFullScreen` sets it immediately after calling `setWindowState`; verify whether this intentional difference is required or if these code paths should align for predictable behavior.
## Individual Comments
### Comment 1
<location path="src/common/utility_x11.cpp" line_range="412-413" />
<code_context>
+ return;
+ }
+
+ unsigned long value = bypass ? 1 : 0;
+ XChangeProperty(display, windowId, bypassAtom, cardinalAtom, 32,
+ PropModeReplace, reinterpret_cast<unsigned char*>(&value), 1);
+ XFlush(display);
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Use a fixed-width 32-bit type for the CARDINAL property value
`_NET_WM_BYPASS_COMPOSITOR` is defined as a 32-bit `CARDINAL`, but `unsigned long` can be 64-bit on some platforms while we declare a 32-bit format in `XChangeProperty`. This mismatch can cause ABI/endianness issues. Use a fixed-width 32-bit type, e.g. `uint32_t value = bypass ? 1u : 0u;`, and pass that to `XChangeProperty`.
Suggested implementation:
```cpp
uint32_t value = bypass ? 1u : 0u;
XChangeProperty(display, windowId, bypassAtom, cardinalAtom, 32,
PropModeReplace, reinterpret_cast<unsigned char*>(&value), 1);
XFlush(display);
```
To compile cleanly with `uint32_t`, ensure that either `<cstdint>` or `<stdint.h>` is included in this translation unit (or via a common header). Add the appropriate include near the top of `utility_x11.cpp` if it's not already present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+412
to
+413
| unsigned long value = bypass ? 1 : 0; | ||
| XChangeProperty(display, windowId, bypassAtom, cardinalAtom, 32, |
There was a problem hiding this comment.
suggestion (bug_risk): Use a fixed-width 32-bit type for the CARDINAL property value
_NET_WM_BYPASS_COMPOSITOR is defined as a 32-bit CARDINAL, but unsigned long can be 64-bit on some platforms while we declare a 32-bit format in XChangeProperty. This mismatch can cause ABI/endianness issues. Use a fixed-width 32-bit type, e.g. uint32_t value = bypass ? 1u : 0u;, and pass that to XChangeProperty.
Suggested implementation:
uint32_t value = bypass ? 1u : 0u;
XChangeProperty(display, windowId, bypassAtom, cardinalAtom, 32,
PropModeReplace, reinterpret_cast<unsigned char*>(&value), 1);
XFlush(display);
To compile cleanly with uint32_t, ensure that either <cstdint> or <stdint.h> is included in this translation unit (or via a common header). Add the appropriate include near the top of utility_x11.cpp if it's not already present.
deepin pr auto reviewGit Diff 代码审查报告总体评价这段代码主要实现了在全屏模式下绕过窗口合成器(compositor)的功能，以提升全屏视频播放的性能。修改涉及多个文件，包括 .gitignore、mainwindow.cpp、platform_mainwindow.cpp、utility.h 和 utility_x11.cpp。 1. 语法逻辑审查1.1 mainwindow.cpp 中的逻辑问题void MainWindow::mipsShowFullScreen()
{
// ... 动画设置代码 ...
setWindowState(windowState() | Qt::WindowFullScreen);
connect(pAn, &QPropertyAnimation::finished, this, [=](){
// 全屏时设置绕过合成器
if (windowHandle()) {
Utility::setBypassCompositor(windowHandle()->winId(), true);
}
});
}问题：每次调用 建议：应该使用 1.2 platform_mainwindow.cpp 中的不一致性在 void Platform_MainWindow::mipsShowFullScreen()
{
// ... 其他代码 ...
setWindowState(windowState() | Qt::WindowFullScreen);
setVisible(true);
// 全屏时设置绕过合成器
if (windowHandle()) {
Utility::setBypassCompositor(windowHandle()->winId(), true);
}
}这与 2. 代码质量审查2.1 缺少错误处理在 2.2 魔法数字在 unsigned long value = bypass ? 1 : 0;建议：定义常量或枚举来提高代码可读性： enum BypassCompositorMode {
BypassDisabled = 0,
BypassEnabled = 1
};2.3 代码注释不足
3. 代码性能审查3.1 X11 调用优化在 建议：可以考虑缓存 3.2 动画与合成器设置的时机在 建议：考虑在动画开始前就设置绕过合成器，或者评估动画过程中是否需要合成器。 4. 代码安全审查4.1 潜在的内存安全问题在 XChangeProperty(display, windowId, bypassAtom, cardinalAtom, 32,
PropModeReplace, reinterpret_cast<unsigned char*>(&value), 1);虽然在这个特定情况下是安全的，但建议添加注释说明为什么这种转换是安全的，或者使用更安全的转换方式。 4.2 窗口ID的有效性检查虽然代码中检查了 建议：考虑添加更严格的窗口有效性检查，或者确保在窗口销毁前取消绕过合成器设置。 5. 改进建议5.1 统一全屏处理逻辑建议将全屏处理逻辑提取为一个共同的基类方法，确保 5.2 添加RAII包装器考虑为X11资源创建RAII包装器，确保资源在异常情况下也能正确释放。 5.3 改进后的代码示例// utility.h 中添加
enum BypassCompositorMode {
BypassDisabled = 0,
BypassEnabled = 1
};
// utility_x11.cpp 中改进
void Utility::setBypassCompositor(quint32 WId, bool bypass)
{
if (dmr::utils::check_wayland_env()) {
return;
}
Display *display = QX11Info::display();
if (!display) {
qWarning() << "Failed to get X11 display";
return;
}
Window windowId = static_cast<Window>(WId);
if (windowId == 0) {
qWarning() << "Invalid window ID";
return;
}
// 缓存原子以提高性能
static Atom bypassAtom = None;
static Atom cardinalAtom = None;
if (bypassAtom == None) {
bypassAtom = XInternAtom(display, kAtomNameBypassCompositor, false);
if (bypassAtom == None) {
qWarning() << "Failed to get bypass compositor atom";
return;
}
}
if (cardinalAtom == None) {
cardinalAtom = XInternAtom(display, "CARDINAL", false);
if (cardinalAtom == None) {
qWarning() << "Failed to get cardinal atom";
return;
}
}
unsigned long value = bypass ? BypassEnabled : BypassDisabled;
int result = XChangeProperty(display, windowId, bypassAtom, cardinalAtom, 32,
PropModeReplace, reinterpret_cast<unsigned char*>(&value), 1);
if (result != Success) {
qWarning() << "Failed to set bypass compositor property:" << result;
}
XFlush(display);
}5.4 改进 mainwindow.cpp 中的连接管理// 在 MainWindow 类中添加成员变量
private:
QMetaObject::Connection m_fullScreenAnimationConnection;
// 在 mipsShowFullScreen 中
void MainWindow::mipsShowFullScreen()
{
// ... 动画设置代码 ...
setWindowState(windowState() | Qt::WindowFullScreen);
// 断开之前的连接
if (m_fullScreenAnimationConnection) {
disconnect(m_fullScreenAnimationConnection);
}
// 创建新连接并保存
m_fullScreenAnimationConnection = connect(pAn, &QPropertyAnimation::finished, this, [=](){
// 全屏时设置绕过合成器
if (windowHandle()) {
Utility::setBypassCompositor(windowHandle()->winId(), true);
}
});
}6. 其他建议
这些改进应该能提高代码的健壮性、可维护性和性能，同时保持功能的一致性。 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Log: as title
task: https://pms.uniontech.com/task-view-384659.html
Summary by Sourcery
Allow X11 fullscreen windows to request compositor bypass for performance and restore normal compositing when exiting fullscreen.
New Features: