New-style Disconnect window for Linux Chromoting host.

remoting/host/disconnect_window_linux.cc

Index: remoting/host/disconnect_window_linux.cc
diff --git a/remoting/host/disconnect_window_linux.cc b/remoting/host/disconnect_window_linux.cc
index b7292d61540952537de3e7c2751efdeeae31de8f..4a75cd77c79a53ead31699badfaa16dbb67499d6 100644
--- a/remoting/host/disconnect_window_linux.cc
+++ b/remoting/host/disconnect_window_linux.cc
@@ -5,6 +5,7 @@
 #include "remoting/host/disconnect_window.h"
 
 #include <gtk/gtk.h>
+#include <math.h>
 
 #include "base/compiler_specific.h"
 #include "base/logging.h"
@@ -14,16 +15,6 @@
 #include "remoting/host/ui_strings.h"
 #include "ui/base/gtk/gtk_signal.h"
 
-namespace {
-// The width in pixels at which the message will wrap. Given that the message
-// contains an un-splittable email address, it's unlikely that a fixed width
-// is going to look aesthetically pleasing in all languages.
-// TODO(jamiewalch): Replace this with a layout that only uses a single line,
-// and which is docked at the top or bottom of the host screen, as in our
-// UI mocks.
-const int kMessageWidth = 300;
-}
-
 namespace remoting {
 
 class DisconnectWindowLinux : public DisconnectWindow {
@@ -36,20 +27,33 @@ class DisconnectWindowLinux : public DisconnectWindow {
   virtual void Hide() OVERRIDE;
 
  private:
-  CHROMEGTK_CALLBACK_1(DisconnectWindowLinux, void, OnResponse, int);
+  CHROMEGTK_CALLBACK_1(DisconnectWindowLinux, gboolean, OnDelete, GdkEvent*);
+  CHROMEGTK_CALLBACK_0(DisconnectWindowLinux, void, OnClicked);
+  CHROMEGTK_CALLBACK_1(DisconnectWindowLinux, gboolean, OnConfigure,
+                       GdkEventConfigure*);
+  CHROMEGTK_CALLBACK_1(DisconnectWindowLinux, gboolean, OnButtonPress,
+                       GdkEventButton*);
 
   void CreateWindow(const UiStrings& ui_strings);
 
   ChromotingHost* host_;
   GtkWidget* disconnect_window_;
   GtkWidget* message_;
+  GtkWidget* button_;
+
+  // Used to distinguish resize events from other types of "configure-event"
+  // notifications.
+  int current_width_;
+  int current_height_;
 
   DISALLOW_COPY_AND_ASSIGN(DisconnectWindowLinux);
 };
 
 DisconnectWindowLinux::DisconnectWindowLinux()
     : host_(NULL),
-      disconnect_window_(NULL) {
+      disconnect_window_(NULL),
+      current_width_(0),
+      current_height_(0) {
 }
 
 DisconnectWindowLinux::~DisconnectWindowLinux() {
@@ -58,42 +62,71 @@ DisconnectWindowLinux::~DisconnectWindowLinux() {
 void DisconnectWindowLinux::CreateWindow(const UiStrings& ui_strings) {
   if (disconnect_window_) return;
 
-  disconnect_window_ = gtk_dialog_new_with_buttons(
-      UTF16ToUTF8(ui_strings.product_name).c_str(),
-      NULL,
-      GTK_DIALOG_NO_SEPARATOR,
-      UTF16ToUTF8(ui_strings.disconnect_button_text_plus_shortcut).c_str(),
-      GTK_RESPONSE_OK,
-      NULL);
-
+  disconnect_window_ = gtk_window_new(GTK_WINDOW_TOPLEVEL);
   GtkWindow* window = GTK_WINDOW(disconnect_window_);

[Jamie, 2011/11/30 02:48:56]

Do you ever not want to see disconnect_window_ as a GtkWindow*? Might it be
better to declare the class member as that type and cast before storing?

[Lambros, 2011/12/02 01:41:55]

The Chrome examples I saw elsewhere used GtkWidget* for storing the window, and
it is the declared return type of gtk_window_new(), which does seem unusual :) 
We do need it as a GtkWidget* in a couple of places at least.

+
+  g_signal_connect(disconnect_window_, "delete-event",

[Jamie, 2011/11/30 02:48:56]

...For example, will this still compile if disconnect_window_ is of type
GtkWindow*?

[Lambros, 2011/12/02 01:41:55]

That particular line would still compile (g_signal_connect is a macro), but
others won't, for example gtk_widget_set_app_paintable() and
gtk_widget_show_all().

+                   G_CALLBACK(OnDeleteThunk), this);
+  gtk_window_set_title(window, UTF16ToUTF8(ui_strings.product_name).c_str());

[Jamie, 2011/11/30 02:48:56]

What's the purpose of setting the title?

[Lambros, 2011/12/02 01:41:55]

Might as well keep it.  Theoretically, a user might run a WM that dishonors the
request to hide the title (and other window decorations).  And even if the title
were invisible on the window itself, it might appear elsewhere (taskbar,
window-switcher, a11y tool etc).  There doesn't seem to be any harm keeping it
in.

   gtk_window_set_resizable(window, FALSE);
+
   // Try to keep the window always visible.
   gtk_window_stick(window);
   gtk_window_set_keep_above(window, TRUE);
+
+  // Remove window titlebar.
+  gtk_window_set_decorated(window, FALSE);
+
+  // In case the titlebar is still there, try to remove some of the buttons.
   // Utility windows have no minimize button or taskbar presence.
   gtk_window_set_type_hint(window, GDK_WINDOW_TYPE_HINT_UTILITY);
   gtk_window_set_deletable(window, FALSE);
 
-  g_signal_connect(disconnect_window_, "response",
-                   G_CALLBACK(OnResponseThunk), this);
+  // Allow custom rendering of the background pixmap.
+  gtk_widget_set_app_paintable(disconnect_window_, TRUE);
+
+  // Handle window resizing, to regenerate the background pixmap and window
+  // shape bitmap.  The stored width & height need to be initialized here
+  // in case the window is created a second time (the size of the previous
+  // window would be remembered, preventing the generation of bitmaps for the
+  // new window).
+  current_height_ = current_width_ = 0;

[Jamie, 2011/11/30 02:48:56]

I don't think this is necessary, as we regenerate bitmaps if the size has
changed anyway, and if it hasn't then the old bitmaps can be reused. Also, this
sort of indirect indication that the bitmap needs to be regenerated makes me
nervous--can we reset the bitmap itself instead, or is that owned by the window?

[Lambros, 2011/12/02 01:41:55]

Actually, we were leaking the bitmaps!  I've fixed this to deref the bitmaps
after handing them to the window.  We should still reset stored sizes to 0 (or
at least do something to cause the bitmaps to be regenerated).  Unfortunately,
the problem it fixes can't be reproduced any more - it only happened when
Disconnect Window was shown in Me2Me mode, when you reconnected after
disconnecting.

+  g_signal_connect(disconnect_window_, "configure-event",
+                   G_CALLBACK(OnConfigureThunk), this);
+
+  // Handle mouse events to allow the user to drag the window around.
+  gtk_widget_set_events(disconnect_window_, GDK_BUTTON_PRESS_MASK);
+  g_signal_connect(disconnect_window_, "button-press-event",
+                   G_CALLBACK(OnButtonPressThunk), this);
 
-  GtkWidget* content_area =
-      gtk_dialog_get_content_area(GTK_DIALOG(disconnect_window_));
+  // All magic numbers taken from screen shots provided by UX.
+  // The alignment sets narrow margins at the top and bottom, compared with
+  // left and right.  The left margin is made larger to accommodate the
+  // window movement gripper.

[Jamie, 2011/11/30 02:48:56]

Will this work for bidi languages? If not, then that can be fixed in a separate
CL.

[Lambros, 2011/12/02 01:41:55]

Haven't tried it.  I expect GTK will display the controls (button and
text-label) the other way round (button on the left).  But the gripper will
still be on the left.  Coming to a CL near you...

+  GtkWidget* align = gtk_alignment_new(0, 0, 1, 1);
+  gtk_alignment_set_padding(GTK_ALIGNMENT(align), 8, 8, 24, 12);
+  gtk_container_add(GTK_CONTAINER(window), align);
 
-  GtkWidget* message_row = gtk_hbox_new(FALSE, 0);
-  // TODO(lambroslambrou): Replace the magic number with an appropriate
-  // constant from a header file (such as chrome/browser/ui/gtk/gtk_util.h
-  // but check_deps disallows its #inclusion here).
-  gtk_container_set_border_width(GTK_CONTAINER(message_row), 12);
-  gtk_container_add(GTK_CONTAINER(content_area), message_row);
+  GtkWidget* button_row = gtk_hbox_new(FALSE, 12);
+  gtk_container_add(GTK_CONTAINER(align), button_row);
+
+  button_ = gtk_button_new_with_label(
+      UTF16ToUTF8(ui_strings.disconnect_button_text_plus_shortcut).c_str());
+  gtk_box_pack_end(GTK_BOX(button_row), button_, FALSE, FALSE, 0);
+
+  g_signal_connect(button_, "clicked", G_CALLBACK(OnClickedThunk), this);
 
   message_ = gtk_label_new(NULL);
-  gtk_widget_set_size_request(message_, kMessageWidth, -1);
-  gtk_label_set_line_wrap(GTK_LABEL(message_), true);
-  gtk_container_add(GTK_CONTAINER(message_row), message_);
+  gtk_box_pack_end(GTK_BOX(button_row), message_, FALSE, FALSE, 0);
+
+  // Override any theme setting for the text color, so that the text is
+  // readable against the window's background pixmap.
+  PangoAttrList* attributes = pango_attr_list_new();
+  PangoAttribute* text_color = pango_attr_foreground_new(0, 0, 0);
+  pango_attr_list_insert(attributes, text_color);
+  gtk_label_set_attributes(GTK_LABEL(message_), attributes);
 
-  gtk_widget_show_all(content_area);
+  gtk_widget_show_all(disconnect_window_);
 }
 
 void DisconnectWindowLinux::Show(ChromotingHost* host,
@@ -114,16 +147,132 @@ void DisconnectWindowLinux::Hide() {
   }
 }
 
-void DisconnectWindowLinux::OnResponse(GtkWidget* dialog, int response_id) {
-  // |response_id| is ignored, because there is only one button, and pressing
-  // it should have the same effect as closing the window (if the window Close
-  // button were visible).
+void DisconnectWindowLinux::OnClicked(GtkWidget* button) {
   CHECK(host_);
 
   host_->Shutdown(base::Closure());
   Hide();
 }
 
+gboolean DisconnectWindowLinux::OnDelete(GtkWidget* window, GdkEvent* event) {
+  CHECK(host_);
+
+  host_->Shutdown(base::Closure());
+  Hide();
+
+  return TRUE;
+}
+
+namespace {
+// Helper function for creating a rectangular path with rounded corners, as
+// Cairo doesn't have this facility.  |radius| is the arc-radius of each
+// corner.  The bounding rectangle extends from (0, 0) to (width, height).
+void AddRoundRectPath(cairo_t* cr, int width, int height, int radius) {
+  cairo_new_sub_path(cr);
+  cairo_arc(cr, width - radius, radius, radius, -M_PI_2, 0);
+  cairo_arc(cr, width - radius, height - radius, radius, 0, M_PI_2);
+  cairo_arc(cr, radius, height - radius, radius, M_PI_2, 2 * M_PI_2);
+  cairo_arc(cr, radius, radius, radius, 2 * M_PI_2, 3 * M_PI_2);
+  cairo_close_path(cr);
+}
+
+}  // namespace
+
+gboolean DisconnectWindowLinux::OnConfigure(GtkWidget* widget,
+                                            GdkEventConfigure* event) {
+  // Only generate bitmaps if the size has actually changed.
+  if (event->width == current_width_ && event->height == current_height_)
+    return FALSE;
+
+  current_width_ = event->width;
+  current_height_ = event->height;
+
+  // Create the depth 1 pixmap for the window shape.
+  GdkPixmap* shape_mask = gdk_pixmap_new(NULL, current_width_, current_height_,

[Jamie, 2011/11/30 02:48:56]

I assume that cairo_destroy also destroys this pixmap? It might be worth
checking to see if there's a scoped_ptr equivalent for this, which would save a
couple of calls to cairo_destroy. Not really worth creating one if not though.

[Lambros, 2011/12/02 01:41:55]

I'm pretty sure cairo_destroy() doesn't destroy the pixmap. There is no scoped
wrapper for cairo_destroy() that I can find in Chrome.

+                                         1);
+  cairo_t* cr = gdk_cairo_create(shape_mask);

[Jamie, 2011/11/30 02:48:56]

Can this have a more descriptive name, please?

[Lambros, 2011/12/02 01:41:55]

Done (groan!).

[Jamie, 2011/12/02 02:23:41]

I think |context| would be sufficient if you think it's too long now. I just
didn't know what cr was supposed to stand for, so wasn't sure if this was just
an opaque context or something else.

+
+  // Set the arc radius for the corners.  It is theoretically possible that a
+  // user might be able to resize the window (the Window Manager might not
+  // honor the fixed-size request), so make allowances for extremely thin
+  // sizes just in case.

[Jamie, 2011/11/30 02:48:56]

What happens if we don't? If it just looks crappy then I don't think we care
(it's going to be unusable if resized too small anyway).

[Lambros, 2011/12/02 01:41:55]

Yeah, it just looks very slightly strange at the corners.  Done.

+  int radius = std::min(6, std::min(current_width_ / 2, current_height_ / 2));
+
+  // Initialize the whole bitmap to be transparent.
+  cairo_set_source_rgba(cr, 0, 0, 0, 0);
+  cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
+  cairo_paint(cr);
+
+  // Paint an opaque round rect covering the whole area (leaving the extreme
+  // corners transparent).
+  cairo_set_source_rgba(cr, 1, 1, 1, 1);
+  cairo_set_operator(cr, CAIRO_OPERATOR_SOURCE);
+  AddRoundRectPath(cr, current_width_, current_height_, radius);
+  cairo_fill(cr);
+
+  cairo_destroy(cr);
+  gdk_window_shape_combine_mask(widget->window, shape_mask, 0, 0);
+
+  // Create a full-color pixmap for the window background image.
+  GdkPixmap* background = gdk_pixmap_new(NULL, current_width_, current_height_,
+                                         24);
+  cr = gdk_cairo_create(background);
+
+  // Paint the whole bitmap one color.
+  cairo_set_source_rgb(cr, 233 / 256.0, 233 / 256.0, 233 / 256.0);

[Jamie, 2011/11/30 02:48:56]

Seems an odd way of specifying the colour. I think the Mac version also uses
fractional RGB values, but specifies them as fractions. To mimic the values, the
Windows version multiplies them up, but you seem to be doing the opposite here.
I recommend you just copy the fractional values from the Mac source directly.

[Lambros, 2011/12/02 01:41:55]

Done.

+  cairo_paint(cr);
+
+  // Paint the round-rectangle edge.
+  cairo_set_source_rgb(cr, 32 / 256.0, 176 / 256.0, 29 / 256.0);
+  cairo_set_line_width(cr, 6);
+  AddRoundRectPath(cr, current_width_, current_height_, radius);
+  cairo_stroke(cr);
+
+  // Render the window-gripper.  In order for a straight line to light up
+  // single pixels, Cairo requires the coordinates to have fractional
+  // components of 0.5 (so the "/ 2" is a deliberate integer division).
+  double gripper_top = current_height_ / 2 - 10.5;
+  int gripper_bottom = current_height_ / 2 + 10.5;

[Jamie, 2011/11/30 02:48:56]

I don't understand this. Why is one of these a double and the other an int? And
why do the y-coordinates need to be fractional? I think the comment is intended
to explain this, but I'm still in the dark.

[Lambros, 2011/12/02 01:41:55]

Doh!  They should both be doubles.  Fixed.

+  cairo_set_line_width(cr, 1);
+
+  double x = 12.5;
+  cairo_set_source_rgb(cr, 194 / 256.0, 194 / 256.0, 194 / 256.0);

[Jamie, 2011/11/30 02:48:56]

Seems an odd way of specifying the colour. I think the Mac version also uses
fractional RGB values, but specifies them as fractions. To mimic the values, the
Windows version multiplies them up, but you seem to be doing the opposite here.

[Lambros, 2011/12/02 01:41:55]

Done.

+  cairo_move_to(cr, x, gripper_top);
+  cairo_line_to(cr, x, gripper_bottom);
+  cairo_stroke(cr);
+  x += 3;
+  cairo_move_to(cr, x, gripper_top);
+  cairo_line_to(cr, x, gripper_bottom);
+  cairo_stroke(cr);
+
+  x -= 2;
+  cairo_set_source_rgb(cr, 248 / 256.0, 248 / 256.0, 248 / 256.0);
+  cairo_move_to(cr, x, gripper_top);
+  cairo_line_to(cr, x, gripper_bottom);
+  cairo_stroke(cr);
+  x += 3;
+  cairo_move_to(cr, x, gripper_top);
+  cairo_line_to(cr, x, gripper_bottom);
+  cairo_stroke(cr);
+
+  cairo_destroy(cr);
+
+  gdk_window_set_back_pixmap(widget->window, background, FALSE);
+  gdk_window_invalidate_rect(widget->window, NULL, TRUE);
+
+  return FALSE;
+}
+
+gboolean DisconnectWindowLinux::OnButtonPress(GtkWidget* widget,
+                                              GdkEventButton* event) {
+  gtk_window_begin_move_drag(GTK_WINDOW(disconnect_window_),
+                             event->button,
+                             event->x_root,
+                             event->y_root,
+                             event->time);
+  return FALSE;
+}
+
 DisconnectWindow* DisconnectWindow::Create() {
   return new DisconnectWindowLinux;
 }