Skip to content

Conversation

@misaka18931
Copy link
Collaborator

@misaka18931 misaka18931 commented Jan 14, 2026

Changes:

  • reordered header & forward class decls alphabetically in helper.h and helper.cpp
  • Moved class Session and session management related member functions into the new class SessionManager

SessionManager is designed to be an independent singleton that exclusively manages session creation, invalidation and its related WSockets and WWayland instances.

Summary by Sourcery

Extract session and socket management from Helper into a dedicated SessionManager singleton and update callers to use it.

Enhancements:

  • Introduce a SessionManager singleton that owns Session objects and encapsulates session, WSocket, and WXWayland lifecycle and lookup logic.
  • Simplify Helper by removing embedded session management, exposing only minimal hooks needed by SessionManager and renaming the root container property to rootSurfaceContainer.
  • Refine includes and Q_MOC_INCLUDE ordering for helper-related files to be more consistent and organized.

Build:

  • Register the new session/session.cpp and session/session.h sources in the core CMake target so the SessionManager is built.

Copilot AI review requested due to automatic review settings January 14, 2026 08:21
@sourcery-ai
Copy link

sourcery-ai bot commented Jan 14, 2026

Reviewer's Guide

Refactors session handling by extracting Session and all user session/socket/XWayland management logic from Helper into a dedicated SessionManager singleton, updates call sites to use it, slightly renames/rootContainer accessor, and alphabetizes various includes and forward declarations.

Sequence diagram for updated active session handling via SessionManager

sequenceDiagram
    actor User
    participant GreeterProxy
    participant UserModel
    participant SessionManager
    participant Helper
    participant Treeland

    User->>GreeterProxy: choose session to log in
    GreeterProxy->>UserModel: set user logged in
    UserModel-->>GreeterProxy: userLoggedIn(username, id)
    GreeterProxy->>SessionManager: userLoggedIn(username, id)
    activate SessionManager
    SessionManager->>SessionManager: ensureSession(id, username)
    SessionManager->>Helper: addSocket(WSocket*) when creating session
    SessionManager->>Helper: createXWayland() when creating session
    SessionManager-->>SessionManager: updateActiveUserSession(username, id)
    SessionManager->>Helper: activateSurface(nullptr)
    SessionManager->>SessionManager: disable previous session socket
    SessionManager->>SessionManager: enable new session socket
    SessionManager-->>SessionManager: socketFileChanged()
    SessionManager-->>Treeland: sessionChanged()
    deactivate SessionManager

    Treeland-->>Treeland: SessionChanged D-Bus signal to treeland_sd
    Treeland-->>User: new session environment active
Loading

Class diagram for new Session and SessionManager extraction

classDiagram
    class Session {
        +int id
        +uid_t uid
        +QString username
        +WSocket* socket
        +WXWayland* xwayland
        +quint32 noTitlebarAtom
        +SettingManager* settingManager
        +QThread* settingManagerThread
        +~Session()
        +aboutToBeDestroyed()
    }

    class SessionManager {
        +static SessionManager* m_instance
        +std::weak_ptr~Session~ m_activeSession
        +QList~std::shared_ptr~Session~~ m_sessions
        +SessionManager(QObject* parent)
        +~SessionManager()
        +static SessionManager* instance()
        +QList~std::shared_ptr~Session~~ sessions() const
        +std::weak_ptr~Session~ activeSession() const
        +bool activeSocketEnabled() const
        +void setActiveSocketEnabled(bool newEnabled)
        +std::shared_ptr~Session~ ensureSession(int id, QString username)
        +void updateActiveUserSession(const QString& username, int id)
        +void removeSession(std::shared_ptr~Session~ session)
        +std::shared_ptr~Session~ sessionForId(int id) const
        +std::shared_ptr~Session~ sessionForUid(uid_t uid) const
        +std::shared_ptr~Session~ sessionForUser(const QString& username) const
        +std::shared_ptr~Session~ sessionForXWayland(WXWayland* xwayland) const
        +std::shared_ptr~Session~ sessionForSocket(WSocket* socket) const
        +WXWayland* xwaylandForUid(uid_t uid) const
        +WSocket* waylandSocketForUid(uid_t uid) const
        +WSocket* globalWaylandSocket() const
        +WXWayland* globalXWayland() const
        +socketFileChanged()
        +sessionChanged()
    }

    class Helper {
        +static Helper* m_instance
        -RootSurfaceContainer* m_rootSurfaceContainer
        +RootSurfaceContainer* rootSurfaceContainer() const
        +void addSocket(WSocket* socket)
        +WXWayland* createXWayland()
        +ShellHandler* shellHandler() const
        +WOutputRenderWindow* window() const
        +void activateSurface(SurfaceWrapper* wrapper)
    }

    class TreelandPrivate {
        +SessionManager* sessionManager
        +Helper* helper
    }

    class Treeland {
        +Treeland()
        +SessionChanged()
    }

    class GreeterProxy {
        +void logout()
        +void connected()
        +void onSessionNew(const QString& id, const QDBusObjectPath& path)
        +void onSessionRemoved(const QString& id, const QDBusObjectPath& path)
    }

    class ShortcutManagerV2 {
        +void onSessionChanged()
    }

    class UserModel {
        +void userLoggedIn(const QString& username, int id)
    }

    SessionManager --* Session : manages
    Helper ..> SessionManager : uses_singleton
    TreelandPrivate o--> SessionManager : owns
    TreelandPrivate o--> Helper : owns
    Treeland ..> TreelandPrivate : uses
    GreeterProxy ..> SessionManager : uses_singleton
    ShortcutManagerV2 ..> SessionManager : uses_singleton
    UserModel ..> SessionManager : emits_to_updateActiveUserSession
    Session ..> Helper : calls_shellHandler_and_window
    SessionManager ..> Helper : uses_addSocket_and_createXWayland
Loading

Focused class diagram for Helper and session-related responsibilities after refactor

classDiagram
    class Helper {
        +static Helper* m_instance
        -RootSurfaceContainer* m_rootSurfaceContainer
        +RootSurfaceContainer* rootSurfaceContainer() const
        +void addSocket(WSocket* socket)
        +WXWayland* createXWayland()
        +ShellHandler* shellHandler() const
        +WOutputRenderWindow* window() const
        +void activateSurface(SurfaceWrapper* wrapper)
        +void init(Treeland::Treeland* treeland)
    }

    class SessionManager {
        +static SessionManager* instance()
        +QList~std::shared_ptr~Session~~ sessions() const
        +std::weak_ptr~Session~ activeSession() const
        +bool activeSocketEnabled() const
        +void setActiveSocketEnabled(bool newEnabled)
        +std::shared_ptr~Session~ ensureSession(int id, QString username)
        +void updateActiveUserSession(const QString& username, int id)
        +void removeSession(std::shared_ptr~Session~ session)
        +std::shared_ptr~Session~ sessionForXWayland(WXWayland* xwayland) const
        +WXWayland* xwaylandForUid(uid_t uid) const
        +WSocket* waylandSocketForUid(uid_t uid) const
        +WSocket* globalWaylandSocket() const
        +WXWayland* globalXWayland() const
        +socketFileChanged()
        +sessionChanged()
    }

    class RootSurfaceContainer {
        +QList~WOutput*~ outputs() const
        +WOutput* primaryOutput() const
        +WOutputRenderWindow* window() const
    }

    class ShellHandler {
        +WXWayland* createXWayland(WServer* server, WSeat* seat, qw_compositor* compositor, bool lazy)
        +void removeXWayland(WXWayland* xwayland)
    }

    Helper ..> SessionManager : delegates_session_logic
    SessionManager ..> Helper : uses_addSocket
    SessionManager ..> Helper : uses_createXWayland
    Helper --> RootSurfaceContainer : owns
    Helper --> ShellHandler : owns_or_accesses
    SessionManager ..> Session : manages
    Helper ..> RootSurfaceContainer : rootSurfaceContainer_renamed_accessor
Loading

File-Level Changes

Change Details Files
Extract Session and session lifecycle management from Helper into a dedicated SessionManager singleton and new session module.
  • Move Session struct and its destructor from helper.{h,cpp} into new session.{h,cpp} files
  • Introduce SessionManager QObject singleton that owns the session list, active session tracking, and related lookup helpers
  • Port Helper’s ensureSession, updateActiveUserSession, removeSession, and sessionFor* helpers into SessionManager with minimal behavioral changes
  • Move WSocket/WXWayland creation and XWayland-ready handling (including SettingManager threading and scaling setup) into SessionManager
  • Expose accessors for globalWaylandSocket/globalXWayland and session queries required by other components
src/seat/helper.cpp
src/seat/helper.h
src/session/session.cpp
src/session/session.h
src/CMakeLists.txt
Wire SessionManager into the existing application lifecycle and replace direct Helper-based session access throughout the codebase.
  • Instantiate SessionManager in TreelandPrivate, hook its sessionChanged signal to Treeland::SessionChanged, and use it to obtain sockets/XWayland for environment setup and DBus APIs
  • Update GreeterProxy to use SessionManager::instance() for activeSession, globalWaylandSocket, sessionForId and removeSession
  • Update ShortcutManagerV2 to get the active session via SessionManager instead of Helper
  • Replace Helper::sessionForXWayland and Helper::isXWaylandClient usage in Helper::init with SessionManager equivalents and a local lambda for filter functions
src/core/treeland.cpp
src/greeter/greeterproxy.cpp
src/modules/shortcut/shortcutmanager.cpp
src/seat/helper.cpp
Simplify Helper by removing session responsibilities and providing only minimal hooks needed by SessionManager and other modules.
  • Remove all session-related member variables, Q_PROPERTY socketEnabled, and signals like socketEnabledChanged/socketFileChanged from Helper
  • Delete Helper’s session helper methods (sessionFor*, activeSession, ensureSession, updateActiveUserSession, globalWaylandSocket/globalXWayland, isXWaylandClient, removeSession, socketEnabled accessors) now implemented in SessionManager
  • Add a small factory method Helper::createXWayland() for SessionManager to construct WXWayland instances tied to the existing Wayland server, seat, and compositor
  • Keep addSocket exposed so SessionManager can register per-session sockets on the Wayland server
src/seat/helper.cpp
src/seat/helper.h
src/session/session.cpp
src/session/session.h
Tidy naming and container access around root surface handling and include ordering.
  • Rename Helper::rootContainer() to rootSurfaceContainer() in the property and all call sites (e.g. ForeignToplevelV1 and ShortcutRunner) for clearer semantics
  • Adjust helper.cpp and helper.h include and Q_MOC_INCLUDE blocks to be alphabetically ordered and to reference the new session module
  • Minor style and whitespace fixes (e.g., comment alignment, trailing-space removal)
src/seat/helper.cpp
src/seat/helper.h
src/modules/foreign-toplevel/foreigntoplevelmanagerv1.cpp
src/modules/shortcut/shortcutrunner.cpp

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 4 issues, and left some high level feedback:

  • SessionManager::~Session() and Session::~Session() both delete the same xwayland/socket pointers, which will lead to double-deletion; centralize ownership (either let Session clean them up or only reset pointers in SessionManager) so each resource is freed in exactly one place.
  • Several call sites (e.g. Helper::onSurfaceWrapperAdded, ShortcutManagerV2::onSessionChanged, GreeterProxy) assume SessionManager::instance() is non-null; consider making SessionManager a lazy-initialized singleton or adding null checks/guards so these paths cannot crash if SessionManager has not been constructed yet.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- SessionManager::~Session() and Session::~Session() both delete the same xwayland/socket pointers, which will lead to double-deletion; centralize ownership (either let Session clean them up or only reset pointers in SessionManager) so each resource is freed in exactly one place.
- Several call sites (e.g. Helper::onSurfaceWrapperAdded, ShortcutManagerV2::onSessionChanged, GreeterProxy) assume SessionManager::instance() is non-null; consider making SessionManager a lazy-initialized singleton or adding null checks/guards so these paths cannot crash if SessionManager has not been constructed yet.

## Individual Comments

### Comment 1
<location> `src/seat/helper.cpp:1371-1379` </location>
<code_context>
-    xdgOutputManager->setFilter([this](WClient *client) { return !isXWaylandClient(client); });
-    xwaylandOutputManager->setFilter([this](WClient *client) { return isXWaylandClient(client); });
+
+    auto isXwaylandClient = [this](WClient *client) {
+        for (auto session : SessionManager::instance()->sessions()) {
+            if (session && session->xwayland && session->xwayland->waylandClient() == client) {
+                return true;
+            }
+        }
+        return false;
+    };
+    xdgOutputManager->setFilter([&isXwaylandClient](WClient *client) { return !isXwaylandClient(client); });
+    xwaylandOutputManager->setFilter([&isXwaylandClient](WClient *client) { return isXwaylandClient(client); });
     // User dde does not has a real Logind session, so just pass 0 as id
</code_context>

<issue_to_address>
**issue (bug_risk):** The `setFilter` lambdas capture `isXwaylandClient` by reference, which will dangle after `Helper::init` returns.

`isXwaylandClient` is a local lambda in `Helper::init`, but the filter lambdas passed to `setFilter` outlive `init`. Capturing it by reference (`[&isXwaylandClient]`) therefore creates a dangling reference and undefined behavior. Capture it by value instead (e.g. `[isXwaylandClient](WClient *client) { ... }`) or duplicate the logic in each filter.
</issue_to_address>

### Comment 2
<location> `src/session/session.cpp:238-80` </location>
<code_context>
-            }
-            session->socket = socket;
-        }
-        if (!session->xwayland) {
-            auto *xwayland = createXWayland(session->socket);
-            if (!xwayland) {
-                delete session->socket;
-                m_sessions.removeOne(session);
-                return nullptr;
</code_context>

<issue_to_address>
**issue (bug_risk):** On XWayland creation failure for an existing session, `session->socket` is deleted but not nulled, which can lead to double delete later.

In the existing-session path of `ensureSession`, if `createXWayland(session->socket)` fails you `delete session->socket`, but `Session` is still managed by `std::shared_ptr` and its destructor later does `if (socket) delete socket;`. Since `socket` isn’t nulled, this causes a double delete. Please either set `session->socket = nullptr;` after deleting, or (preferably) make `Session` solely own and delete the socket so it isn’t freed here at all.
</issue_to_address>

### Comment 3
<location> `src/session/session.h:19` </location>
<code_context>
-class qw_ext_foreign_toplevel_image_capture_source_manager_v1;
-QW_END_NAMESPACE
-
-struct Session : QObject {
-    Q_OBJECT
-public:
</code_context>

<issue_to_address>
**issue (complexity):** Consider encapsulating Session’s state, narrowing SessionManager’s public surface, and using keyed containers so lookups and future evolution remain simpler and clearer without changing behavior.

You can reduce complexity and tighten the design without changing behavior by:

### 1. Encapsulate `Session` internals

Right now all fields are public and the type is a `struct` inheriting `QObject`. That leaks implementation details and encourages external code to poke at internals.

Make it a `class` with minimal accessors for what call‑sites actually need (e.g. `uid`, `username`, `socket`, `xwayland`), and keep the rest private:

```cpp
class Session : public QObject {
    Q_OBJECT
public:
    explicit Session(QObject *parent = nullptr);

    int id() const { return m_id; }
    uid_t uid() const { return m_uid; }
    const QString &username() const { return m_username; }
    WSocket *socket() const { return m_socket; }
    WXWayland *xwayland() const { return m_xwayland; }

    void setSocket(WSocket *socket) { m_socket = socket; }
    void setXWayland(WXWayland *xwayland) { m_xwayland = xwayland; }
    // add setters for id/uid/username only if needed

    ~Session() override;

Q_SIGNALS:
    void aboutToBeDestroyed();

private:
    int m_id = 0;
    uid_t m_uid = 0;
    QString m_username;
    WSocket *m_socket = nullptr;
    WXWayland *m_xwayland = nullptr;
    quint32 m_noTitlebarAtom = XCB_ATOM_NONE;
    SettingManager *m_settingManager = nullptr;
    QThread *m_settingManagerThread = nullptr;
};
```

Call‑sites that currently access fields directly will then use `session->socket()` / `session->xwayland()` etc. Functionality is unchanged but the data model is clearer and easier to evolve.

### 2. Narrow `SessionManager`’s public API via internal helpers

You can keep all existing behavior but reduce surface area by implementing the “special” lookups in terms of one or two generic methods, and making some of them `private` or `static` helpers in the `.cpp`.

Header:

```cpp
struct SessionManager : QObject {
    Q_OBJECT
public:
    // keep generic entry points
    std::shared_ptr<Session> sessionForId(int id) const;
    std::shared_ptr<Session> sessionForUid(uid_t uid) const;
    std::shared_ptr<Session> sessionForUser(const QString &username) const;

    // expose only what really needs to be public:
    WSocket *waylandSocketForUid(uid_t uid) const;
    WXWayland *xwaylandForUid(uid_t uid) const;

    // consider dropping these from the public API if they are only used in one place:
    WSocket *globalWaylandSocket() const;
    WXWayland *globalXWayland() const;

    // ...
};
```

`.cpp` (no behavior change, just indirection):

```cpp
WSocket *SessionManager::waylandSocketForUid(uid_t uid) const
{
    if (auto s = sessionForUid(uid)) {
        return s->socket();
    }
    return nullptr;
}

WXWayland *SessionManager::xwaylandForUid(uid_t uid) const
{
    if (auto s = sessionForUid(uid)) {
        return s->xwayland();
    }
    return nullptr;
}

WSocket *SessionManager::globalWaylandSocket() const
{
    auto pw = getpwnam("dde");
    return pw ? waylandSocketForUid(pw->pw_uid) : nullptr;
}

WXWayland *SessionManager::globalXWayland() const
{
    auto pw = getpwnam("dde");
    return pw ? xwaylandForUid(pw->pw_uid) : nullptr;
}
```

If `globalWaylandSocket()` / `globalXWayland()` are only used in one module, consider moving them as free functions or making them private helpers to keep `SessionManager`’s public API smaller.

### 3. Use keyed containers internally to simplify lookups

All your lookup functions iterate over `m_sessions`. You can keep the existing API and add internal maps keyed by the most common lookups:

```cpp
struct SessionManager : QObject {
    // ...

private:
    std::weak_ptr<Session> m_activeSession;
    QList<std::shared_ptr<Session>> m_sessions;

    QHash<int, std::shared_ptr<Session>> m_sessionsById;
    QHash<uid_t, std::shared_ptr<Session>> m_sessionsByUid;
    QHash<QString, std::shared_ptr<Session>> m_sessionsByUser;
};
```

Then implement `ensureSession` / `removeSession` to maintain these maps:

```cpp
std::shared_ptr<Session> SessionManager::ensureSession(int id, QString username)
{
    if (auto existing = sessionForId(id)) {
        return existing;
    }

    auto session = std::make_shared<Session>();
    // initialize id/uid/username/...
    m_sessions.append(session);
    m_sessionsById.insert(id, session);
    m_sessionsByUser.insert(username, session);
    // fill m_sessionsByUid when uid is known

    return session;
}

void SessionManager::removeSession(std::shared_ptr<Session> session)
{
    m_sessions.removeAll(session);
    m_sessionsById.remove(session->id());
    m_sessionsByUid.remove(session->uid());
    m_sessionsByUser.remove(session->username());
}
```

And the lookups become trivial and O(1):

```cpp
std::shared_ptr<Session> SessionManager::sessionForId(int id) const
{
    return m_sessionsById.value(id);
}

std::shared_ptr<Session> SessionManager::sessionForUid(uid_t uid) const
{
    return m_sessionsByUid.value(uid);
}

std::shared_ptr<Session> SessionManager::sessionForUser(const QString &username) const
{
    return m_sessionsByUser.value(username);
}
```

This keeps all existing methods but removes repeated iteration logic and makes the intent clearer.

### 4. Plan to remove the singleton (future‑compatible)

If you must keep the singleton right now, you can still make it easier to replace later by constraining its use:

- Prefer passing `SessionManager *` into constructors of components that need it.
- Avoid calling `SessionManager::instance()` deep in leaf code.

Later, switching to an injected/owned instance becomes mechanical:

```cpp
class Helper {
public:
    Helper(QObject *parent = nullptr)
        : m_sessionManager(new SessionManager(this))
    {}

private:
    SessionManager *m_sessionManager;
};
```

You don’t need to drop the singleton immediately, but reducing direct `instance()` usage will reduce the global‑state footprint and cognitive load over time.
</issue_to_address>

### Comment 4
<location> `src/session/session.cpp:37` </location>
<code_context>
     return data;
 }

-Session::~Session()
-{
-    qCDebug(treelandCore) << "Deleting session for uid:" << uid << socket;
</code_context>

<issue_to_address>
**issue (complexity):** Consider centralizing resource ownership in Session and extracting shared helpers in SessionManager to simplify lifetime management and reduce repeated lookup/creation logic.

You can reduce the complexity here without changing behaviour by tightening ownership and factoring out some repeated logic.

### 1. Centralize teardown in `Session`

Right now `Session::~Session` and `SessionManager::~SessionManager` both manually delete `socket` and `xwayland`, which is easy to get wrong.

Let `Session` fully own its resources, and let `SessionManager` only manage `std::shared_ptr<Session>`.

**Session.h / Session.cpp**

```cpp
Session::~Session()
{
    qCDebug(treelandCore) << "Deleting session for uid:" << uid << socket;
    Q_EMIT aboutToBeDestroyed();

    if (settingManagerThread) {
        settingManagerThread->quit();
        settingManagerThread->wait(QDeadlineTimer(25000));
    }

    delete settingManager;
    settingManager = nullptr;

    // Centralize XWayland teardown here
    if (xwayland) {
        Helper::instance()->shellHandler()->removeXWayland(xwayland);
        delete xwayland;
        xwayland = nullptr;
    }

    // Centralize socket teardown here
    delete socket;
    socket = nullptr;
}
```

**SessionManager::~SessionManager**

```cpp
SessionManager::~SessionManager()
{
    // shared_ptr will trigger Session::~Session where teardown is centralized
    m_sessions.clear();
}
```

This removes duplicated cleanup logic and makes lifetime reasoning simpler.

---

### 2. Extract helpers from `ensureSession`

`ensureSession` is doing multiple things: session lookup, socket creation, XWayland creation, settings thread setup, and error rollback. Pulling some of this into private helpers will make it easier to follow and test.

**SessionManager.h**

```cpp
private:
    WSocket *createWSocket();
    WXWayland *createXWayland(WSocket *socket);
    void setupSessionSettings(Session &session);
```

**SessionManager.cpp**

```cpp
WSocket *SessionManager::createWSocket()
{
    auto socket = new WSocket(true, this);
    if (!socket->autoCreate()) {
        qCCritical(treelandCore) << "Failed to create Wayland socket";
        delete socket;
        return nullptr;
    }

    connect(socket, &WSocket::fullServerNameChanged, this, [this] {
        if (m_activeSession.lock())
            Q_EMIT socketFileChanged();
    });

    Helper::instance()->addSocket(socket);
    return socket;
}

WXWayland *SessionManager::createXWayland(WSocket *socket)
{
    auto *xwayland = Helper::instance()->createXWayland();
    if (!xwayland) {
        qCCritical(treelandCore) << "Failed to create XWayland instance";
        return nullptr;
    }

    xwayland->setOwnsSocket(socket);
    connect(xwayland, &WXWayland::ready, this, [this, xwayland] {
        if (auto session = sessionForXWayland(xwayland)) {
            setupSessionSettings(*session);
        }
    });

    return xwayland;
}

void SessionManager::setupSessionSettings(Session &session)
{
    session.noTitlebarAtom =
        internAtom(session.xwayland->xcbConnection(), _DEEPIN_NO_TITLEBAR, false);
    if (!session.noTitlebarAtom) {
        qCWarning(treelandInput) << "Failed to intern atom:" << _DEEPIN_NO_TITLEBAR;
    }

    session.settingManager = new SettingManager(session.xwayland->xcbConnection(),
                                                session.xwayland);
    session.settingManagerThread = new QThread(session.xwayland);

    session.settingManager->moveToThread(session.settingManagerThread);

    const qreal scale =
        Helper::instance()->rootSurfaceContainer()->window()->effectiveDevicePixelRatio();
    const auto renderWindow = Helper::instance()->window();

    connect(session.settingManagerThread, &QThread::started, this,
            [&session, scale, renderWindow] {
                QMetaObject::invokeMethod(
                    session.settingManager,
                    [&session, scale]() {
                        session.settingManager->setGlobalScale(scale);
                        session.settingManager->apply();
                    },
                    Qt::QueuedConnection);

                QObject::connect(
                    renderWindow,
                    &WOutputRenderWindow::effectiveDevicePixelRatioChanged,
                    session.settingManager,
                    [&session](qreal dpr) {
                        session.settingManager->setGlobalScale(dpr);
                        session.settingManager->apply();
                    },
                    Qt::QueuedConnection);
            });

    connect(session.settingManagerThread, &QThread::finished,
            session.settingManagerThread, &QThread::deleteLater);

    session.settingManagerThread->start();
}
```

Then `ensureSession` becomes much flatter:

```cpp
std::shared_ptr<Session> SessionManager::ensureSession(int id, QString username)
{
    if (auto session = sessionForUser(username)) {
        if (!session->socket) {
            auto *socket = createWSocket();
            if (!socket) {
                m_sessions.removeOne(session);
                return nullptr;
            }
            session->socket = socket;
        }

        if (!session->xwayland) {
            auto *xwayland = createXWayland(session->socket);
            if (!xwayland) {
                delete session->socket;
                m_sessions.removeOne(session);
                return nullptr;
            }
            session->xwayland = xwayland;
        }
        return session;
    }

    auto session = std::make_shared<Session>();
    session->id = id;
    session->username = username;
    session->uid = getpwnam(username.toLocal8Bit().data())->pw_uid;

    session->socket = createWSocket();
    if (!session->socket) {
        return nullptr;
    }

    session->xwayland = createXWayland(session->socket);
    if (!session->xwayland) {
        return nullptr;
    }

    m_sessions.append(session);
    return session;
}
```

This keeps the same behaviour but significantly reduces nesting and local lambdas.

---

### 3. Deduplicate linear search over `m_sessions`

The `sessionFor*` functions all re-implement a linear scan. A small internal helper reduces boilerplate and makes it easier to change storage later.

```cpp
template <typename Predicate>
std::shared_ptr<Session> SessionManager::findSession(Predicate pred) const
{
    for (const auto &session : m_sessions) {
        if (session && pred(*session))
            return session;
    }
    return nullptr;
}

std::shared_ptr<Session> SessionManager::sessionForId(int id) const
{
    return findSession([&](const Session &s) { return s.id == id; });
}

std::shared_ptr<Session> SessionManager::sessionForUid(uid_t uid) const
{
    return findSession([&](const Session &s) { return s.uid == uid; });
}

std::shared_ptr<Session> SessionManager::sessionForUser(const QString &username) const
{
    return findSession([&](const Session &s) { return s.username == username; });
}

std::shared_ptr<Session> SessionManager::sessionForXWayland(WXWayland *xwayland) const
{
    return findSession([&](const Session &s) { return s.xwayland == xwayland; });
}

std::shared_ptr<Session> SessionManager::sessionForSocket(WSocket *socket) const
{
    return findSession([&](const Session &s) { return s.socket == socket; });
}
```

This keeps functionality intact while cutting a lot of repetition and making the lookup semantics explicit.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@deepin-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: misaka18931

The full list of commands accepted by this bot can be found here.

Details 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

@deepin-ci-robot
Copy link

@misaka18931: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
github-trigger-obs-ci a804d37 link true /test github-trigger-obs-ci

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors session management by extracting session-related functionality from the Helper class into a new dedicated SessionManager singleton. The goal is to improve code organization and separation of concerns.

Changes:

  • Created new SessionManager class with Session struct in dedicated session/ directory
  • Moved all session management logic (create, remove, lookup, activation) from Helper to SessionManager
  • Reordered header includes and forward declarations alphabetically in helper.h and helper.cpp

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
src/session/session.h New header defining Session struct and SessionManager singleton class
src/session/session.cpp Implementation of SessionManager with all session management logic moved from Helper
src/seat/helper.h Removed Session/session-related declarations, reordered includes alphabetically, changed rootContainer to rootSurfaceContainer
src/seat/helper.cpp Removed session implementation, added createXWayland() method, updated to delegate to SessionManager
src/modules/shortcut/shortcutrunner.cpp Updated rootContainer() call to rootSurfaceContainer()
src/modules/shortcut/shortcutmanager.cpp Updated to use SessionManager::instance() instead of Helper
src/modules/foreign-toplevel/foreigntoplevelmanagerv1.cpp Updated rootContainer() call to rootSurfaceContainer()
src/greeter/greeterproxy.cpp Updated to use SessionManager::instance() instead of Helper for session access
src/core/treeland.cpp Created SessionManager instance and updated to use SessionManager for session operations
src/CMakeLists.txt Added new session.cpp and session.h files to build

@deepin-bot
Copy link

deepin-bot bot commented Jan 15, 2026

TAG Bot

New tag: 0.8.1
DISTRIBUTION: unstable
Suggest: synchronizing this PR through rebase #691

@misaka18931 misaka18931 force-pushed the move-session branch 3 times, most recently from cf47127 to cf24ebf Compare January 19, 2026 06:16
Changes:
- reordered header & forward class decls alphabetically in helper.h and
  helper.cpp
- removed duplicate getter for `m_rootSurfaceContainer` in `Helper`.
- Moved `class Session` and session management related member functions
  into the new `class SessionManager`

SessionManager is designed to be an independent singleton that
exclusively manages session creation, invalidation and its related
WSockets and WWayland instances.

if (CmdLine::ref().run().has_value()) {
auto exec = [runCmd = CmdLine::ref().run().value(), d] {
auto exec = [runCmd = CmdLine::ref().run().value(), d, &defaultSession] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个 defaultSession 不是个指针吗,用&有啥意义吗

// assert(wlroots: assert(wl_list_empty(&cur->events.button.listener_list)))
// failed during quit(If the quit call is from the cursor's button press/release event)
connect(qmlEngine, &QQmlEngine::quit, q, &Treeland::quit, Qt::QueuedConnection);
sessionManager = new SessionManager();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个对象在哪里销毁的?


d->init();

auto defaultSession = d->sessionManager->defaultSession();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个是defaultSession还是globalSession,如果之前是叫global的,就不要改名叫default。

Q_EMIT isLoggedInChanged();
auto session = Helper::instance()->activeSession().lock();
SocketWriter(d->socket) << quint32(GreeterMessages::Logout) << session->id;
auto session = SessionManager::instance()->activeSession().lock();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

单例建议只保留一个 Helper 对象,用法上可以 Helper::instance()->sessionManager()->activeSession().lock()

所有的内部接口都通过Helper对象暴露。

xwaylandOutputManager->setFilter([this](WClient *client) { return isXWaylandClient(client); });

static const auto isXWaylandClient = [](WClient *client) {
for (auto session : SessionManager::instance()->sessions()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

确保 SessionManager::instance()->sessions() 是个 const 类型,如果不是,可以用 const auto &xxx = SessionManager::instance()->sessions(); 存储一个临时值。

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