refactor: migrate foreign-toplevel protocol to qtwayland scanner#911
Open
zorowk wants to merge 4 commits into
Open
refactor: migrate foreign-toplevel protocol to qtwayland scanner#911zorowk wants to merge 4 commits into
zorowk wants to merge 4 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zorowk 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 GuideRefactors the foreign-toplevel Wayland protocol implementation to use the QtWayland QWaylandServer code generator, inlining the previous impl layer into a new ForeignToplevelManagerInterfaceV1 and reworking dock-preview and toplevel-handle handling while updating shell and QML integration accordingly. Sequence diagram for dock preview interaction via ForeignToplevelManagerInterfaceV1sequenceDiagram
actor User
participant DockPreviewQml as DockPreview
participant ShellHandler
participant Manager as ForeignToplevelManagerInterfaceV1
participant Context as DockPreviewContextV1
participant ContextPriv as DockPreviewContextV1Private
User->>DockPreviewQml: hover over dock item
DockPreviewQml->>Manager: enterDockPreview(relativeSurface)
Manager->>Context: enter()
note over ContextPriv,ContextPriv: client later requests preview
ContextPriv->>Context: show(resource, surfaces, x, y, direction)
Context->>Manager: requestShow(event)
Manager->>Manager: handleDockPreviewShow(event)
Manager->>ShellHandler: requestDockPreview(surfaces, target, pos, direction)
ShellHandler->>DockPreviewQml: onDockPreview(...)
User-->>DockPreviewQml: leaves dock item
DockPreviewQml->>Manager: leaveDockPreview(relativeSurface)
Manager->>Context: leave()
File-Level Changes
Possibly linked issues
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:
- In ForeignToplevelManagerInterfaceV1::addSurface, a single ForeignToplevelHandleV1 per SurfaceWrapper is stored in d->m_surfaces even though you create one wl_resource per manager_resource; this means multiple client-specific handles for the same wrapper overwrite each other, so consider tracking handles per (SurfaceWrapper, client) or otherwise ensuring the map accurately represents all active toplevel handles.
- PreviewDirection now uses plain 0/1/2/3 constants instead of the protocol enums (TREELAND_DOCK_PREVIEW_CONTEXT_V1_DIRECTION_*); to avoid subtle mismatches between the QML-facing enum and the wire protocol, it would be safer to keep these values derived directly from the protocol enums or provide an explicit mapping function.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In ForeignToplevelManagerInterfaceV1::addSurface, a single ForeignToplevelHandleV1 per SurfaceWrapper is stored in d->m_surfaces even though you create one wl_resource per manager_resource; this means multiple client-specific handles for the same wrapper overwrite each other, so consider tracking handles per (SurfaceWrapper, client) or otherwise ensuring the map accurately represents all active toplevel handles.
- PreviewDirection now uses plain 0/1/2/3 constants instead of the protocol enums (TREELAND_DOCK_PREVIEW_CONTEXT_V1_DIRECTION_*); to avoid subtle mismatches between the QML-facing enum and the wire protocol, it would be safer to keep these values derived directly from the protocol enums or provide an explicit mapping function.
## Individual Comments
### Comment 1
<location path="src/modules/foreign-toplevel/foreigntoplevelmanagerv1.h" line_range="34" />
<code_context>
+};
+
+class DockPreviewContextV1Private;
+class DockPreviewContextV1 : public QObject
+{
+ Q_OBJECT
</code_context>
<issue_to_address>
**issue (complexity):** Consider restructuring the new foreign-toplevel API so that wl/protocol details are hidden behind private implementations and smaller headers, leaving a slimmer, higher-level public surface.
You can reduce the added complexity without changing behavior by tightening the public surface and pushing protocol/wl details into private/PIMPL classes and cpp files. Concretely:
---
### 1. Split the monolithic header and hide protocol structs
`DockPreviewContextV1`, `ForeignToplevelHandleV1`, `ForeignToplevelManagerInterfaceV1`, and all `treeland_*` structs don’t need to live in one public header.
**Actionable suggestion**
Create smaller headers and move the protocol structs into the corresponding cpp, exposing only high-level types in the header:
```cpp
// dockpreviewcontextv1.h (public)
class DockPreviewContextV1Private;
class DockPreviewContextV1 : public QObject {
Q_OBJECT
public:
~DockPreviewContextV1() override;
WSurface *relativeSurface() const;
void enter();
void leave();
Q_SIGNALS:
void requestShow(const QPoint &pos, ForeignToplevelManagerInterfaceV1::PreviewDirection dir,
const QList<ForeignToplevelHandleV1 *> &toplevels);
void requestShowTooltip(const QString &tooltip, const QPoint &pos,
ForeignToplevelManagerInterfaceV1::PreviewDirection dir);
void requestClose();
void beforeDestroy();
private:
explicit DockPreviewContextV1(wl_resource *resource, wlr_surface *_relativeSurface);
std::unique_ptr<DockPreviewContextV1Private> d;
friend class ForeignToplevelManagerInterfaceV1Private;
};
```
```cpp
// dockpreviewcontextv1.cpp (private protocol structs)
struct DockPreviewPreviewEvent {
std::vector<uint32_t> toplevels;
int32_t x, y;
int32_t direction;
};
struct DockPreviewTooltipEvent {
QString tooltip;
int32_t x, y;
int32_t direction;
};
// map generated treeland_* events into the higher-level signal signatures above
```
Same pattern for `ForeignToplevelHandleV1` – keep only Qt/Waylib-friendly types in the header and map to/from `treeland_*` structs in the cpp.
---
### 2. Hide wl/wlr pointers from the public API
The new public structs (e.g. `treeland_foreign_toplevel_handle_v1_*_event`) expose `wlr_surface *`, `wlr_output *`, and `wlr_seat *` directly. You can keep this wiring internal and expose wrappers in the signals instead.
**Actionable suggestion**
In the header, expose higher-level objects, and keep wl/wlr types in the private implementation:
```cpp
// foreigntoplevelhandlev1.h
class ForeignToplevelHandleV1 : public QObject {
Q_OBJECT
public:
// ...
Q_SIGNALS:
void requestMaximize(bool maximized);
void requestMinimize(bool minimized);
void requestActivate(QSeat *seat); // or your existing seat wrapper
void requestFullscreen(bool fullscreen, WOutput *output);
void requestClose();
void rectangleChanged(WSurface *surface, const QRect &rect);
private:
std::unique_ptr<ForeignToplevelHandleV1Private> d;
};
```
```cpp
// foreigntoplevelhandlev1.cpp
struct MaximizedEvent { bool maximized; };
struct MinimizedEvent { bool minimized; };
struct ActivatedEvent { wlr_seat *seat; };
// ...
void ForeignToplevelHandleV1Private::on_maximize(const MaximizedEvent &ev)
{
// convert & emit public signal
Q_EMIT q->requestMaximize(ev.maximized);
}
void ForeignToplevelHandleV1Private::on_fullscreen(const FullscreenEvent &ev)
{
auto outputWrapper = WOutput::from(ev.output);
Q_EMIT q->requestFullscreen(ev.fullscreen, outputWrapper);
}
```
This keeps consumers decoupled from wlroots, while retaining your protocol plumbing internally.
---
### 3. Encapsulate state → wire-encoding mapping
`ForeignToplevelHandleV1::State` + `States` and `send_state()` currently encode protocol values manually. You can hide this serialization detail in a small helper in the cpp.
**Actionable suggestion**
Keep the public enum/flags, but move the wire encoding into a private helper:
```cpp
// foreigntoplevelhandlev1.h
class ForeignToplevelHandleV1 : public QObject {
Q_OBJECT
public:
enum class State {
Maximized = 1,
Minimized = 2,
Activated = 4,
Fullscreen = 8,
Attention = 16,
};
Q_ENUM(State)
Q_DECLARE_FLAGS(States, State)
void set_maximized(bool maximized);
// ...
private:
void send_state(); // keep public behaviour via thin wrapper
std::unique_ptr<ForeignToplevelHandleV1Private> d;
};
```
```cpp
// foreigntoplevelhandlev1.cpp
namespace {
QByteArray encodeStates(ForeignToplevelHandleV1::States states)
{
QByteArray data;
// map States bits to protocol enum ints here, once
if (states.testFlag(ForeignToplevelHandleV1::State::Maximized))
data.append(static_cast<char>(TREELAND_FOREIGN_TOPLEVEL_STATE_MAXIMIZED));
// ...
return data;
}
}
void ForeignToplevelHandleV1::send_state()
{
auto data = encodeStates(d->states);
// use data for treeland_foreign_toplevel_handle_v1_state
}
```
Now the mapping logic lives in a single implementation helper, not spread through the public class.
---
### 4. Keep lifetime/idle management methods non-public
`ForeignToplevelManagerInterfaceV1::releaseHandle`, `ForeignToplevelHandleV1::reset_idle_source`, and `send_done` represent internal lifetime / idle handling and don’t need to be part of the public surface.
**Actionable suggestion**
Move them to the private class and expose only semantic operations:
```cpp
// foreigntoplevelhandlev1.h
class ForeignToplevelHandleV1 : public QObject {
Q_OBJECT
public:
// semantic methods only
void set_maximized(bool maximized);
void set_minimized(bool minimized);
// ...
void close(); // wraps send_closed()
private:
void reset_idle_source(); // moved to private section
void send_done(); // moved to private section
// send_output can also be private if only manager uses it
std::unique_ptr<ForeignToplevelHandleV1Private> d;
friend class ForeignToplevelManagerInterfaceV1Private;
};
```
```cpp
// foreigntoplevelmanagerinterfacev1.h
class ForeignToplevelManagerInterfaceV1 : public QObject, public WServerInterface {
Q_OBJECT
public:
void addSurface(SurfaceWrapper *wrapper);
void removeSurface(SurfaceWrapper *wrapper);
private:
void releaseHandle(ForeignToplevelHandleV1 *handle);
std::unique_ptr<ForeignToplevelManagerInterfaceV1Private> d;
friend class ForeignToplevelManagerInterfaceV1Private;
};
```
Callers continue to use `addSurface/removeSurface`; the manager’s private side orchestrates handle release and idle timers.
---
### 5. Avoid exposing global lookup helpers
`DockPreviewContextV1::get(wl_resource *)` and `getDockPreviewContext(WSurface *)` encourage global searches and leak registry details. If you must maintain the registry, keep lookup methods internal.
**Actionable suggestion**
Remove them from the public interface and relocate to the private/impl:
```cpp
// dockpreviewcontextv1.h
class DockPreviewContextV1 : public QObject {
Q_OBJECT
public:
// ...
WSurface *relativeSurface() const;
private:
std::unique_ptr<DockPreviewContextV1Private> d;
friend class ForeignToplevelManagerInterfaceV1Private;
};
```
```cpp
// dockpreviewcontextv1_p.h (internal header)
class DockPreviewContextV1Private {
public:
static DockPreviewContextV1 *fromResource(wl_resource *resource);
static DockPreviewContextV1 *fromSurface(WSurface *relativeSurface);
// internal registry implementation
};
```
Internal call sites (manager, impl cpp files) can still use these helpers; external consumers only see the managed objects.
---
Overall, these focused changes allow you to keep all added functionality but:
- shrink the public header surface,
- decouple consumers from wlroots/protocol details,
- centralize state/wire mapping, and
- keep lifetime and lookup mechanics internal.
</issue_to_address>
### Comment 2
<location path="src/modules/foreign-toplevel/foreigntoplevelmanagerv1.cpp" line_range="24" />
<code_context>
#include <qwxdgshell.h>
-static ForeignToplevelV1 *FOREIGN_TOPLEVEL_MANAGER = nullptr;
+static QList<DockPreviewContextV1 *> s_dockPreviewContexts;
+static QList<ForeignToplevelHandleV1 *> s_foreignToplevelHandles;
</code_context>
<issue_to_address>
**issue (complexity):** Consider refactoring the new manager/handle code to avoid global registries, factor out handle creation, and hide idle scheduling details behind private helpers to keep ownership and responsibilities localized.
You can reduce some of the new complexity without changing behavior by tightening ownership and encapsulating repeated patterns.
### 1. Replace global registries with manager‑owned containers
`static QList<DockPreviewContextV1 *> s_dockPreviewContexts;` and
`static QList<ForeignToplevelHandleV1 *> s_foreignToplevelHandles;` introduce global mutable state and require manual cleanup from multiple places (`destroy_resource`, `QObject::destroyed` lambdas, etc).
You can keep the existing behavior while making ownership explicit by moving these lists into `ForeignToplevelManagerInterfaceV1Private` and using `wl_resource_set_user_data` for lookups.
Example:
```cpp
// foreigntoplevelmanagerv1_p.h (or current TU)
class ForeignToplevelManagerInterfaceV1Private
: public QtWaylandServer::treeland_foreign_toplevel_manager_v1
{
public:
// ...
QList<DockPreviewContextV1 *> dockPreviewContexts;
QList<ForeignToplevelHandleV1 *> handles;
};
```
Creation:
```cpp
void ForeignToplevelManagerInterfaceV1Private::get_dock_preview_context(
Resource *resource,
struct ::wl_resource *relative_surface,
uint32_t id)
{
// ... validation ...
wl_resource *dockPreviewContextResource = wl_resource_create(...);
if (!dockPreviewContextResource) {
wl_client_post_no_memory(resource->client());
return;
}
auto *context = new DockPreviewContextV1(dockPreviewContextResource, relativeSurface);
wl_resource_set_user_data(dockPreviewContextResource, context);
dockPreviewContexts.append(context);
QObject::connect(context, &QObject::destroyed, q, [this, context]() {
dockPreviewContexts.removeOne(context);
});
// connects unchanged...
}
```
Then `DockPreviewContextV1::get` can avoid the global:
```cpp
DockPreviewContextV1 *DockPreviewContextV1::get(wl_resource *resource)
{
return static_cast<DockPreviewContextV1 *>(wl_resource_get_user_data(resource));
}
```
Same pattern for `ForeignToplevelHandleV1`:
```cpp
ForeignToplevelHandleV1::ForeignToplevelHandleV1(ForeignToplevelManagerInterfaceV1 *manager,
wl_resource *resource)
: QObject(nullptr)
, d(new ForeignToplevelHandleV1Private(this, manager, resource))
{
wl_resource_set_user_data(resource, this);
d->manager->d->handles.append(this);
}
```
And `destroy_resource` becomes simpler:
```cpp
void ForeignToplevelHandleV1Private::destroy_resource(Resource *resource)
{
if (manager) {
manager->d->handles.removeOne(q);
manager->releaseHandle(q); // keeps current manager logic
return;
}
delete q;
}
```
This preserves existing semantics, but removes global state and dual registries, and lets you centralize all cleanup under the manager.
### 2. Encapsulate handle creation per client in a helper
`ForeignToplevelManagerInterfaceV1::addSurface` currently mixes:
- iterating over `resourceMap()`,
- creating Wayland resources,
- instantiating `ForeignToplevelHandleV1`,
- inserting into `m_surfaces`,
- wiring up signals.
Extract the “create handle resource for manager resource” into a private helper; `addSurface` will read more clearly and be easier to maintain.
```cpp
// in ForeignToplevelManagerInterfaceV1Private
ForeignToplevelHandleV1 *ForeignToplevelManagerInterfaceV1Private::createHandle(
SurfaceWrapper *wrapper,
Resource *manager_resource)
{
wl_client *client = wl_resource_get_client(manager_resource->handle);
wl_resource *resource = wl_resource_create(client,
&treeland_foreign_toplevel_handle_v1_interface,
wl_resource_get_version(manager_resource->handle),
0);
if (!resource) {
qCCritical(treelandProtocol) << "wl_resource_create failed!";
wl_client_post_no_memory(client);
return nullptr;
}
auto *handle = new ForeignToplevelHandleV1(q, resource);
handles.append(handle); // from suggestion #1
QObject::connect(handle, &QObject::destroyed, q, [this, handle]() {
handles.removeOne(handle);
});
send_toplevel(manager_resource->handle, resource);
m_surfaces.insert({ wrapper, std::unique_ptr<ForeignToplevelHandleV1>(handle) });
return handle;
}
```
Usage:
```cpp
void ForeignToplevelManagerInterfaceV1::addSurface(SurfaceWrapper *wrapper)
{
if (d->m_surfaces.contains(wrapper)) {
qCCritical(treelandProtocol)
<< wrapper << " has been add to foreign toplevel twice";
return;
}
for (const auto &manager_resource : d->resourceMap()) {
auto *handle = d->createHandle(wrapper, manager_resource);
if (!handle)
break;
handle->set_identifier(d->m_nextIdentifier++);
connect(handle, &ForeignToplevelHandleV1::requestClose, wrapper, [wrapper]() {
wrapper->close();
});
if (wrapper->type() == SurfaceWrapper::Type::SplashScreen) {
// existing splash handling...
} else {
initializeToplevelHandle(wrapper, handle);
}
}
}
```
This keeps behavior identical, but localizes low-level Wayland boilerplate inside the private implementation.
### 3. Hide idle scheduling inside `ForeignToplevelHandleV1Private`
`ForeignToplevelHandleV1` currently exposes `reset_idle_source()` and `update_idle_source()` publicly and relies on a free function (`toplevel_idle_send_done`) to drive `send_done()`. This makes the public API aware of event-loop details that only the private side needs.
You can keep the current semantics and reduce surface area by:
- Moving `update_idle_source()` / `reset_idle_source()` into the private class.
- Making the idle callback a `static` member or friend that operates purely on the private.
Example:
```cpp
// in ForeignToplevelHandleV1Private
static void idleSendDone(void *data)
{
auto *self = static_cast<ForeignToplevelHandleV1Private *>(data);
self->send_done();
self->idle_source = nullptr;
}
void ForeignToplevelHandleV1Private::scheduleDone()
{
if (idle_source || !manager) {
return;
}
idle_source = wl_event_loop_add_idle(manager->eventLoop(), idleSendDone, this);
}
```
Public methods then call `scheduleDone()` instead of `update_idle_source()`:
```cpp
void ForeignToplevelHandleV1::set_title(const QString &title)
{
if (d->title == title)
return;
d->title = title;
d->send_title(title);
d->scheduleDone();
}
```
And similarly in `set_app_id`, `set_pid`, `set_identifier`, `set_parent`, `send_state`, `send_output`.
You can then drop the public `reset_idle_source()` / `update_idle_source()` entirely, shrinking the visible API while keeping the same “batch and send done on idle” behavior.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR migrates the foreign-toplevel protocol implementation from the legacy local scanner to the QtWayland server protocol generator, while consolidating the old implementation layer into ForeignToplevelManagerInterfaceV1.
此 PR 将 foreign-toplevel 协议实现从旧的本地 scanner 迁移到 QtWayland server 协议生成器，并把旧的 impl 层合并到 ForeignToplevelManagerInterfaceV1 中。
Changes:
- Replaces
ws_generate_localwithlocal_qtwayland_server_protocol_treeland. - Renames and expands the API around
ForeignToplevelManagerInterfaceV1,ForeignToplevelHandleV1, andDockPreviewContextV1. - Updates ShellHandler and QML dock preview integration to use the renamed singleton/API.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/seat/helper.h |
Updates forward declaration to the renamed manager type. |
src/modules/foreign-toplevel/impl/foreign_toplevel_manager_impl.h |
Removes the old protocol implementation header. |
src/modules/foreign-toplevel/impl/foreign_toplevel_manager_impl.cpp |
Removes the old protocol implementation source. |
src/modules/foreign-toplevel/foreigntoplevelmanagerv1.h |
Introduces the consolidated manager, handle, and dock preview context interfaces. |
src/modules/foreign-toplevel/foreigntoplevelmanagerv1.cpp |
Implements the QtWayland-generated protocol integration and refactored runtime behavior. |
src/modules/foreign-toplevel/CMakeLists.txt |
Switches protocol generation to the QtWayland scanner path. |
src/core/shellhandler.h |
Updates type references for the renamed manager. |
src/core/shellhandler.cpp |
Registers and connects the renamed QML/C++ manager singleton. |
src/core/qml/DockPreview.qml |
Updates QML references to the renamed singleton and enum. |
Replace the legacy ws_generate_local scanner with the qtwayland QWaylandServer protocol generator. Merge the separate impl layer into ForeignToplevelManagerInterfaceV1 (renamed from ForeignToplevelV1), eliminating redundant indirection between the server protocol wrapper and the underlying implementation. log: migrate foreign-toplevel protocol to qtwayland scanner
zccrs
requested changes
May 30, 2026
added 2 commits
June 1, 2026 13:44
Replace one-to-one surface-to-handle mapping with one-to-many model. SurfaceEntry now owns a handle list per surface, one per bound client. Fixes parent window matching and dock preview for multi-client scenarios. Log: multi-client foreign-toplevel handles Influence: foreign-toplevel handle lifecycle, parent matching, dock preview
…heduling - Move dock preview contexts and foreign toplevel handles into manager-private ownership. - Factor out per-client handle creation and simplify dock preview lookup/cleanup. - Hide idle done scheduling behind ForeignToplevelHandleV1Private and remove public idle helpers. Log: refactored foreign-toplevel manager ownership and idle batching Influence: reduces global state, centralizes lifecycle cleanup, and lowers public API surface.
zccrs
previously approved these changes
Jun 1, 2026
- Move DockPreviewContextV1 and ForeignToplevelHandleV1 lists into manager private class. - Encapsulate handle creation in manager private helper. - Move idle scheduling and protocol details to private classes. - Clean up public API: only expose semantic methods and Qt-friendly signals. - Keeps existing behavior but improves ownership, readability, and maintainability. Log: remove globals and encapulate handles
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
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.
Replace the legacy ws_generate_local scanner with the qtwayland QWaylandServer protocol generator. Merge the separate impl layer into ForeignToplevelManagerInterfaceV1 (renamed from ForeignToplevelV1), eliminating redundant indirection between the server protocol wrapper and the underlying implementation.
log: migrate foreign-toplevel protocol to qtwayland scanner
Summary by Sourcery
Migrate the treeland foreign-toplevel manager protocol implementation to use the QtWayland server protocol generator and consolidate the protocol wrapper and implementation into a single manager interface class.
Enhancements:
Build: