Re: [PATCH] Renaming non-X x_* identifiers (was: Renaming non-X x_* procedures in xdisp.c (and elsewhere))

[Eli Zaretskii <eliz@gnu.org>]

> From: Alex Gramiak <agrambot@gmail.com>
> Cc: emacs-devel@gnu.org, Alan Third <alan@idiocy.org>
> Date: Sat, 13 Apr 2019 10:13:36 -0600
> 
> Okay, I believe I'm essentially done now, at least with the C-side. I
> can't test with either w32 and NS, so I hope you and Alan can test the
> attached patches with those backends.

I think we should soon push these to a scratch branch, so that people
could try that and provide feedback.  It's hard to work with such
large changes otherwise.

I have a few comments/questions:

> +#ifdef HAVE_WINDOW_SYSTEM
> +Lisp_Object
> +gui_get_focus_frame (struct frame *frame)

I don't think I understand why this function is implemented
differently from others, i.e. why it isn't a frame hook.  Doing so
would remove system-dependent #ifdef's from frame.c, at the very
least.  Can you explain why you did it this way?

> @@ -2771,7 +2786,7 @@ doesn't support multiple overlapping frames, this function does nothing.  */)
>    struct frame *f = decode_live_frame (frame);
>  
>    if (FRAME_TERMINAL (f)->frame_raise_lower_hook)
> -    (*FRAME_TERMINAL (f)->frame_raise_lower_hook) (f, 0);
> +    (*FRAME_TERMINAL (f)->frame_raise_lower_hook) (f, false);
>  
>    return Qnil;
>  }
> @@ -2840,7 +2855,9 @@ If there is no window system support, this function does nothing.  */)
>       (Lisp_Object frame, Lisp_Object noactivate)
>  {
>  #ifdef HAVE_WINDOW_SYSTEM
> -  x_focus_frame (decode_window_system_frame (frame), !NILP (noactivate));
> +  struct frame *f = decode_window_system_frame (frame);
> +  if (f)
> +    FRAME_TERMINAL (f)->focus_frame_hook (f, !NILP (noactivate));
>  #endif

Some frame hooks are called after first making sure they are non-NULL,
others skip the test.  Is there a reason for this inconsistency?

> @@ -4035,7 +4052,7 @@ x_set_frame_parameters (struct frame *f, Lisp_Object alist)
>  
>  	  store_frame_param (f, prop, val);
>  
> -	  param_index = Fget (prop, Qx_frame_parameter);
> +	  param_index = Fget (prop, Qframe_parameter_pos);

The x-frame-parameter property is visible from Lisp, no?  You are here
replacing it with a different symbol, which is a backward-incompatible
change.

> -    adjust_frame_size (f, width, height, 1, 0, Qx_set_frame_parameters);
> +    adjust_frame_size (f, width, height, 1, 0, Qgui_set_frame_parameters);

Likewise here.

>  /* Store F's background color into *BGCOLOR.  */
>  static void
> -x_query_frame_background_color (struct frame *f, XColor *bgcolor)
> +gui_query_frame_background_color (struct frame *f, XColor *bgcolor)
>  {
> -#ifndef HAVE_NS
> -  bgcolor->pixel = FRAME_BACKGROUND_PIXEL (f);
> -  x_query_color (f, bgcolor);
> +#ifdef HAVE_NS
> +  ns_query_color (FRAME_BACKGROUND_COLOR (f), bgcolor, true);
>  #else
> -  ns_query_color (FRAME_BACKGROUND_COLOR (f), bgcolor, 1);
> +  bgcolor->pixel = FRAME_BACKGROUND_PIXEL (f);
> +# ifdef HAVE_NTGUI
> +  w32_query_colors (f, bgcolor, 1);
> +# else
> +  x_query_colors (f, bgcolor, 1);
> +# endif /* HAVE_NTGUI */
>  #endif

Why didn't you convert this to a terminal hook?

>  static void
> -x_set_image_size (struct frame *f, struct image *img)
> +gui_set_image_size (struct frame *f, struct image *img)

Any reason why some x_* functions in image.c were renamed image_*,
while others gui_* ?

> -	x_query_colors (f, row, img->width);
> -
> +        {
> +# ifdef HAVE_NTGUI
> +          w32_query_colors (f, row, img->width);
> +# else
> +          x_query_colors (f, row, img->width);
> +# endif
> +        }

Can this be a terminal hook?

> @@ -4187,7 +4194,7 @@ gui_set_frame_parameters (struct frame *f, Lisp_Object alist)
>        Lisp_Object old_value = get_frame_param (f, Qfullscreen);
>  
>        frame_size_history_add
> -	(f, Qx_set_fullscreen, 0, 0, list2 (old_value, fullscreen));
> +	(f, Qgui_set_fullscreen, 0, 0, list2 (old_value, fullscreen));

This is also visible from Lisp, right?  So renaming the symbol would
be an incompatible change.

> -  DEFSYM (Qx_set_window_size_1, "x-set-window-size-1");
> -  DEFSYM (Qx_set_window_size_2, "x-set-window-size-2");
> -  DEFSYM (Qx_set_window_size_3, "x-set-window-size-3");
> +  DEFSYM (Qgui_set_window_size_1, "gui-set-window-size-1");
> +  DEFSYM (Qgui_set_window_size_2, "gui-set-window-size-2");
> +  DEFSYM (Qgui_set_window_size_3, "gui-set-window-size-3");
>    DEFSYM (Qxg_change_toolbar_position, "xg-change-toolbar-position");
>    DEFSYM (Qx_net_wm_state, "x-net-wm-state");
>    DEFSYM (Qx_handle_net_wm_state, "x-handle-net-wm-state");
> @@ -5872,8 +5878,8 @@ syms_of_frame (void)
>    DEFSYM (Qchange_frame_size, "change-frame-size");
>    DEFSYM (Qxg_frame_set_char_size, "xg-frame-set-char-size");
>    DEFSYM (Qset_window_configuration, "set-window-configuration");
> -  DEFSYM (Qx_create_frame_1, "x-create-frame-1");
> -  DEFSYM (Qx_create_frame_2, "x-create-frame-2");
> +  DEFSYM (Qgui_create_frame_1, "gui-create-frame-1");
> +  DEFSYM (Qgui_create_frame_2, "gui-create-frame-2");
>    DEFSYM (Qterminal_frame, "terminal-frame");

Likewise with these symbols.

> diff --git a/src/menu.h b/src/menu.h
> index 0321c27454..4412948224 100644
> --- a/src/menu.h
> +++ b/src/menu.h
> @@ -47,14 +47,17 @@ extern widget_value *digest_single_submenu (int, int, bool);
>  #if defined (HAVE_X_WINDOWS) || defined (MSDOS)
>  extern Lisp_Object x_menu_show (struct frame *, int, int, int,
>  				Lisp_Object, const char **);
> +extern void x_activate_menubar (struct frame *);
>  #endif
>  #ifdef HAVE_NTGUI
>  extern Lisp_Object w32_menu_show (struct frame *, int, int, int,
>  				  Lisp_Object, const char **);
> +extern void w32_activate_menubar (struct frame *);
>  #endif
>  #ifdef HAVE_NS
>  extern Lisp_Object ns_menu_show (struct frame *, int, int, int,
>  				 Lisp_Object, const char **);
> +extern void ns_activate_menubar (struct frame *);

Since you introduced activate_menubar_hook, why do we need to declare
prototypes for its implementation on menu.h, which is a
system-independent header?

> +/* Wrapper for defined_color_hook to support the extra argument in
> +   ns_defined_color. */

If the extra parameter is the only problem, we could add it to all the
terminal types, and just ignore it where it is not needed.  Then we
won't need a wrapper.

Thanks again for working on this.