Changeset View
Standalone View
wayland_server.cpp
Show First 20 Lines • Show All 499 Lines • ▼ Show 20 Line(s) | 495 | if (hasScreenLockerIntegration()) { | |||
---|---|---|---|---|---|
500 | } | 500 | } | ||
501 | } else { | 501 | } else { | ||
502 | emit initialized(); | 502 | emit initialized(); | ||
503 | } | 503 | } | ||
504 | } | 504 | } | ||
505 | 505 | | |||
506 | void WaylandServer::initScreenLocker() | 506 | void WaylandServer::initScreenLocker() | ||
507 | { | 507 | { | ||
508 | ScreenLocker::KSldApp::self(); | 508 | auto *screenLockerApp = ScreenLocker::KSldApp::self(); | ||
509 | ScreenLocker::KSldApp::self()->setWaylandDisplay(m_display); | 509 | | ||
apol: Sounds like if KSldApp wasn't a singleton much of the cleanup code would be much simpler. It… | |||||
Oh yea. I guess it would also make the autotest easier to maintain since you could just reset the screen locker. (In regards to the autotest see also: https://bugreports.qt.io/browse/QTBUG-82911) And actually I tried to make it a member of WaylandServer (and already replaced all the KSldApp::self() getters in the code base with some waylandServer()->screenLocker() getter). But crudely enough the screen locker is already queried before the WaylandServer singleton is created by classes like PointerInputRedirection. What sounds kind of wrong. But to tackle this there would be a larger refactoring needed for the startup order or with which class the screen locker interfaces. romangg: Oh yea. I guess it would also make the autotest easier to maintain since you could just reset… | |||||
not sure that's the right. KSLDApp is the process that launches the screenlocker when needed, it has to have the lifespan of the session. davidedmundson: not sure that's the right.
KSLDApp is the process that launches the screenlocker when needed… | |||||
510 | ScreenLocker::KSldApp::self()->setGreeterEnvironment(kwinApp()->processStartupEnvironment()); | 510 | ScreenLocker::KSldApp::self()->setGreeterEnvironment(kwinApp()->processStartupEnvironment()); | ||
511 | ScreenLocker::KSldApp::self()->initialize(); | 511 | ScreenLocker::KSldApp::self()->initialize(); | ||
512 | 512 | | |||
513 | connect(ScreenLocker::KSldApp::self(), &ScreenLocker::KSldApp::greeterClientConnectionChanged, this, | 513 | connect(ScreenLocker::KSldApp::self(), &ScreenLocker::KSldApp::aboutToLock, this, | ||
514 | [this] () { | 514 | [this, screenLockerApp] () { | ||
515 | m_screenLockerClientConnection = ScreenLocker::KSldApp::self()->greeterClientConnection(); | 515 | if (m_screenLockerClientConnection) { | ||
516 | // Already sent data to KScreenLocker. | ||||
517 | return; | ||||
518 | } | ||||
519 | int clientFd = createScreenLockerConnection(); | ||||
520 | if (clientFd < 0) { | ||||
521 | return; | ||||
522 | } | ||||
523 | ScreenLocker::KSldApp::self()->setWaylandFd(clientFd); | ||||
davidedmundson: why do we do this on every lock? | |||||
Because the client connection is reset after every lock/unlock and I'm not sure it's possible to reuse the same socket for a new client. The connection is accessed by the greeter. And if the greeter exits the client connection is destroyed by callback. romangg: Because the client connection is reset after every lock/unlock and I'm not sure it's possible… | |||||
524 | | ||||
525 | for (auto *seat : m_display->seats()) { | ||||
526 | connect(seat, &KWayland::Server::SeatInterface::timestampChanged, | ||||
Can this be done the other way round in the seat constructor then we get rid of all these connects and disconnects davidedmundson: Can this be done the other way round in the seat constructor
then we get rid of all these… | |||||
Yea, when the KSldApp is a undestroyed singleton sure. Having it here improves locality imo but I can also put it in the constructors. romangg: Yea, when the KSldApp is a undestroyed singleton sure. Having it here improves locality imo but… | |||||
527 | screenLockerApp, &ScreenLocker::KSldApp::userActivity); | ||||
528 | } | ||||
516 | } | 529 | } | ||
517 | ); | 530 | ); | ||
518 | 531 | | |||
519 | connect(ScreenLocker::KSldApp::self(), &ScreenLocker::KSldApp::unlocked, this, | 532 | connect(ScreenLocker::KSldApp::self(), &ScreenLocker::KSldApp::unlocked, this, | ||
520 | [this] () { | 533 | [this, screenLockerApp] () { | ||
534 | if (m_screenLockerClientConnection) { | ||||
535 | m_screenLockerClientConnection->destroy(); | ||||
536 | delete m_screenLockerClientConnection; | ||||
521 | m_screenLockerClientConnection = nullptr; | 537 | m_screenLockerClientConnection = nullptr; | ||
522 | } | 538 | } | ||
539 | | ||||
540 | for (auto *seat : m_display->seats()) { | ||||
541 | disconnect(seat, &KWayland::Server::SeatInterface::timestampChanged, | ||||
542 | screenLockerApp, &ScreenLocker::KSldApp::userActivity); | ||||
543 | } | ||||
544 | ScreenLocker::KSldApp::self()->setWaylandFd(-1); | ||||
545 | } | ||||
523 | ); | 546 | ); | ||
524 | 547 | | |||
525 | if (m_initFlags.testFlag(InitializationFlag::LockScreen)) { | 548 | if (m_initFlags.testFlag(InitializationFlag::LockScreen)) { | ||
526 | ScreenLocker::KSldApp::self()->lock(ScreenLocker::EstablishLock::Immediate); | 549 | ScreenLocker::KSldApp::self()->lock(ScreenLocker::EstablishLock::Immediate); | ||
527 | } | 550 | } | ||
528 | emit initialized(); | 551 | emit initialized(); | ||
529 | } | 552 | } | ||
530 | 553 | | |||
531 | WaylandServer::SocketPairConnection WaylandServer::createConnection() | 554 | WaylandServer::SocketPairConnection WaylandServer::createConnection() | ||
532 | { | 555 | { | ||
533 | SocketPairConnection ret; | 556 | SocketPairConnection ret; | ||
534 | int sx[2]; | 557 | int sx[2]; | ||
535 | if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, sx) < 0) { | 558 | if (socketpair(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0, sx) < 0) { | ||
536 | qCWarning(KWIN_CORE) << "Could not create socket"; | 559 | qCWarning(KWIN_CORE) << "Could not create socket"; | ||
537 | return ret; | 560 | return ret; | ||
538 | } | 561 | } | ||
539 | ret.connection = m_display->createClient(sx[0]); | 562 | ret.connection = m_display->createClient(sx[0]); | ||
540 | ret.fd = sx[1]; | 563 | ret.fd = sx[1]; | ||
541 | return ret; | 564 | return ret; | ||
542 | } | 565 | } | ||
543 | 566 | | |||
567 | int WaylandServer::createScreenLockerConnection() | ||||
568 | { | ||||
569 | const auto socket = createConnection(); | ||||
570 | if (!socket.connection) { | ||||
571 | return -1; | ||||
572 | } | ||||
573 | m_screenLockerClientConnection = socket.connection; | ||||
574 | connect(m_screenLockerClientConnection, &KWayland::Server::ClientConnection::disconnected, | ||||
575 | this, [this] { m_screenLockerClientConnection = nullptr; }); | ||||
davidedmundson: who deletes m_screenLockerClientConnection in this case? | |||||
When the client goes down libwayland calls into the client connection destroy callback and that deletes the client here: At this point trying to delete the connection from KWin's side would be a double-free. If you think this API is not so intuitive don't look in the Client lib. :) romangg: When the client goes down libwayland calls into the client connection destroy callback and that… | |||||
davidedmundson: I don't like that client API :/
But this code makes sense then. | |||||
576 | return socket.fd; | ||||
577 | } | ||||
578 | | ||||
544 | int WaylandServer::createXWaylandConnection() | 579 | int WaylandServer::createXWaylandConnection() | ||
545 | { | 580 | { | ||
546 | const auto socket = createConnection(); | 581 | const auto socket = createConnection(); | ||
547 | if (!socket.connection) { | 582 | if (!socket.connection) { | ||
548 | return -1; | 583 | return -1; | ||
549 | } | 584 | } | ||
550 | m_xwayland.client = socket.connection; | 585 | m_xwayland.client = socket.connection; | ||
551 | m_xwayland.destroyConnection = connect(m_xwayland.client, &KWayland::Server::ClientConnection::disconnected, this, | 586 | m_xwayland.destroyConnection = connect(m_xwayland.client, &KWayland::Server::ClientConnection::disconnected, this, | ||
▲ Show 20 Lines • Show All 240 Lines • Show Last 20 Lines |
Sounds like if KSldApp wasn't a singleton much of the cleanup code would be much simpler. It could maybe make sense to revisit this?