Merge pull request #777 from contour-terminal/improvement/pty-deferre…

…d-start

defer PTY (and Process) creation

Changelog.md

@@ -6,6 +6,7 @@
 - [Linux] Provide an AppStream XML file.
 - [Linux] Drop KDE/KWin dependency on the binary by implementing enabling blur-behind background manually.
 - [Linux] Adds support for blur-behind window on GNOME shell (Please read https://github.com/aunetx/blur-my-shell/issues/300 for further details if in trouble).
+- Changes behavior of PTY (and shell process) creation until only when a PTY is required by the terminal emulator during instanciation, possibly avoiding problems with xdotool running too early.
 - Internal: Y-axis inverted to match GUI coordinate systems where (0, 0) is top left rather than bottom left.
 - Fixes logging file toggle.
 

src/contour/TerminalSession.cpp

@@ -167,6 +167,7 @@ void TerminalSession::scheduleRedraw()
 
 void TerminalSession::start()
 {
+    terminal_.device().start();
     screenUpdateThread_ = make_unique<std::thread>(bind(&TerminalSession::mainLoop, this));
 }
 

src/contour/display/TerminalWidget.cpp

@@ -590,6 +590,11 @@ float TerminalWidget::uptime() const noexcept
 
 void TerminalWidget::onFrameSwapped()
 {
+    // This test here is solely to prevent a crash in case an OpenGL context could not be acquired.
+    // Then we also do not have a session.
+    if (!session_)
+        return;
+
     if (!state_.finish())
         update();
     else if (auto timeout = terminal().nextRender(); timeout.has_value())
@@ -626,13 +631,15 @@ void TerminalWidget::mouseReleaseEvent(QMouseEvent* _event)
 void TerminalWidget::focusInEvent(QFocusEvent* _event)
 {
     QOpenGLWidget::focusInEvent(_event);
-    session_->sendFocusInEvent(); // TODO: paint with "normal" colors
+    if (session_)
+        session_->sendFocusInEvent(); // TODO: paint with "normal" colors
 }
 
 void TerminalWidget::focusOutEvent(QFocusEvent* _event)
 {
     QOpenGLWidget::focusOutEvent(_event);
-    session_->sendFocusOutEvent(); // TODO maybe paint with "faint" colors
+    if (session_)
+        session_->sendFocusOutEvent(); // TODO maybe paint with "faint" colors
 }
 
 void TerminalWidget::inputMethodEvent(QInputMethodEvent* _event)

src/terminal/Process.h

@@ -95,6 +95,7 @@ class [[nodiscard]] Process: public Pty
 
     // Pty overrides
     // clang-format off
+    void start() override;
     PtySlave& slave() noexcept override { return pty().slave(); }
     void close() override { pty().close(); }
     bool isClosed() const noexcept override { return pty().isClosed(); }

src/terminal/Process_unix.cpp

@@ -113,8 +113,13 @@ namespace
 
 struct Process::Private
 {
-    mutable pid_t pid {};
+    string path;
+    vector<string> args;
+    FileSystem::path cwd;
+    Environment env;
+
     unique_ptr<Pty> pty {};
+    mutable pid_t pid {};
     mutable std::mutex exitStatusMutex {};
     mutable std::optional<Process::ExitStatus> exitStatus {};
 
@@ -126,10 +131,15 @@ Process::Process(string const& _path,
                  FileSystem::path const& _cwd,
                  Environment const& _env,
                  unique_ptr<Pty> _pty):
-    d(new Private {}, [](Private* p) { delete p; })
+    d(new Private { _path, _args, _cwd, _env, move(_pty) }, [](Private* p) { delete p; })
 {
+}
+
+void Process::start()
+{
+    d->pty->start();
+
     d->pid = fork();
-    d->pty = move(_pty);
 
     UnixPipe* stdoutFastPipe = [this]() -> UnixPipe* {
         if (auto* p = dynamic_cast<SystemPty*>(d->pty.get()))
@@ -150,42 +160,42 @@ Process::Process(string const& _path,
         {
             (void) d->pty->slave().login();
 
-            auto const& cwd = _cwd.generic_string();
+            auto const& cwd = d->cwd.generic_string();
             if (!isFlatpak())
             {
-                if (!_cwd.empty() && chdir(cwd.c_str()) != 0)
+                if (!d->cwd.empty() && chdir(cwd.c_str()) != 0)
                 {
                     printf("Failed to chdir to \"%s\". %s\n", cwd.c_str(), strerror(errno));
                     exit(EXIT_FAILURE);
                 }
 
-                for (auto&& [name, value]: _env)
+                for (auto&& [name, value]: d->env)
                     setenv(name.c_str(), value.c_str(), true);
 
                 if (stdoutFastPipe)
                     setenv(StdoutFastPipeEnvironmentName.data(), StdoutFastPipeFdStr.data(), true);
             }
 
-            char** argv = [=]() -> char** {
+            char** argv = [stdoutFastPipe, this]() -> char** {
                 if (!isFlatpak())
-                    return createArgv(_path, _args, 0);
+                    return createArgv(d->path, d->args, 0);
 
                 // Prepend flatpak to jump out of sandbox:
                 // flatpak-spawn --host --watch-bus --env=TERM=$TERM /bin/zsh
                 auto realArgs = std::vector<string> {};
                 realArgs.emplace_back("--host");
                 realArgs.emplace_back("--watch-bus");
-                if (!_cwd.empty())
-                    realArgs.emplace_back(fmt::format("--directory={}", cwd));
+                if (!d->cwd.empty())
+                    realArgs.emplace_back(fmt::format("--directory={}", d->cwd.generic_string()));
                 if (auto const value = getenv("TERM"))
                     realArgs.emplace_back(fmt::format("--env=TERM={}", value));
-                for (auto&& [name, value]: _env)
+                for (auto&& [name, value]: d->env)
                     realArgs.emplace_back(fmt::format("--env={}={}", name, value));
                 if (stdoutFastPipe)
                     realArgs.emplace_back(
                         fmt::format("--env={}={}", StdoutFastPipeEnvironmentName, StdoutFastPipeFd));
-                realArgs.push_back(_path);
-                for (auto const& arg: _args)
+                realArgs.push_back(d->path);
+                for (auto const& arg: d->args)
                     realArgs.push_back(arg);
 
                 return createArgv("/usr/bin/flatpak-spawn", realArgs, 0);

src/terminal/Process_win32.cpp

@@ -154,13 +154,17 @@ namespace
 
 struct Process::Private
 {
+    string path;
+    vector<string> args;
+    FileSystem::path cwd;
+    Environment env;
+    std::unique_ptr<Pty> pty {};
+
     mutable HANDLE pid {};
     mutable std::mutex exitStatusMutex {};
     mutable std::optional<Process::ExitStatus> exitStatus {};
     std::optional<std::thread> exitWatcher;
 
-    std::unique_ptr<Pty> pty {};
-
     PROCESS_INFORMATION processInfo {};
     STARTUPINFOEX startupInfo {};
 
@@ -172,26 +176,29 @@ Process::Process(string const& _path,
                  FileSystem::path const& _cwd,
                  Environment const& _env,
                  std::unique_ptr<Pty> _pty):
-    d(new Private {}, [](Private* p) { delete p; })
+    d(new Private {_path, _args, _cwd, _env, move(_pty)}, [](Private* p) { delete p; })
+{
+}
+
+void Process::start()
 {
-    d->pty = move(_pty);
     Require(static_cast<ConPty const*>(d->pty.get()));
 
     initializeStartupInfoAttachedToPTY(d->startupInfo, static_cast<ConPty&>(*d->pty));
 
-    string cmd = _path;
-    for (size_t i = 0; i < _args.size(); ++i)
+    string cmd = d->path;
+    for (size_t i = 0; i < d->args.size(); ++i)
     {
         cmd += ' ';
-        if (_args[i].find(' ') != std::string::npos)
-            cmd += '\"' + _args[i] + '\"';
+        if (d->args[i].find(' ') != std::string::npos)
+            cmd += '\"' + d->args[i] + '\"';
         else
-            cmd += _args[i];
+            cmd += d->args[i];
     }
 
     // In case of PATH environment variable, extend it rather then overwriting it.
-    auto env = _env;
-    for (auto [name, value]: _env)
+    auto env = d->env;
+    for (auto const& [name, value]: d->env)
     {
         if (crispy::toUpper(name) == "PATH")
         {
@@ -203,7 +210,7 @@ Process::Process(string const& _path,
     }
     auto const envScope = InheritingEnvBlock { env };
 
-    auto const cwd = _cwd.generic_string();
+    auto const cwd = d->cwd.generic_string();
     auto const cwdPtr = !cwd.empty() ? cwd.c_str() : nullptr;
 
     PtyLog()("Creating process for command line: {}", cmd);

src/terminal/pty/ConPty.cpp

@@ -69,6 +69,25 @@ ConPty::ConPty(PageSize const& _windowSize): size_ { _windowSize }
     master_ = INVALID_HANDLE_VALUE;
     input_ = INVALID_HANDLE_VALUE;
     output_ = INVALID_HANDLE_VALUE;
+    buffer_.resize(10240);
+}
+
+ConPty::~ConPty()
+{
+    PtyLog()("~ConPty()");
+    close();
+}
+
+bool ConPty::isClosed() const noexcept
+{
+    return master_ == INVALID_HANDLE_VALUE;
+}
+
+void ConPty::start()
+{
+    PtyLog()("Starting ConPTY");
+    assert(!slave_);
+
     slave_ = make_unique<ConPtySlave>(output_);
 
     HANDLE hPipePTYIn { INVALID_HANDLE_VALUE };
@@ -85,11 +104,8 @@ ConPty::ConPty(PageSize const& _windowSize): size_ { _windowSize }
     }
 
     // Create the Pseudo Console of the required size, attached to the PTY-end of the pipes
-    HRESULT hr = CreatePseudoConsole({ unbox<SHORT>(_windowSize.columns), unbox<SHORT>(_windowSize.lines) },
-                                     hPipePTYIn,
-                                     hPipePTYOut,
-                                     0,
-                                     &master_);
+    HRESULT hr = CreatePseudoConsole(
+        { unbox<SHORT>(size_.columns), unbox<SHORT>(size_.lines) }, hPipePTYIn, hPipePTYOut, 0, &master_);
 
     if (hPipePTYIn != INVALID_HANDLE_VALUE)
         CloseHandle(hPipePTYIn);
@@ -99,19 +115,6 @@ ConPty::ConPty(PageSize const& _windowSize): size_ { _windowSize }
 
     if (hr != S_OK)
         throw runtime_error { GetLastErrorAsString() };
-
-    buffer_.resize(10240);
-}
-
-ConPty::~ConPty()
-{
-    PtyLog()("~ConPty()");
-    close();
-}
-
-bool ConPty::isClosed() const noexcept
-{
-    return master_ == INVALID_HANDLE_VALUE;
 }
 
 void ConPty::close()

src/terminal/pty/ConPty.h

@@ -31,6 +31,7 @@ class ConPty: public Pty
     explicit ConPty(PageSize const& windowSize);
     ~ConPty() override;
 
+    void start() override;
     void close() override;
     bool isClosed() const noexcept override;
 

src/terminal/pty/LinuxPty.cpp

@@ -163,14 +163,16 @@ int LinuxPty::Slave::write(std::string_view text) noexcept
 }
 // }}}
 
-LinuxPty::LinuxPty(PageSize const& _windowSize, optional<ImageSize> _pixels):
-    LinuxPty(createLinuxPty(_windowSize, _pixels), _windowSize)
+LinuxPty::LinuxPty(PageSize pageSize, optional<ImageSize> pixels): _pageSize { pageSize }, _pixels { pixels }
 {
 }
 
-LinuxPty::LinuxPty(PtyHandles handles, PageSize pageSize):
-    _masterFd { unbox<int>(handles.master) }, _pageSize { pageSize }, _slave { handles.slave }
+void LinuxPty::start()
 {
+    auto const handles = createLinuxPty(_pageSize, _pixels);
+    _masterFd = unbox<int>(handles.master);
+    _slave = std::make_unique<Slave>(handles.slave);
+
     if (!detail::setFileFlags(_masterFd, O_CLOEXEC | O_NONBLOCK))
         throw runtime_error { "Failed to configure PTY. "s + strerror(errno) };
 
@@ -210,7 +212,8 @@ LinuxPty::~LinuxPty()
 
 PtySlave& LinuxPty::slave() noexcept
 {
-    return _slave;
+    assert(_slave);
+    return *_slave;
 }
 
 PtyMasterHandle LinuxPty::handle() const noexcept

src/terminal/pty/LinuxPty.h

@@ -17,6 +17,7 @@
 #include <terminal/pty/UnixPty.h> // UnixPipe (TODO: move somewhere else)
 
 #include <array>
+#include <memory>
 #include <optional>
 #include <vector>
 
@@ -49,13 +50,13 @@ class LinuxPty: public Pty
         PtySlaveHandle slave;
     };
 
-    LinuxPty(PageSize const& _windowSize, std::optional<ImageSize> _pixels);
-    LinuxPty(PtyHandles handles, PageSize size);
+    LinuxPty(PageSize _windowSize, std::optional<ImageSize> _pixels);
     ~LinuxPty() override;
 
     PtySlave& slave() noexcept override;
 
     [[nodiscard]] PtyMasterHandle handle() const noexcept;
+    void start() override;
     void close() override;
     [[nodiscard]] bool isClosed() const noexcept override;
     void wakeupReader() noexcept override;
@@ -72,12 +73,13 @@ class LinuxPty: public Pty
     std::optional<std::string_view> readSome(int fd, char* target, size_t n) noexcept;
     int waitForReadable(std::chrono::milliseconds timeout) noexcept;
 
-    int _masterFd;
-    int _epollFd;
-    int _eventFd;
+    int _masterFd = -1;
+    int _epollFd = -1;
+    int _eventFd = -1;
     UnixPipe _stdoutFastPipe;
     PageSize _pageSize;
-    Slave _slave;
+    std::optional<ImageSize> _pixels;
+    std::unique_ptr<Slave> _slave;
 };
 
 } // namespace terminal

src/terminal/pty/MockPty.cpp

@@ -56,6 +56,11 @@ void MockPty::resizeScreen(PageSize _cells, std::optional<ImageSize> _pixels)
     pixelSize_ = _pixels;
 }
 
+void MockPty::start()
+{
+    closed_ = false;
+}
+
 void MockPty::close()
 {
     closed_ = true;

src/terminal/pty/MockPty.h

@@ -36,6 +36,7 @@ class MockPty: public Pty
     [[nodiscard]] PageSize pageSize() const noexcept override;
     void resizeScreen(PageSize _cells, std::optional<ImageSize> _pixels = std::nullopt) override;
 
+    void start() override;
     void close() override;
     [[nodiscard]] bool isClosed() const noexcept override;
 

src/terminal/pty/MockViewPty.cpp

@@ -66,6 +66,11 @@ void MockViewPty::resizeScreen(terminal::PageSize _cells, std::optional<terminal
     pixelSize_ = _pixels;
 }
 
+void MockViewPty::start()
+{
+    closed_ = false;
+}
+
 void MockViewPty::close()
 {
     closed_ = true;

src/terminal/pty/MockViewPty.h

@@ -35,6 +35,7 @@ class MockViewPty: public Pty
     [[nodiscard]] PageSize pageSize() const noexcept override;
     void resizeScreen(PageSize _cells, std::optional<ImageSize> _pixels = std::nullopt) override;
 
+    void start() override;
     void close() override;
     [[nodiscard]] bool isClosed() const noexcept override;
 

src/terminal/pty/Pty.h

@@ -66,6 +66,9 @@ class Pty
 
     virtual ~Pty() = default;
 
+    /// Starts the PTY instance.
+    virtual void start() = 0;
+
     virtual PtySlave& slave() noexcept = 0;
 
     /// Releases this PTY early.