feat: add move_xembed_window protocol for later use#414
Merged
BLumia merged 2 commits intolinuxdeepin:masterfrom Jan 20, 2026
Merged
feat: add move_xembed_window protocol for later use#414BLumia merged 2 commits intolinuxdeepin:masterfrom
BLumia merged 2 commits intolinuxdeepin:masterfrom
Conversation
Reviewer's GuideAdds a new Wayland protocol request and callback interface to move XEmbed windows via the plugin manager, exposes this as an asynchronous callback-based API in PluginManagerIntegration, and wires in initial Wayland-specific handling in the Xembed protocol handler (currently with a TODO). Sequence diagram for move_xembed_window request and callbacksequenceDiagram
actor User
participant XembedProtocolHandler
participant EmbedPlugin
participant PluginManagerIntegration
participant WaylandCompositor
participant PluginManagerCallback
User->>XembedProtocolHandler: triggers input on tray icon
XembedProtocolHandler->>XembedProtocolHandler: detect Wayland session
XembedProtocolHandler->>EmbedPlugin: get(windowHandle)
EmbedPlugin-->>XembedProtocolHandler: plugin_id, item_key
XembedProtocolHandler->>PluginManagerIntegration: moveXembedWindow(xembedWinId, plugin_id, item_key)
PluginManagerIntegration->>PluginManagerIntegration: create PluginManagerCallback instance
PluginManagerIntegration->>WaylandCompositor: plugin_manager_v1.move_xembed_window(xembed_winid, plugin_id, item_key, callback)
WaylandCompositor-->>PluginManagerCallback: plugin_manager_callback_v1.done
PluginManagerCallback->>PluginManagerCallback: plugin_manager_callback_v1_done()
PluginManagerCallback-->>PluginManagerCallback: emit done()
Class diagram for PluginManagerIntegration and PluginManagerCallback changesclassDiagram
class PluginManagerIntegration {
+void requestMessage(QString plugin_id, QString item_key, QString msg)
+PluginManagerCallback* moveXembedWindow(uint32_t xembedWinId, QString pluginId, QString itemKey)
+void plugin_manager_v1_position_changed(uint32_t dock_position)
+bool tryCreatePopupForSubWindow(QWindow* window)
-uint32_t m_dockPosition
-uint32_t m_dockColorType
-static PluginManagerIntegration* s_instance
}
class PluginManagerCallback {
+PluginManagerCallback()
+~PluginManagerCallback()
+signal void done()
+void plugin_manager_callback_v1_done()
}
class QObject
class QtWaylandClient_QWaylandShellIntegrationTemplate_PluginManagerIntegration
class QtWayland_plugin_manager_v1
class QtWayland_plugin_manager_callback_v1
PluginManagerIntegration ..|> QtWaylandClient_QWaylandShellIntegrationTemplate_PluginManagerIntegration
PluginManagerIntegration ..|> QtWayland_plugin_manager_v1
PluginManagerCallback ..|> QObject
PluginManagerCallback ..|> QtWayland_plugin_manager_callback_v1
PluginManagerIntegration --> PluginManagerCallback : creates
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The new
moveXembedWindowhelper allocates aPluginManagerCallbackbut never passes acallbackobject to the Waylandmove_xembed_windowrequest (which expects anew_id), so the callback will never be wired up to protocol events; consider using the generated request overload that takes aplugin_manager_callback_v1and binding it to thePluginManagerCallbackinstance. - Ownership and lifetime of
PluginManagerCallbackare unclear:moveXembedWindowreturns a raw pointer that is never parented or deleted, which risks a leak; consider giving it a QObject parent (e.g., the caller orPluginManagerIntegration) or returning a smart pointer. - The Wayland branch in
XembedProtocolHandler::updateEmbedWindowPosForGetInputEventcurrently contains only a TODO and does not use the new protocol/methods, leaving Wayland behavior effectively unchanged; if this is intended to be usable now, wire it up toPluginManagerIntegration::moveXembedWindowwith the retrievedplugin_idanditem_key.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `moveXembedWindow` helper allocates a `PluginManagerCallback` but never passes a `callback` object to the Wayland `move_xembed_window` request (which expects a `new_id`), so the callback will never be wired up to protocol events; consider using the generated request overload that takes a `plugin_manager_callback_v1` and binding it to the `PluginManagerCallback` instance.
- Ownership and lifetime of `PluginManagerCallback` are unclear: `moveXembedWindow` returns a raw pointer that is never parented or deleted, which risks a leak; consider giving it a QObject parent (e.g., the caller or `PluginManagerIntegration`) or returning a smart pointer.
- The Wayland branch in `XembedProtocolHandler::updateEmbedWindowPosForGetInputEvent` currently contains only a TODO and does not use the new protocol/methods, leaving Wayland behavior effectively unchanged; if this is intended to be usable now, wire it up to `PluginManagerIntegration::moveXembedWindow` with the retrieved `plugin_id` and `item_key`.
## Individual Comments
### Comment 1
<location> `src/tray-wayland-integration/pluginmanagerintegration.cpp:51-54` </location>
<code_context>
request_message(plugin_id, item_key, msg);
}
+PluginManagerCallback *PluginManagerIntegration::moveXembedWindow(uint32_t xembedWinId, const QString &pluginId, const QString &itemKey)
+{
+ auto callback = new PluginManagerCallback();
+ move_xembed_window(xembedWinId, pluginId, itemKey);
+ return callback;
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** The Wayland callback object is never wired to the `move_xembed_window` request, so it will never receive `done` events.
Here you allocate a `PluginManagerCallback` but never pass it to the Wayland `move_xembed_window` request, so its `plugin_manager_callback_v1_done()` will never be called. You should use the generated API overload that accepts a `plugin_manager_callback_v1 *` (or equivalent) and construct `PluginManagerCallback` with the proxy returned from `move_xembed_window`, for example:
```cpp
auto callback = new PluginManagerCallback(this, move_xembed_window(xembedWinId, pluginId, itemKey));
return callback;
```
Adjust the exact construction to match your generated bindings, ensuring the `new_id` callback proxy is bound to `PluginManagerCallback`.
</issue_to_address>
### Comment 2
<location> `src/tray-wayland-integration/pluginmanagerintegration_p.h:48-52` </location>
<code_context>
+ static PluginManagerIntegration *s_instance;
};
+
+class PluginManagerCallback : public QObject, public QtWayland::plugin_manager_callback_v1
+{
+ Q_OBJECT
+public:
+ PluginManagerCallback();
+ ~PluginManagerCallback();
+
</code_context>
<issue_to_address>
**issue (bug_risk):** `PluginManagerCallback`'s construction doesn’t initialize the underlying Wayland proxy, which can break the generated base class expectations.
The generated `QtWayland::plugin_manager_callback_v1` is meant to be constructed with a valid `wl_registry`/`wl_proxy` (or via the `new_id` factory), not default-constructed. With the current empty ctor, the base may hold a null/invalid proxy and `plugin_manager_callback_v1_done()` could be called on an unbound object.
To avoid this, consider:
- Removing the public default constructor, and
- Adding a constructor that takes the handle returned for the `callback` `new_id` of `move_xembed_window` (e.g. a `struct ::wl_proxy *` or forwarding to the appropriate base-class ctor),
so the instance is always bound to the correct Wayland object.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
TAG Bot New tag: 2.0.22 |
BLumia
force-pushed
the
protocol-move-xembed-window
branch
from
b90a156 to
058940d
Compare
tsic404
reviewed
Jan 19, 2026
BLumia
force-pushed
the
protocol-move-xembed-window
branch
from
1c2d742 to
8bccff2
Compare
deepin pr auto review这段代码主要是为了在 Wayland 会话下实现 XEmbed 窗口的移动功能，以替代 X11 下直接移动窗口的方式。以下是对这段代码的详细审查和改进建议： 1. 语法逻辑审查问题 1：潜在的资源泄漏 struct ::wl_callback *PluginManagerIntegration::moveXembedWindow(uint32_t xembedWinId, const QString &pluginId, const QString &itemKey)
{
auto callback = move_xembed_window(xembedWinId, pluginId, itemKey);
wl_callback_add_listener(callback, &xembedWindowMovedListener, this);
return callback;
}如果 问题 2：超时处理后的状态 if (timer.isActive()) {
timer.stop();
} else {
qDebug() << "requestMoveXembedWindow() timeout";
}超时后，代码仅打印日志，但未处理窗口可能处于错误位置的状态。 问题 3：静态实例未初始化 2. 代码质量审查问题 1：魔法数字 timer.start(3000); // 3秒超时建议：将超时时间定义为常量，例如： static constexpr int MOVE_WINDOW_TIMEOUT_MS = 3000;
timer.start(MOVE_WINDOW_TIMEOUT_MS);问题 2：日志级别 qWarning() << "requestMoveXembedWindow() timeout after" << MOVE_WINDOW_TIMEOUT_MS << "ms";问题 3：代码重复 3. 代码性能审查问题 1：阻塞主线程 QEventLoop loop;
// ...
loop.exec();使用 问题 2：频繁的环境变量检查 if (qgetenv("XDG_SESSION_TYPE") == "wayland") {如果该函数被频繁调用，每次检查环境变量会有性能开销。 bool XembedProtocolHandler::isWaylandSession() const {
static const bool isWayland = (qgetenv("XDG_SESSION_TYPE") == "wayland");
return isWayland;
}4. 代码安全审查问题 1：空指针解引用风险 auto plugin = Plugin::EmbedPlugin::get(window()->windowHandle());如果 auto handle = window() ? window()->windowHandle() : nullptr;
if (!handle) {
qWarning() << "Failed to get window handle";
return QPoint();
}
auto plugin = Plugin::EmbedPlugin::get(handle);
if (!plugin) {
qWarning() << "Failed to get EmbedPlugin";
return QPoint();
}问题 2：Wayland 协议安全性 5. 改进后的代码示例以下是部分改进后的代码示例： xembedprotocolhandler.cppQPoint XembedProtocolHandler::updateEmbedWindowPosForGetInputEvent()
{
static constexpr int MOVE_WINDOW_TIMEOUT_MS = 3000;
static const bool isWayland = (qgetenv("XDG_SESSION_TYPE") == "wayland");
if (isWayland) {
auto handle = window() ? window()->windowHandle() : nullptr;
if (!handle) {
qWarning() << "Failed to get window handle";
return QPoint();
}
auto plugin = Plugin::EmbedPlugin::get(handle);
if (!plugin) {
qWarning() << "Failed to get EmbedPlugin";
return QPoint();
}
QEventLoop loop;
connect(plugin, &Plugin::EmbedPlugin::xembedWindowMoved, &loop, &QEventLoop::quit);
QTimer timer;
timer.setSingleShot(true);
connect(&timer, &QTimer::timeout, &loop, &QEventLoop::quit);
timer.start(MOVE_WINDOW_TIMEOUT_MS);
plugin->requestMoveXembedWindow(m_containerWid);
loop.exec();
if (!timer.isActive()) {
qWarning() << "requestMoveXembedWindow() timeout after" << MOVE_WINDOW_TIMEOUT_MS << "ms";
// 添加回退逻辑
}
} else {
QPoint p = UTIL->getMousePos();
UTIL->moveX11Window(m_containerWid, p.x(), p.y());
}
UTIL->setX11WindowInputShape(m_containerWid, QSize(1, 1));
return QPoint();
}pluginmanagerintegration.cppstruct ::wl_callback *PluginManagerIntegration::moveXembedWindow(uint32_t xembedWinId, const QString &pluginId, const QString &itemKey)
{
auto callback = move_xembed_window(xembedWinId, pluginId, itemKey);
if (!callback) {
qWarning() << "Failed to move xembed window: move_xembed_window returned nullptr";
return nullptr;
}
wl_callback_add_listener(callback, &xembedWindowMovedListener, this);
return callback;
}总结这段代码在功能上实现了 Wayland 下 XEmbed 窗口的移动，但在错误处理、性能优化和安全性方面还有改进空间。建议优先处理空指针检查和超时后的状态恢复问题，以提高代码的健壮性。 |
tsic404
approved these changes
Jan 20, 2026
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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
3 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:
Summary by Sourcery
Introduce a Wayland protocol and client integration for moving XEmbed windows via the plugin manager, preparing Wayland-specific handling in the application tray.
New Features:
Enhancements: