Refactor monitor_reconfigure to stop using system("xrandr").

monitor_reconfigure_main.cc

Index: monitor_reconfigure_main.cc
diff --git a/monitor_reconfigure_main.cc b/monitor_reconfigure_main.cc
index bfabae7731e44a35fdf073ac7cae248300a98b1f..a6974b8ff8ce30b34ee3b1d462074952bb6593bd 100644
--- a/monitor_reconfigure_main.cc
+++ b/monitor_reconfigure_main.cc
@@ -20,13 +20,17 @@ using std::vector;
 namespace monitor_reconfig {
 
 static const char kLcdOutputName[] = "LVDS1";
+static const float kScreenDpi = 96.0;
+static const float kInchInMm = 25.4;
 
 MonitorReconfigureMain::MonitorReconfigureMain(Display* display,
+                                               Window window,
                                                XRRScreenResources* screen_info)
     : display_(display),
+      window_(window),
       screen_info_(screen_info),
-      lcd_output_(NULL),
-      external_output_(NULL) {
+      lcd_output_(0),
+      external_output_(0) {
   for (int i = 0; i < screen_info_->nmode; ++i) {
     XRRModeInfo* current_mode = &screen_info_->modes[i];
     mode_map_[current_mode->id] = current_mode;
@@ -36,92 +40,118 @@ MonitorReconfigureMain::MonitorReconfigureMain(Display* display,
 
 void MonitorReconfigureMain::Run() {
   vector<ResolutionSelector::Mode> lcd_modes;
-  SortModesByResolution(*lcd_output_, &lcd_modes);
+  SortModesByResolution(lcd_output_, &lcd_modes);
   DCHECK(!lcd_modes.empty());
 
   vector<ResolutionSelector::Mode> external_modes;
   if (IsExternalMonitorConnected()) {
-    SortModesByResolution(*external_output_, &external_modes);
+    SortModesByResolution(external_output_, &external_modes);
     DCHECK(!external_modes.empty());
   }
 
   ResolutionSelector selector;
-  string lcd_resolution, external_resolution, screen_resolution;
+  ResolutionSelector::Mode lcd_resolution,
+                           external_resolution,

[Daniel Erat, 2011/03/24 23:02:11]

make each of these be declared in a separate statement

+                           screen_resolution;
   CHECK(selector.FindBestResolutions(lcd_modes,
                                      external_modes,
                                      &lcd_resolution,
                                      &external_resolution,
                                      &screen_resolution));
-  CHECK(!lcd_resolution.empty() || !external_resolution.empty());
+  CHECK(!lcd_resolution.name.empty() || !external_resolution.name.empty());
+
+  // Grab the server during mode changes.
+  XGrabServer (display_);

[Daniel Erat, 2011/03/24 23:02:11]

no spaces between function names and opening parentheses (ditto for the Xlib
calls later in this method)

 
   // Disable the LCD if we were told to do so (because we're using a higher
   // resolution that'd be clipped on the LCD).
-  if (lcd_resolution.empty())
-    DisableDevice(lcd_output_->name);
+  // Otherwise enable the LCD if appropriate.
+  if (lcd_resolution.name.empty())
+    DisableDevice(lcd_output_, lcd_output_info_);
+  else
+    SetDeviceResolution(lcd_output_, lcd_output_info_, lcd_resolution);
 
   // If there's no external output connected, disable the device before we try
   // to set the screen resolution; otherwise xrandr will complain if we're
   // trying to set the screen to a smaller size than what the now-unplugged
   // device was using.
-  if (external_resolution.empty())
-    DisableDevice(external_output_->name);
-
-  // Set the fb to try to avoid the driver's "crtc has no fb" message.
-  // Doing this before enabling the display reduces the likelihood of a
-  // visible "snap" when returning to the panel.
+  // Otherwise enable the external device if appropriate.
+  if (external_resolution.name.empty())
+    DisableDevice(external_output_, external_output_info_);
+  else
+    SetDeviceResolution(external_output_, external_output_info_,
+                        external_resolution);
+
+  // Set the fb resolution.
   SetScreenResolution(screen_resolution);
 
-  // Enable the LCD if appropriate.
-  if (!lcd_resolution.empty())
-    SetDeviceResolution(lcd_output_->name, lcd_resolution);
+  if (!lcd_resolution.name.empty())
+
+  if (!external_resolution.name.empty())
 
-  // Enable the external device if appropriate.
-  if (!external_resolution.empty())
-    SetDeviceResolution(external_output_->name, external_resolution);
+  // Now let the server go and sync all changes.
+  XUngrabServer (display_);
+  XSync (display_, False);
 }
 
 void MonitorReconfigureMain::DetermineOutputs() {
   CHECK(screen_info_->noutput > 1) << "Expected at least two outputs";
-  XRROutputInfo* first_output =
-      XRRGetOutputInfo(display_, screen_info_, screen_info_->outputs[0]);
-  XRROutputInfo* second_output =
-      XRRGetOutputInfo(display_, screen_info_, screen_info_->outputs[1]);
 
-  if (strcmp(first_output->name, kLcdOutputName) == 0) {
+  RROutput first_output = screen_info_->outputs[0];
+  RROutput second_output = screen_info_->outputs[1];
+
+  XRROutputInfo* first_output_info =
+      XRRGetOutputInfo(display_, screen_info_, first_output);
+  XRROutputInfo* second_output_info =
+      XRRGetOutputInfo(display_, screen_info_, second_output);
+
+  if (strcmp(first_output_info->name, kLcdOutputName) == 0) {
     lcd_output_ = first_output;
+    lcd_output_info_ = first_output_info;
     external_output_ = second_output;
+    external_output_info_ = second_output_info;
   } else {
     lcd_output_ = second_output;
+    lcd_output_info_ = second_output_info;
     external_output_ = first_output;
+    external_output_info_ = first_output_info;
   }
 
-  LOG(INFO) << "LCD name: " << lcd_output_->name;
-  for (int i = 0; i < lcd_output_->nmode; ++i) {
-    XRRModeInfo* mode = mode_map_[lcd_output_->modes[i]];
-    LOG(INFO) << "  Mode: " << mode->width << "x" << mode->height;
+  LOG(INFO) << "LCD name: " << lcd_output_info_->name << " (xid "
+            << lcd_output_ << ")";
+  for (int i = 0; i < lcd_output_info_->nmode; ++i) {
+    XRRModeInfo* mode = mode_map_[lcd_output_info_->modes[i]];
+    LOG(INFO) << "  Mode: " << mode->width << "x" << mode->height
+              << " (xid " << mode->id << ")";
   }
 
-  LOG(INFO) << "External name: " << external_output_->name;
-  for (int i = 0; i < external_output_->nmode; ++i) {
-    XRRModeInfo* mode = mode_map_[external_output_->modes[i]];
-    LOG(INFO) << "  Mode: " << mode->width << "x" << mode->height;
+  LOG(INFO) << "External name: " << external_output_info_->name
+            << " (xid " << external_output_ << ")";
+  for (int i = 0; i < external_output_info_->nmode; ++i) {
+    XRRModeInfo* mode = mode_map_[external_output_info_->modes[i]];
+    LOG(INFO) << "  Mode: " << mode->width << "x" << mode->height
+              << " (xid " << mode->id << ")";
   }
 }
 
 bool MonitorReconfigureMain::IsExternalMonitorConnected() {
-  return (external_output_->connection == RR_Connected);
+  return (external_output_info_->connection == RR_Connected);
 }
 
 void MonitorReconfigureMain::SortModesByResolution(
-    const XRROutputInfo& output_info,
+    const RROutput output,
     vector<ResolutionSelector::Mode>* modes_out) {
   modes_out->clear();
 
-  for (int i = 0; i < output_info.nmode; ++i) {
-    const XRRModeInfo* mode = mode_map_[output_info.modes[i]];
+  XRROutputInfo* output_info = XRRGetOutputInfo(display_, screen_info_, output);
+  for (int i = 0; i < output_info->nmode; ++i) {
+    const XRRModeInfo* mode = mode_map_[output_info->modes[i]];
     DCHECK(mode);
+    LOG(INFO) << "Adding mode " << mode->width << " " << mode->height
+              << " (xid " << mode->id << ")";
     modes_out->push_back(
-        ResolutionSelector::Mode(mode->width, mode->height, mode->name));
+        ResolutionSelector::Mode(mode->width, mode->height, mode->name,
+                                 mode->id));
   }
 
   sort(modes_out->begin(), modes_out->end(),
@@ -129,25 +159,38 @@ void MonitorReconfigureMain::SortModesByResolution(
 }
 
 bool MonitorReconfigureMain::SetDeviceResolution(
-    const std::string& device_name, const std::string& resolution) {
-  string command = StringPrintf("xrandr --output %s --mode %s",
-                                device_name.c_str(), resolution.c_str());
-  LOG(INFO) << "Running " << command.c_str();
-  return system(command.c_str()) == 0;
+    const RROutput output, const XRROutputInfo* output_info,
+    const ResolutionSelector::Mode resolution) {
+  RROutput o = output;

[Daniel Erat, 2011/03/24 23:02:11]

why do you need to make a copy here?

[marcheu, 2011/03/24 23:17:53]

XRRSetCrtcConfig has a non-const RROutput* parameter. The other option is remove
const from the parameters.

[Daniel Erat, 2011/03/24 23:39:46]

How about const_cast?

+  Status s = XRRSetCrtcConfig (display_, screen_info_, output_info->crtcs[0],
+                               CurrentTime, 0, 0, resolution.id, RR_Rotate_0,
+                               &o, 1);
+  return (s == RRSetConfigSuccess);
 }
 
 bool MonitorReconfigureMain::SetScreenResolution(
-    const std::string& resolution) {
-  string command = StringPrintf("xrandr --fb %s", resolution.c_str());
-  LOG(INFO) << "Running " << command.c_str();
-  return system(command.c_str()) == 0;
+    const ResolutionSelector::Mode resolution) {
+  int screen = DefaultScreen(display_);
+  LOG(INFO) << "Setting resolution " << resolution.name.c_str() << " "
+            << resolution.width << "x" << resolution.height;
+  XRRSetScreenSize (display_, window_,
+                    resolution.width,
+                    resolution.height,
+                    resolution.width * kInchInMm / kScreenDpi,
+                    resolution.height * kInchInMm / kScreenDpi
+                    );

[Daniel Erat, 2011/03/24 23:02:11]

move this to the end of the previous line

+
+  return true;
 }
 
-bool MonitorReconfigureMain::DisableDevice(const std::string& device_name) {
-  string command = StringPrintf("xrandr --output %s --off",
-                                device_name.c_str());
-  LOG(INFO) << "Running " << command.c_str();
-  return system(command.c_str()) == 0;
+bool MonitorReconfigureMain::DisableDevice(const RROutput output,
+                                           const XRROutputInfo* output_info) {
+  if (!output_info->crtc)
+    return true;
+  LOG(INFO) << "Disabling output " << output_info->name;
+  Status s = XRRSetCrtcConfig (display_, screen_info_, output_info->crtc,
+    CurrentTime, 0, 0, None, RR_Rotate_0, NULL, 0);
+  return (s == RRSetConfigSuccess);
 }
 
 }  // end namespace monitor_reconfig
@@ -163,7 +206,8 @@ int main(int argc, char** argv) {
 
   Window window = RootWindow(display, DefaultScreen(display));
   XRRScreenResources* screen_info = XRRGetScreenResources(display, window);
-  monitor_reconfig::MonitorReconfigureMain main_app(display, screen_info);
+  monitor_reconfig::MonitorReconfigureMain main_app(display, window,
+                                                    screen_info);
   main_app.Run();
   return 0;
 }