platform: refactor Win32ConsoleStrategy

The current platform classes have many issues. One of them is that they interact with the terminal in the constructor. This makes it impossible for an strategy to "fail" so that we can fall back on another one, unless we use exceptions, which I'm not willing to use because I think they are not the best solution. This commit changes several things: first, it fixes the "fail" issue for Win32ConsoleStrategy by hiding the constructor and having a public static 'create' function which returns nullptr on error. Second, it reduces the amount of state by splitting the member variables between Win32Input and Win32Display, so that these don't need a reference to Win32ConsoleStrategy. Third, it moves the handle ownership handling to StdioCtl, so that the global TermIO::consoleWrite can be unified. The console detection has also been improved and it is now more resilient and it finally supports spawning a new console when none is available (fixes #25). There's still room for improvement. I look forward to having just PlatformStrategy as singleton instead of StdioCtl. StdioCtl functionality should be moved into PlatformStrategy, and TermIO functions would receive a pointer to PlatformStrategy instead of referring to the StdioCtl singleton. StdioCtl is currently the equivalent of passing parameters by global variables. Also, it can be seen clearly that the resource lifetime of StdioCtl on Windows is tied to Win32ConsoleStrategy's lifetime. This is why I believe they should both be part of the same object.
magiblot · Jan 22, 2021 · 3eb1aaf · 3eb1aaf
1 parent 188009b
commit 3eb1aaf
Show file tree

Hide file tree

Showing 12 changed files with 351 additions and 213 deletions.
diff --git a/include/tvision/hardware.h b/include/tvision/hardware.h
@@ -54,9 +54,6 @@ class THardwareInfo
     static TEvent eventQ[eventQSize];
     static size_t eventCount;
     static void flushScreen();
-    static bool isLinuxConsole();
-    static bool isWin32Console();
-    static void consoleWrite(const void*, size_t);
 #endif
 
     static ulong getTickCount();

diff --git a/include/tvision/internal/linuxcon.h b/include/tvision/internal/linuxcon.h
@@ -22,7 +22,8 @@ class LinuxConsoleStrategy : public UnixPlatformStrategy {
 
 public:
 
-    LinuxConsoleStrategy(DisplayStrategy*, FdInputStrategy*);
+    LinuxConsoleStrategy( std::unique_ptr<DisplayStrategy> &&,
+                          std::unique_ptr<FdInputStrategy> && );
 
     int getButtonCount();
 

diff --git a/include/tvision/internal/platform.h b/include/tvision/internal/platform.h
@@ -79,10 +79,12 @@ class PlatformStrategy {
 public:
 
     PlatformStrategy() {}
-    PlatformStrategy(DisplayStrategy* d, InputStrategy *i) :
-        display(d), input(i)
+    PlatformStrategy( std::unique_ptr<DisplayStrategy> &&d,
+                      std::unique_ptr<InputStrategy> &&i ) :
+        display(std::move(d)),
+        input(std::move(i))
     {
-        if (d) d->reloadScreenInfo();
+        if (display) display->reloadScreenInfo();
     }
 
     virtual ~PlatformStrategy() {}

diff --git a/include/tvision/internal/stdioctl.h b/include/tvision/internal/stdioctl.h
@@ -32,18 +32,34 @@ class StdioCtl {
 
 };
 
-#else
+#elif defined(_WIN32)
 
-class StdioCtl {
+#include <tvision/compat/win.h>
+
+class StdioCtl
+{
+
+    friend class Win32ConsoleStrategy;
+
+    enum { input = 0, activeOutput = 1, startupOutput = 2 };
+
+    struct
+    {
+        HANDLE handle {INVALID_HANDLE_VALUE};
+        bool owning {false};
+    } cn[3];
+
+    bool ownsConsole {};
+
+    void setUp();
+    void tearDown();
+
+    static StdioCtl instance;
 
 public:
 
-    static int in() { return 0; }
-    static int out() { return 1; }
-    static int err() { return 2; }
-    static FILE *fin() { return stdin; }
-    static FILE *fout() { return stdout; }
-    static FILE *ferr() { return stderr; }
+    static HANDLE in() { return instance.cn[input].handle; }
+    static HANDLE out() { return instance.cn[activeOutput].handle; }
 
 };
 

diff --git a/include/tvision/internal/terminal.h b/include/tvision/internal/terminal.h
@@ -96,12 +96,27 @@ namespace TermIO
     bool acceptMouseEvent(TEvent &ev, MouseState &oldm, const MouseState &newm);
     void setAltModifier(TEvent &ev);
 
-    #ifdef _TV_UNIX
+#ifdef _TV_UNIX
     namespace Unix
     {
+        void consoleWrite(const void *data, size_t bytes);
         TPoint getSize();
     }
-    #endif // _TV_UNIX
+    namespace Impl = Unix;
+#elif defined(_WIN32)
+    namespace Win32
+    {
+        void consoleWrite(const void *data, size_t bytes);
+    }
+    namespace Impl = Win32;
+#endif // _TV_UNIX
+
+    bool isLinuxConsole();
+
+    inline void consoleWrite(const void *data, size_t bytes)
+    {
+        Impl::consoleWrite(data, bytes);
+    }
 
 }
 

diff --git a/include/tvision/internal/win32con.h b/include/tvision/internal/win32con.h
@@ -14,56 +14,39 @@ class Win32Display;
 class Win32ConsoleStrategy : public PlatformStrategy
 {
 
-    friend class Win32Display;
-    friend class Win32Input;
+    UINT cpInput, cpOutput;
 
-    enum ConsoleType { cnInput = 0, cnOutput = 1, cnStartup = 2 };
-
-    HANDLE consoleHandle[3];
-    UINT consoleCp[2];
-    DWORD consoleMode[2];
-    CONSOLE_SCREEN_BUFFER_INFO sbInfo {};
+    Win32ConsoleStrategy( UINT cpInput, UINT cpOutput,
+                          std::unique_ptr<DisplayStrategy> &&display,
+                          std::unique_ptr<InputStrategy> &&input );
+    ~Win32ConsoleStrategy();
 
-    void initConsole();
+    static bool initConsole( UINT &cpInput, UINT &cpOutput,
+                             std::unique_ptr<DisplayStrategy> &display,
+                             std::unique_ptr<InputStrategy> &input );
     void restoreConsole();
     void resetConsole();
-    void reloadScreenBufferInfo();
-
-    static Win32ConsoleStrategy *instance;
 
 public:
 
-    Win32ConsoleStrategy();
-    ~Win32ConsoleStrategy();
+    static Win32ConsoleStrategy *create();
 
     bool waitForEvent(long ms, TEvent &ev) override;
-
-    static void write(const void *data, size_t bytes);
-
 };
 
-inline void Win32ConsoleStrategy::write(const void *data, size_t bytes)
-{
-    if (instance)
-        WriteConsole(instance->consoleHandle[cnOutput], data, bytes, nullptr, nullptr);
-}
-
 class Win32Input : public InputStrategy
 {
 
-    Win32ConsoleStrategy &cnState;
     bool insertState;
     ushort surrogate;
 
     bool getKeyEvent(KEY_EVENT_RECORD, TEvent &ev);
     bool getUnicodeEvent(KEY_EVENT_RECORD, TEvent &ev);
     bool getMouseEvent(MOUSE_EVENT_RECORD, TEvent &ev);
 
-    HANDLE cnHandle() const;
-
 public:
 
-    Win32Input(Win32ConsoleStrategy &);
+    Win32Input();
 
     int getButtonCount() override;
     void cursorOn() override;
@@ -72,24 +55,16 @@ class Win32Input : public InputStrategy
 
 };
 
-inline HANDLE Win32Input::cnHandle() const
-{
-    return cnState.consoleHandle[0];
-}
-
 class Win32Display : public BufferedDisplay
 {
 
-    Win32ConsoleStrategy &cnState;
-
+    COORD dwSize;
     uchar lastAttr;
     std::vector<char> buf;
 
-    HANDLE cnHandle() const;
-
 public:
 
-    Win32Display(Win32ConsoleStrategy &);
+    Win32Display();
 
     void reloadScreenInfo() override;
     TPoint getScreenSize() override;
@@ -107,11 +82,6 @@ class Win32Display : public BufferedDisplay
 
 };
 
-inline HANDLE Win32Display::cnHandle() const
-{
-    return cnState.consoleHandle[1];
-}
-
 #endif // _WIN32
 
 #endif // WIN32CON_H
diff --git a/source/platform/ansidisp.cpp b/source/platform/ansidisp.cpp
@@ -1,19 +1,10 @@
-#define Uses_TScreen
-#define Uses_THardwareInfo
-#include <tvision/tv.h>
 #include <internal/ansidisp.h>
 #include <internal/codepage.h>
 #include <internal/textattr.h>
-#include <internal/stdioctl.h>
 #include <internal/terminal.h>
 #include <internal/strings.h>
 #include <cstdio>
 #include <cstdlib>
-#ifdef _TV_UNIX
-#include <termios.h>
-#include <sys/ioctl.h>
-#include <unistd.h>
-#endif
 #ifdef HAVE_NCURSES
 #include <ncurses.h> // For COLORS
 #else
@@ -28,7 +19,7 @@ AnsiDisplayBase::AnsiDisplayBase() :
     lastAttr(SGRAttribs::defaultInit),
     sgrFlags(0)
 {
-    if (THardwareInfo::isLinuxConsole())
+    if (TermIO::isLinuxConsole())
         sgrFlags |= sgrBrightIsBlink | sgrNoItalic | sgrNoUnderline;
     if (COLORS < 16)
         sgrFlags |= sgrBrightIsBold;
@@ -98,7 +89,7 @@ void AnsiDisplayBase::lowlevelMoveCursor(uint x, uint y)
 }
 
 void AnsiDisplayBase::lowlevelFlush() {
-    THardwareInfo::consoleWrite(buf.data(), buf.size());
+    TermIO::consoleWrite(buf.data(), buf.size());
     buf.resize(0);
 }
 

diff --git a/source/platform/hardware.cpp b/source/platform/hardware.cpp
@@ -11,7 +11,7 @@
 #include <internal/ansidisp.h>
 #include <internal/linuxcon.h>
 #include <internal/sighandl.h>
-#include <internal/stdioctl.h>
+#include <internal/terminal.h>
 #include <internal/getenv.h>
 #include <chrono>
 #ifdef _TV_UNIX
@@ -37,44 +37,6 @@ THardwareInfo::THardwareInfo()
 #endif
 }
 
-bool THardwareInfo::isLinuxConsole()
-{
-#ifdef __linux__
-    /* This is the same function used to get the Shift/Ctrl/Alt modifiers
-     * on the console. It fails if stdin is not a console file descriptor. */
-    for (int fd : {StdioCtl::in(), StdioCtl::out()})
-    {
-        char subcode[] = {6, 0}; // Null-terminate so that valgrind doesn't complain.
-        if (ioctl(fd, TIOCLINUX, subcode) != -1)
-            return true;
-    }
-    return false;
-#else
-    return false;
-#endif
-}
-
-bool THardwareInfo::isWin32Console()
-{
-#ifdef _WIN32
-    DWORD consoleMode;
-    return GetConsoleMode( GetStdHandle(STD_INPUT_HANDLE), &consoleMode ) != 0;
-#else
-    return false;
-#endif
-}
-
-void THardwareInfo::consoleWrite(const void *data, size_t bytes)
-{
-#ifdef _WIN32
-    Win32ConsoleStrategy::write(data, bytes);
-#else
-    fflush(StdioCtl::fout());
-    int rr = ::write(StdioCtl::out(), data, bytes);
-    (void) rr;
-#endif
-}
-
 /* We don't include these in hardware.h as done originally to prevent it to
  * depend on platform.h. Otherwise, any change in platform.h would affect
  * hardware.h, causing the whole tvision library to recompile. */
@@ -111,24 +73,23 @@ void THardwareInfo::setUpConsole()
     if (platf == &nullPlatf)
     {
 #ifdef _WIN32
-        if (isWin32Console())
-            platf = new Win32ConsoleStrategy();
-        else
+        if (!(platf = Win32ConsoleStrategy::create()))
         {
-            cerr << "Error: standard input is being redirected or is not a "
-                    "Win32 console." << endl;
+            cerr << "Error: cannot get a console." << endl;
             ExitProcess(1);
         }
 #else
-        DisplayStrategy *disp;
+        std::unique_ptr<DisplayStrategy> disp;
         if (getEnv<TStringView>("TVISION_DISPLAY") == "ncurses")
-            disp = new NcursesDisplay();
+            disp = std::make_unique<NcursesDisplay>();
         else
-            disp = new AnsiDisplay<NcursesDisplay>();
-        if (isLinuxConsole())
-            platf = new LinuxConsoleStrategy(disp, new NcursesInput(false));
+            disp = std::make_unique<AnsiDisplay<NcursesDisplay>>();
+        if (TermIO::isLinuxConsole())
+            platf = new LinuxConsoleStrategy( std::move(disp),
+                                              std::make_unique<NcursesInput>(false) );
         else
-            platf = new UnixPlatformStrategy(disp, new NcursesInput());
+            platf = new UnixPlatformStrategy( std::move(disp),
+                                              std::make_unique<NcursesInput>() );
 #endif
     }
 }

diff --git a/source/platform/linuxcon.cpp b/source/platform/linuxcon.cpp
@@ -45,8 +45,10 @@ static constexpr auto keyCodeWithAlt = constexpr_map<ushort, ushort>::from_array
     { kbDown,       kbAltDown       },
 });
 
-LinuxConsoleStrategy::LinuxConsoleStrategy(DisplayStrategy *d, FdInputStrategy *i) :
-    UnixPlatformStrategy(d, i), gpm(new GpmInput())
+LinuxConsoleStrategy::LinuxConsoleStrategy( std::unique_ptr<DisplayStrategy> &&d,
+                                            std::unique_ptr<FdInputStrategy> &&i ) :
+    UnixPlatformStrategy(std::move(d), std::move(i)),
+    gpm(std::make_unique<GpmInput>())
 {
     /* The FdInputStrategy instance which reads key events is stored in
      * the 'input' attribute of PlatformStrategy, while the GpmInput instance