Re: [RFC] Using a display_info union instead of a typedef Display_Info

[Alex Gramiak <agrambot@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2398 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex Gramiak <agrambot@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>, Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Fri, 12 Apr 2019 10:50:09 -0600
>> 
>> The jist of the attached patch is that generalizes x_get_string_resource
>> into a terminal hook, and in the process changes several procedures to
>> use union display_info instead of the typedef Display_Info, which is
>> display system-dependent. One uses DISPLAY_INFO (dpyinfo) to access the
>> common structure of the union, and there are macros
>> U_DISPLAY_INFO_<suffix> to convert from a specific display_info to the
>> union. There is also a procedure gui_frame_display_info, that grabs a
>> window system display_info from a frame.
>> 
>> I've also included a new procedure decode_display_info, which takes the
>> role of the display system-dependent check_x_display_info. I've left out
>> the removal of the check_x_display_info since I'm not sure if this patch
>> will be accepted. I've also left out NS support until a later stage.
>> 
>> WDYT?
>
> Looks to me like replacing one set of window-system specific code with
> another: still the same #ifdef's and the same window-system specific
> code fragments.  Maybe I'm missing something, but it doesn't look to
> me like a change for the better.

There will always have to be window-system specific code somewhere. The
difference with this patch is that the changed parts of the code will be
able to be used by multiple different window systems. That is to say, I
believe this is a necessary step towards Stefan's goal of using multiple
window systems simultaneously.

Note how before there was the implicit conditional compilation regarding
the definition of check_x_display_info and x_get_arg that allowed the
procedure to be used by _only_ 1 window system, but decode_display_info
and gui_display_get_arg would be able to handle _all_ graphical window
systems that Emacs is built with.

This currently doesn't mean any functional change as that number can
currently only be 1, but it would be nice for that to change. Again, I
believe this is (part of) the way to do so.

To show how it's used, I've included a diff that uses
decode_display_info to simplify all 3 check_x_display_info
implementations into a single conditional. (I could remove that
conditional with another terminal hook, but I'm not sure that's worth
it).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: decode_display_info usage --]
[-- Type: text/x-patch, Size: 11501 bytes --]

diff --git a/src/dispextern.h b/src/dispextern.h
index 79a97f6d6d..fb556c906d 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -3589,7 +3589,6 @@ enum resource_types
   RES_TYPE_BOOLEAN_NUMBER
 };
 
-extern Display_Info *check_x_display_info (Lisp_Object);
 extern Lisp_Object gui_display_get_arg (union display_info, Lisp_Object,
                                         Lisp_Object, const char *,
                                         const char *, enum resource_types);
diff --git a/src/nsfns.m b/src/nsfns.m
index 5968b38e28..b5285ce828 100644
--- a/src/nsfns.m
+++ b/src/nsfns.m
@@ -72,40 +72,17 @@ Updated by Christian Limpach (chris@nice.ch)
    nil stands for the selected frame--or, if that is not a Nextstep frame,
    the first Nextstep display on the list.  */
 
-static struct ns_display_info *
+struct ns_display_info *
 check_ns_display_info (Lisp_Object object)
 {
-  struct ns_display_info *dpyinfo = NULL;
-
-  if (NILP (object))
-    {
-      struct frame *sf = XFRAME (selected_frame);
-
-      if (FRAME_NS_P (sf) && FRAME_LIVE_P (sf))
-	dpyinfo = FRAME_DISPLAY_INFO (sf);
-      else if (x_display_list != 0)
-	dpyinfo = x_display_list;
-      else
-        error ("Nextstep windows are not in use or not initialized");
-    }
-  else if (TERMINALP (object))
-    {
-      struct terminal *t = decode_live_terminal (object);
-
-      if (t->type != output_ns)
-        error ("Terminal %d is not a Nextstep display", t->id);
-
-      dpyinfo = t->display_info.ns;
-    }
-  else if (STRINGP (object))
-    dpyinfo = ns_display_info_for_name (object);
+  if (STRINGP (object))
+    return ns_display_info_for_name (object);
   else
     {
-      struct frame *f = decode_window_system_frame (object);
-      dpyinfo = FRAME_DISPLAY_INFO (f);
-    }
+      union display_info dpyinfo_u = decode_display_info (object, output_ns);
 
-  return dpyinfo;
+      return dpyinfo_u.ns;
+    }
 }
 
 
@@ -2221,14 +2198,6 @@ Frames are listed from topmost (first) to bottommost (last).  */)
 
    ========================================================================== */
 
-/* called from frame.c */
-struct ns_display_info *
-check_x_display_info (Lisp_Object frame)
-{
-  return check_ns_display_info (frame);
-}
-
-
 void
 x_set_scroll_bar_default_width (struct frame *f)
 {
diff --git a/src/w32fns.c b/src/w32fns.c
index 4873e7866c..c6da1cef73 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -288,44 +288,26 @@ typedef BOOL (WINAPI *IsDebuggerPresent_Proc) (void);
 /* stdin, from w32console.c */
 extern HANDLE keyboard_handle;
 
+
+static struct w32_display_info *w32_display_info_for_name (Lisp_Object);
+
 /* Let the user specify a display with a frame.
    nil stands for the selected frame--or, if that is not a w32 frame,
    the first display on the list.  */
 
-struct w32_display_info *
-check_x_display_info (Lisp_Object object)
+static struct w32_display_info *
+check_w32_display_info (Lisp_Object object)
 {
-  if (NILP (object))
-    {
-      struct frame *sf = XFRAME (selected_frame);
-
-      if (FRAME_W32_P (sf) && FRAME_LIVE_P (sf))
-	return FRAME_DISPLAY_INFO (sf);
-      else
-	return &one_w32_display_info;
-    }
-  else if (TERMINALP (object))
-    {
-      struct terminal *t = decode_live_terminal (object);
-
-      if (t->type != output_w32)
-	error ("Terminal %d is not a W32 display", t->id);
-
-      return t->display_info.w32;
-    }
-  else if (STRINGP (object))
-    return x_display_info_for_name (object);
+  if (STRINGP (object))
+    return w32_display_info_for_name (object);
   else
     {
-      struct frame *f;
+      union display_info dpyinfo_u = decode_display_info (object, output_w32);
 
-      CHECK_LIVE_FRAME (object);
-      f = XFRAME (object);
-      if (! FRAME_W32_P (f))
-	error ("Non-W32 frame used");
-      return FRAME_DISPLAY_INFO (f);
+      return dpyinfo_u.w32;
     }
 }
+
 \f
 /* Return the Emacs frame-object corresponding to an w32 window.
    It could be the frame's main window or an icon window.  */
@@ -5724,7 +5706,7 @@ DEFUN ("x-create-frame", Fx_create_frame, Sx_create_frame,
                                    parameters, Qdisplay, 0, 0, RES_TYPE_STRING);
   if (EQ (display, Qunbound))
     display = Qnil;
-  dpyinfo = check_x_display_info (display);
+  dpyinfo = check_w32_display_info (display);
   kb = dpyinfo->terminal->kboard;
 
   if (!dpyinfo->terminal->name)
@@ -6159,7 +6141,7 @@ DEFUN ("xw-display-color-p", Fxw_display_color_p, Sxw_display_color_p, 0, 1, 0,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
 
   if ((dpyinfo->n_planes * dpyinfo->n_cbits) <= 2)
     return Qnil;
@@ -6172,7 +6154,7 @@ DEFUN ("x-display-grayscale-p", Fx_display_grayscale_p,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
 
   if ((dpyinfo->n_planes * dpyinfo->n_cbits) <= 1)
     return Qnil;
@@ -6185,7 +6167,7 @@ DEFUN ("x-display-pixel-width", Fx_display_pixel_width,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
 
   return make_fixnum (x_display_pixel_width (dpyinfo));
 }
@@ -6195,7 +6177,7 @@ DEFUN ("x-display-pixel-height", Fx_display_pixel_height,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
 
   return make_fixnum (x_display_pixel_height (dpyinfo));
 }
@@ -6205,7 +6187,7 @@ DEFUN ("x-display-planes", Fx_display_planes, Sx_display_planes,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
 
   return make_fixnum (dpyinfo->n_planes * dpyinfo->n_cbits);
 }
@@ -6215,7 +6197,7 @@ DEFUN ("x-display-color-cells", Fx_display_color_cells, Sx_display_color_cells,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
   int cap;
 
   /* Don't use NCOLORS: it returns incorrect results under remote
@@ -6262,7 +6244,7 @@ DEFUN ("x-display-mm-height", Fx_display_mm_height,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
   HDC hdc;
   double mm_per_pixel;
 
@@ -6278,7 +6260,7 @@ DEFUN ("x-display-mm-width", Fx_display_mm_width, Sx_display_mm_width, 0, 1, 0,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
   HDC hdc;
   double mm_per_pixel;
 
@@ -6303,7 +6285,7 @@ DEFUN ("x-display-visual-class", Fx_display_visual_class,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
   Lisp_Object result = Qnil;
 
   if (dpyinfo->has_palette)
@@ -6505,7 +6487,7 @@ If omitted or nil, that stands for the selected frame's display.
 Internal use only, use `display-monitor-attributes-list' instead.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
   Lisp_Object attributes_list;
 
   block_input ();
@@ -6558,8 +6540,8 @@ x_screen_planes (register struct frame *f)
 /* Return the display structure for the display named NAME.
    Open a new connection if necessary.  */
 
-struct w32_display_info *
-x_display_info_for_name (Lisp_Object name)
+static struct w32_display_info *
+w32_display_info_for_name (Lisp_Object name)
 {
   struct w32_display_info *dpyinfo;
 
@@ -6670,7 +6652,7 @@ DEFUN ("x-close-connection", Fx_close_connection,
        doc: /* SKIP: real doc in xfns.c.  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
 
   if (dpyinfo->reference_count > 0)
     error ("Display still has frames on it");
@@ -8979,7 +8961,7 @@ return the child frames of that frame in Z (stacking) order.
 Frames are listed from topmost (first) to bottommost (last).  */)
   (Lisp_Object display)
 {
-  struct w32_display_info *dpyinfo = check_x_display_info (display);
+  struct w32_display_info *dpyinfo = check_w32_display_info (display);
   HWND window;
 
   block_input ();
diff --git a/src/w32term.h b/src/w32term.h
index 6cc0cedc14..e4ebb620a0 100644
--- a/src/w32term.h
+++ b/src/w32term.h
@@ -239,8 +239,6 @@ extern int menubar_in_use;
 
 extern struct frame *x_window_to_frame (struct w32_display_info *, HWND);
 
-struct w32_display_info *x_display_info_for_name (Lisp_Object);
-
 /* also defined in xterm.h XXX: factor out to common header */
 
 extern struct w32_display_info *w32_term_init (Lisp_Object,
diff --git a/src/xfns.c b/src/xfns.c
index 5d92ab9086..7e5e5985eb 100644
--- a/src/xfns.c
+++ b/src/xfns.c
@@ -158,37 +158,15 @@ static void set_up_x_back_buffer (struct frame *f);
 struct x_display_info *
 check_x_display_info (Lisp_Object object)
 {
-  struct x_display_info *dpyinfo = NULL;
-
-  if (NILP (object))
-    {
-      struct frame *sf = XFRAME (selected_frame);
-
-      if (FRAME_X_P (sf) && FRAME_LIVE_P (sf))
-	dpyinfo = FRAME_DISPLAY_INFO (sf);
-      else if (x_display_list != 0)
-	dpyinfo = x_display_list;
-      else
-	error ("X windows are not in use or not initialized");
-    }
-  else if (TERMINALP (object))
-    {
-      struct terminal *t = decode_live_terminal (object);
-
-      if (t->type != output_x_window)
-        error ("Terminal %d is not an X display", t->id);
-
-      dpyinfo = t->display_info.x;
-    }
-  else if (STRINGP (object))
-    dpyinfo = x_display_info_for_name (object);
+  if (STRINGP (object))
+    return x_display_info_for_name (object);
   else
     {
-      struct frame *f = decode_window_system_frame (object);
-      dpyinfo = FRAME_DISPLAY_INFO (f);
-    }
+      union display_info dpyinfo_u = decode_display_info (object,
+                                                          output_x_window);
 
-  return dpyinfo;
+      return dpyinfo_u.x;
+    }
 }
 
 /* Return the screen positions and offsets of frame F.
diff --git a/src/xterm.h b/src/xterm.h
index da95926eda..6548b0ef8c 100644
--- a/src/xterm.h
+++ b/src/xterm.h
@@ -497,6 +497,7 @@ extern bool use_xim;
 extern struct x_display_info *x_display_list;
 
 extern struct x_display_info *x_display_info_for_display (Display *);
+extern struct x_display_info *check_x_display_info (Lisp_Object);
 extern struct frame *x_top_window_to_frame (struct x_display_info *, int);
 extern struct x_display_info *x_term_init (Lisp_Object, char *, char *);
 extern bool x_display_ok (const char *);