feat: add move_xembed_window protocol for later use #414
+34
−3
Conversation
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: BLumia 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 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
deepin pr auto reviewGit Diff 代码审查报告总体评估这段代码主要实现了在 Wayland 会话下移动 XEmbed 窗口的功能，通过添加新的 Wayland 协议请求来替代 X11 会话下的直接窗口移动方法。整体实现思路正确，但在代码完整性、错误处理和资源管理方面存在一些问题。 详细审查意见1. 语法逻辑问题
2. 代码质量问题
3. 代码性能问题
4. 代码安全问题
5. 其他问题
改进建议
示例改进代码// 在类初始化时缓存会话类型
class XembedProtocolHandler {
// ...
private:
bool m_isWaylandSession = qgetenv("XDG_SESSION_TYPE") == "wayland";
};
QPoint XembedProtocolHandler::updateEmbedWindowPosForGetInputEvent()
{
if (m_isWaylandSession) {
auto plugin = Plugin::EmbedPlugin::get(window()->windowHandle());
if (!plugin) {
qWarning() << "Failed to get EmbedPlugin";
return QPoint();
}
auto pluginId = plugin->pluginId();
auto itemKey = plugin->itemKey();
if (pluginId.isEmpty() || itemKey.isEmpty()) {
qWarning() << "Invalid plugin_id or item_key";
return QPoint();
}
// 创建回调处理函数
auto callback = plugin->moveXembedWindow(m_containerWid, pluginId, itemKey);
if (!callback) {
qWarning() << "Failed to move xembed window";
return QPoint();
}
// 设置回调处理函数
wl_callback_add_listener(callback, &callbackListener, this);
// 保存回调对象以便后续释放
m_pendingCallbacks.append(callback);
} else {
QPoint p = UTIL->getMousePos();
UTIL->moveX11Window(m_containerWid, p.x(), p.y());
}
// make window normal and above for get input
UTIL->setX11WindowInputShape(m_containerWid, QSize(1, 1));
return p;
}总结这段代码实现了在 Wayland 会话下移动 XEmbed 窗口的功能，但存在未完成的代码实现、错误处理不足、资源管理不明确等问题。建议在提交前完成代码实现，添加必要的错误处理和资源管理，并进行充分的测试。 |
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:
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: