A few more FrameBuffer cleanups.

- refactored a block of code that was essentially the same in 3 different places

src/common/FrameBufferSDL2.cxx

@@ -267,7 +267,7 @@ bool FrameBufferSDL2::activateVideoMode(const string& title,
         y1 = std::max(y1, rect.y + rect.h);
       }
     }
-    posX = BSPF::clamp(posX, x0 - Int32(mode.screen.w) + 50, x1 - 50);
+    posX = BSPF::clamp(posX, x0 - Int32(mode.screenS.w) + 50, x1 - 50);
     posY = BSPF::clamp(posY, y0 + 50, y1 - 50);
   }
 
@@ -292,8 +292,8 @@ bool FrameBufferSDL2::activateVideoMode(const string& title,
     int w, h;
 
     SDL_GetWindowSize(myWindow, &w, &h);
-    if(d != displayIndex || uInt32(w) != mode.screen.w || uInt32(h) != mode.screen.h
+    if(d != displayIndex || uInt32(w) != mode.screenS.w ||
-       || adaptRefresh)
+       uInt32(h) != mode.screenS.h || adaptRefresh)
     {
       SDL_DestroyWindow(myWindow);
       myWindow = nullptr;
@@ -310,7 +310,7 @@ bool FrameBufferSDL2::activateVideoMode(const string& title,
   {
     forceCreateRenderer = true;
     myWindow = SDL_CreateWindow(title.c_str(), posX, posY,
-                                mode.screen.w, mode.screen.h, flags);
+                                mode.screenS.w, mode.screenS.h, flags);
     if(myWindow == nullptr)
     {
       string msg = "ERROR: Unable to open SDL window: " + string(SDL_GetError());

src/common/VideoModeHandler.cxx

@@ -111,7 +111,7 @@ VideoModeHandler::Mode::Mode(uInt32 iw, uInt32 ih, uInt32 sw, uInt32 sh,
     fsIndex(fsindex)
 {
   // First set default size and positioning
-  screen = Common::Size(sw, sh);
+  screenS = Common::Size(sw, sh);
 
   // Now resize based on windowed/fullscreen mode and stretch factor
   if(fsIndex != -1)  // fullscreen mode
@@ -119,22 +119,20 @@ VideoModeHandler::Mode::Mode(uInt32 iw, uInt32 ih, uInt32 sw, uInt32 sh,
     switch(stretch)
     {
       case Stretch::Preserve:
-      {
         iw *= overscan;
         ih *= overscan;
         break;
-      }
 
       case Stretch::Fill:
         // Scale to all available space
-        iw = screen.w * overscan;
+        iw = screenS.w * overscan;
-        ih = screen.h * overscan;
+        ih = screenS.h * overscan;
         break;
 
       case Stretch::None:
         // Don't do any scaling at all
-        iw = std::min(iw, screen.w) * overscan;
+        iw = std::min(iw, screenS.w) * overscan;
-        ih = std::min(ih, screen.h) * overscan;
+        ih = std::min(ih, screenS.h) * overscan;
         break;
     }
   }
@@ -146,19 +144,22 @@ VideoModeHandler::Mode::Mode(uInt32 iw, uInt32 ih, uInt32 sw, uInt32 sh,
     {
       case Stretch::Preserve:
       case Stretch::Fill:
-        screen.w = iw;
+        screenS.w = iw;
-        screen.h = ih;
+        screenS.h = ih;
         break;
+
       case Stretch::None:
         break;  // Do not change image or screen rects whatsoever
     }
   }
 
   // Now re-calculate the dimensions
-  iw = std::min(iw, screen.w);
+  iw = std::min(iw, screenS.w);
-  ih = std::min(ih, screen.h);
+  ih = std::min(ih, screenS.h);
 
-  image.moveTo((screen.w - iw) >> 1, (screen.h - ih) >> 1);
+  imageR.moveTo((screenS.w - iw) >> 1, (screenS.h - ih) >> 1);
-  image.setWidth(iw);
+  imageR.setWidth(iw);
-  image.setHeight(ih);
+  imageR.setHeight(ih);
+
+  screenR = Common::Rect(screenS);
 }

src/common/VideoModeHandler.hxx

@@ -39,8 +39,9 @@ class VideoModeHandler
         None        // No stretching (1x zoom)
       };
 
-      Common::Rect image;
+      Common::Rect imageR;
-      Common::Size screen;
+      Common::Rect screenR;
+      Common::Size screenS;
       Stretch stretch{Mode::Stretch::None};
       string description;
       float zoom{1.F};
@@ -55,7 +56,7 @@ class VideoModeHandler
 
       friend ostream& operator<<(ostream& os, const Mode& vm)
       {
-        os << "image=" << vm.image << "  screen=" << vm.screen
+        os << "image=" << vm.imageR << "  screen=" << vm.screenS
            << "  stretch=" << (vm.stretch == Stretch::Preserve ? "preserve" :
                                vm.stretch == Stretch::Fill ? "fill" : "none")
            << "  desc=" << vm.description << "  zoom=" << vm.zoom
@@ -93,7 +94,8 @@ class VideoModeHandler
 
       @return  A video mode based on the given criteria
     */
-    const VideoModeHandler::Mode& buildMode(const Settings& settings, bool inTIAMode);
+    const VideoModeHandler::Mode& buildMode(const Settings& settings,
+                                            bool inTIAMode);
 
   private:
     Common::Size myImage, myDisplay;

src/emucore/EventHandler.hxx

@@ -102,7 +102,7 @@ class EventHandler
     void poll(uInt64 time);
 
     /**
-      Get/set the current state of the EventHandler
+      Get/set the current state of the EventHandler.
 
       @return The EventHandlerState type
     */
@@ -110,7 +110,18 @@ class EventHandler
     void setState(EventHandlerState state);
 
     /**
-      Resets the state machine of the EventHandler to the defaults
+      Convenience method that checks if we're in TIA mode.
+
+      @return Whether TIA mode is active
+    */
+    bool inTIAMode() const {
+      return !(myState == EventHandlerState::DEBUGGER ||
+               myState == EventHandlerState::LAUNCHER ||
+               myState == EventHandlerState::NONE);
+    }
+
+    /**
+      Resets the state machine of the EventHandler to the defaults.
 
       @param state  The current state to set
     */

src/emucore/FrameBuffer.cxx

@@ -209,10 +209,6 @@ FontDesc FrameBuffer::getFontDesc(const string& name) const
 FBInitStatus FrameBuffer::createDisplay(const string& title, BufferType type,
                                         Common::Size size, bool honourHiDPI)
 {
-  // always save, maybe only the mode of the window has changed
-  saveCurrentWindowPosition();
-  myBufferType = type;
-
   ++myInitializedCount;
   myScreenTitle = title;
 
@@ -250,44 +246,18 @@ FBInitStatus FrameBuffer::createDisplay(const string& title, BufferType type,
 #endif
 
   // Initialize video mode handler, so it can know what video modes are
-  // appropriate for this framebuffer
+  // appropriate for the requested image size
   myVidModeHandler.setImageSize(size);
 
-  // Initialize video subsystem (make sure we get a valid mode)
+  // Always save, maybe only the mode of the window has changed
+  saveCurrentWindowPosition();
+  myBufferType = type;
+
+  // Initialize video subsystem
   string pre_about = about();
-  myActiveVidMode = buildVideoMode();
+  FBInitStatus status = applyVideoMode();
-  if(size <= myActiveVidMode.screen)
+  if(status != FBInitStatus::Success)
-  {
+    return status;
-    // Changing the video mode can take some time, during which the last
-    // sound played may get 'stuck'
-    // So we mute the sound until the operation completes
-    bool oldMuteState = myOSystem.sound().mute(true);
-    if(activateVideoMode(myScreenTitle, myActiveVidMode))
-    {
-      myImageRect = myActiveVidMode.image;
-      myScreenSize = myActiveVidMode.screen;
-      myScreenRect = Common::Rect(myActiveVidMode.screen);
-
-      // Inform TIA surface about new mode
-      if(myOSystem.eventHandler().state() != EventHandlerState::LAUNCHER &&
-         myOSystem.eventHandler().state() != EventHandlerState::DEBUGGER)
-        myTIASurface->initialize(myOSystem.console(), myActiveVidMode);
-
-      // Did we get the requested fullscreen state?
-      myOSystem.settings().setValue("fullscreen", fullScreen());
-      resetSurfaces();
-      setCursorState();
-
-      myOSystem.sound().mute(oldMuteState);
-    }
-    else
-    {
-      Logger::error("ERROR: Couldn't initialize video subsystem");
-      return FBInitStatus::FailNotSupported;
-    }
-  }
-  else
-    return FBInitStatus::FailTooLarge;
 
 #ifdef GUI_SUPPORT
   // Erase any messages from a previous run
@@ -328,7 +298,7 @@ FBInitStatus FrameBuffer::createDisplay(const string& title, BufferType type,
       Logger::info(post_about);
   }
 
-  return FBInitStatus::Success;
+  return status;
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@@ -661,7 +631,7 @@ void FrameBuffer::drawFrameStats(float framesPerSecond)
   myStatsMsg.surface->drawString(f, ss.str(), xPos, yPos,
       myStatsMsg.w, myStatsMsg.color, TextAlign::Left, 0, true, kBGColor);
 
-  myStatsMsg.surface->setDstPos(myImageRect.x() + 10, myImageRect.y() + 8);
+  myStatsMsg.surface->setDstPos(imageRect().x() + 10, imageRect().y() + 8);
   myStatsMsg.surface->setDstSize(myStatsMsg.w * hidpiScaleFactor(),
                                  myStatsMsg.h * hidpiScaleFactor());
   myStatsMsg.surface->render();
@@ -739,47 +709,47 @@ inline bool FrameBuffer::drawMessage()
       break;
 
     case MessagePosition::TopCenter:
-      myMsg.x = (myImageRect.w() - dst.w()) >> 1;
+      myMsg.x = (imageRect().w() - dst.w()) >> 1;
       myMsg.y = 5;
       break;
 
     case MessagePosition::TopRight:
-      myMsg.x = myImageRect.w() - dst.w() - 5;
+      myMsg.x = imageRect().w() - dst.w() - 5;
       myMsg.y = 5;
       break;
 
     case MessagePosition::MiddleLeft:
       myMsg.x = 5;
-      myMsg.y = (myImageRect.h() - dst.h()) >> 1;
+      myMsg.y = (imageRect().h() - dst.h()) >> 1;
       break;
 
     case MessagePosition::MiddleCenter:
-      myMsg.x = (myImageRect.w() - dst.w()) >> 1;
+      myMsg.x = (imageRect().w() - dst.w()) >> 1;
-      myMsg.y = (myImageRect.h() - dst.h()) >> 1;
+      myMsg.y = (imageRect().h() - dst.h()) >> 1;
       break;
 
     case MessagePosition::MiddleRight:
-      myMsg.x = myImageRect.w() - dst.w() - 5;
+      myMsg.x = imageRect().w() - dst.w() - 5;
-      myMsg.y = (myImageRect.h() - dst.h()) >> 1;
+      myMsg.y = (imageRect().h() - dst.h()) >> 1;
       break;
 
     case MessagePosition::BottomLeft:
       myMsg.x = 5;
-      myMsg.y = myImageRect.h() - dst.h() - 5;
+      myMsg.y = imageRect().h() - dst.h() - 5;
       break;
 
     case MessagePosition::BottomCenter:
-      myMsg.x = (myImageRect.w() - dst.w()) >> 1;
+      myMsg.x = (imageRect().w() - dst.w()) >> 1;
-      myMsg.y = myImageRect.h() - dst.h() - 5;
+      myMsg.y = imageRect().h() - dst.h() - 5;
       break;
 
     case MessagePosition::BottomRight:
-      myMsg.x = myImageRect.w() - dst.w() - 5;
+      myMsg.x = imageRect().w() - dst.w() - 5;
-      myMsg.y = myImageRect.h() - dst.h() - 5;
+      myMsg.y = imageRect().h() - dst.h() - 5;
       break;
   }
 
-  myMsg.surface->setDstPos(myMsg.x + myImageRect.x(), myMsg.y + myImageRect.y());
+  myMsg.surface->setDstPos(myMsg.x + imageRect().x(), myMsg.y + imageRect().y());
   myMsg.surface->fillRect(0, 0, myMsg.w, myMsg.h, kColor);
   myMsg.surface->fillRect(BORDER, BORDER, myMsg.w - BORDER * 2, myMsg.h - BORDER * 2, kBtnColor);
   myMsg.surface->drawString(font(), myMsg.text, HBORDER, VBORDER,
@@ -1002,32 +972,10 @@ void FrameBuffer::setFullscreen(bool enable)
     default:
       return;
   }
-  saveCurrentWindowPosition();
-
-  // Changing the video mode can take some time, during which the last
-  // sound played may get 'stuck'
-  // So we mute the sound until the operation completes
-  bool oldMuteState = myOSystem.sound().mute(true);
 
   myOSystem.settings().setValue("fullscreen", enable);
-  myActiveVidMode = buildVideoMode();
+  saveCurrentWindowPosition();
-  if(activateVideoMode(myScreenTitle, myActiveVidMode))
+  applyVideoMode();
-  {
-    myImageRect = myActiveVidMode.image;
-    myScreenSize = myActiveVidMode.screen;
-    myScreenRect = Common::Rect(myActiveVidMode.screen);
-
-    // Inform TIA surface about new mode
-    if(myOSystem.eventHandler().state() != EventHandlerState::LAUNCHER &&
-       myOSystem.eventHandler().state() != EventHandlerState::DEBUGGER)
-      myTIASurface->initialize(myOSystem.console(), myActiveVidMode);
-
-    // Did we get the requested fullscreen state?
-    myOSystem.settings().setValue("fullscreen", fullScreen());
-    resetSurfaces();
-    setCursorState();
-  }
-  myOSystem.sound().mute(oldMuteState);
 #endif
 }
 
@@ -1120,12 +1068,8 @@ void FrameBuffer::changeOverscan(int direction)
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 void FrameBuffer::switchVideoMode(int direction)
 {
-  EventHandlerState state = myOSystem.eventHandler().state();
-  bool tiaMode = (state != EventHandlerState::DEBUGGER &&
-                  state != EventHandlerState::LAUNCHER);
-
   // Only applicable when in TIA/emulation mode
-  if(!tiaMode)
+  if(!myOSystem.eventHandler().inTIAMode())
     return;
 
   if(!fullScreen())
@@ -1151,44 +1095,18 @@ void FrameBuffer::switchVideoMode(int direction)
   }
 
   saveCurrentWindowPosition();
-
+  if(applyVideoMode() == FBInitStatus::Success)
-  // Changing the video mode can take some time, during which the last
-  // sound played may get 'stuck'
-  // So we mute the sound until the operation completes
-  bool oldMuteState = myOSystem.sound().mute(true);
-
-  myActiveVidMode = buildVideoMode();
-  if(activateVideoMode(myScreenTitle, myActiveVidMode))
   {
-    myImageRect = myActiveVidMode.image;
-    myScreenSize = myActiveVidMode.screen;
-    myScreenRect = Common::Rect(myActiveVidMode.screen);
-
-    // Inform TIA surface about new mode
-    myTIASurface->initialize(myOSystem.console(), myActiveVidMode);
-
-    resetSurfaces();
     if(fullScreen())
       showMessage(myActiveVidMode.description);
     else
       showMessage("Zoom", myActiveVidMode.description, myActiveVidMode.zoom,
                   supportedTIAMinZoom(), myTIAMaxZoom);
-    myOSystem.sound().mute(oldMuteState);
-
-    // Error check: were the settings applied as requested?
-    if(fullScreen())
-      myOSystem.settings().setValue("tia.fs_stretch",
-          myActiveVidMode.stretch == VideoModeHandler::Mode::Stretch::Fill);
-    else
-      myOSystem.settings().setValue("tia.zoom", myActiveVidMode.zoom);
-
-    return;
   }
-  myOSystem.sound().mute(oldMuteState);
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-const VideoModeHandler::Mode& FrameBuffer::buildVideoMode()
+FBInitStatus FrameBuffer::applyVideoMode()
 {
   // Update display size, in case windowed/fullscreen mode has changed
   const Settings& s = myOSystem.settings();
@@ -1200,12 +1118,47 @@ const VideoModeHandler::Mode& FrameBuffer::buildVideoMode()
   else
     myVidModeHandler.setDisplaySize(myAbsDesktopSize);
 
-  // And now build the new mode based on current settings
+  const bool inTIAMode = myOSystem.eventHandler().inTIAMode();
-  const bool tiaMode =
-    myOSystem.eventHandler().state() != EventHandlerState::DEBUGGER &&
-    myOSystem.eventHandler().state() != EventHandlerState::LAUNCHER;
 
-  return myVidModeHandler.buildMode(s, tiaMode);
+  // Build the new mode based on current settings
+  const VideoModeHandler::Mode& mode = myVidModeHandler.buildMode(s, inTIAMode);
+  if(mode.imageR.size() > mode.screenS)
+    return FBInitStatus::FailTooLarge;
+
+  // Changing the video mode can take some time, during which the last
+  // sound played may get 'stuck'
+  // So we mute the sound until the operation completes
+  bool oldMuteState = myOSystem.sound().mute(true);
+  FBInitStatus status = FBInitStatus::FailNotSupported;
+  if(activateVideoMode(myScreenTitle, mode))
+  {
+    myActiveVidMode = mode;
+    status = FBInitStatus::Success;
+
+    // Did we get the requested fullscreen state?
+    myOSystem.settings().setValue("fullscreen", fullScreen());
+
+    // Inform TIA surface about new mode, and update TIA settings
+    if(inTIAMode)
+    {
+      myTIASurface->initialize(myOSystem.console(), myActiveVidMode);
+      if(fullScreen())
+        myOSystem.settings().setValue("tia.fs_stretch",
+          myActiveVidMode.stretch == VideoModeHandler::Mode::Stretch::Fill);
+      else
+        myOSystem.settings().setValue("tia.zoom", myActiveVidMode.zoom);
+    }
+
+    resetSurfaces();
+    setCursorState();
+  }
+  else
+    Logger::error("ERROR: Couldn't initialize video subsystem");
+
+  // Restore sound settings
+  myOSystem.sound().mute(oldMuteState);
+
+  return status;
 }
 
 // - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

src/emucore/FrameBuffer.hxx

@@ -186,15 +186,15 @@ class FrameBuffer
       Note that this will take into account the current scaling (if any)
       as well as image 'centering'.
     */
-    const Common::Rect& imageRect() const { return myImageRect; }
+    const Common::Rect& imageRect() const { return myActiveVidMode.imageR; }
 
     /**
       Returns the current dimensions of the framebuffer window.
       This is the entire area containing the framebuffer image as well as any
       'unusable' area.
     */
-    const Common::Size& screenSize() const { return myScreenSize; }
+    const Common::Size& screenSize() const { return myActiveVidMode.screenS; }
-    const Common::Rect& screenRect() const { return myScreenRect; }
+    const Common::Rect& screenRect() const { return myActiveVidMode.screenR; }
 
     /**
       Returns the current dimensions of the users' desktop.
@@ -507,11 +507,10 @@ class FrameBuffer
 
     /**
       Build an applicable video mode based on the current settings in
-      effect, whether TIA mode is active, etc.
+      effect, whether TIA mode is active, etc.  Then tell the backend
-      Note that this only creates the video mode definition itself;
+      to actually use the new mode.
-      to apply it, we need to call 'activateVideoMode()'.
     */
-    const VideoModeHandler::Mode& buildVideoMode();
+    FBInitStatus applyVideoMode();
 
     /**
       Calculate the maximum level by which the base window can be zoomed and
@@ -558,16 +557,6 @@ class FrameBuffer
     // Used to set intervals between messages while in pause mode
     Int32 myPausedCount{0};
 
-    // Dimensions of the actual image, after zooming, and taking into account
-    // any image 'centering'
-    Common::Rect myImageRect;
-
-    // Dimensions of the main window (not always the same as the image)
-    // Use 'size' version when only wxh are required
-    // Use 'rect' version when x/y, wxh are required
-    Common::Size myScreenSize;
-    Common::Rect myScreenRect;
-
     // Maximum dimensions of the desktop area
     // Note that this takes 'hidpi' mode into account, so in some cases
     // it will be less than the absolute desktop size
@@ -579,7 +568,8 @@ class FrameBuffer
     // Supported renderers
     VariantList myRenderers;
 
-    // The VideoModeHandler class takes responsibility for all video mode functionality
+    // The VideoModeHandler class takes responsibility for all video
+    // mode functionality
     VideoModeHandler myVidModeHandler;
     VideoModeHandler::Mode myActiveVidMode;
 
@@ -634,7 +624,7 @@ class FrameBuffer
     FullPaletteArray myFullPalette;
     // Holds UI palette data (for each variation)
     static UIPaletteArray ourStandardUIPalette, ourClassicUIPalette,
-      ourLightUIPalette, ourDarkUIPalette;
+                          ourLightUIPalette, ourDarkUIPalette;
 
   private:
     // Following constructors and assignment operators not supported

src/emucore/TIASurface.cxx

@@ -95,10 +95,10 @@ void TIASurface::initialize(const Console& console,
 {
   myTIA = &(console.tia());
 
-  myTiaSurface->setDstPos(mode.image.x(), mode.image.y());
+  myTiaSurface->setDstPos(mode.imageR.x(), mode.imageR.y());
-  myTiaSurface->setDstSize(mode.image.w(), mode.image.h());
+  myTiaSurface->setDstSize(mode.imageR.w(), mode.imageR.h());
-  mySLineSurface->setDstPos(mode.image.x(), mode.image.y());
+  mySLineSurface->setDstPos(mode.imageR.x(), mode.imageR.y());
-  mySLineSurface->setDstSize(mode.image.w(), mode.image.h());
+  mySLineSurface->setDstSize(mode.imageR.w(), mode.imageR.h());
 
   myPaletteHandler->setPalette();