Re: [PATCH] Renaming non-X x_* identifiers

[Alex Gramiak]

Eli Zaretskii <address@hidden> writes:

>> From: Alex Gramiak <address@hidden>
>> Cc: address@hidden, Alan Third <address@hidden>
>> 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.

Sure. What's the policy is for rebasing on scratch branches? I pushed
the branch as scratch/x_emacs.

> 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?

Honestly, I can't. I think it was one of the ones I did in the
beginning, so I forgot to make it a terminal hook. I changed it to a
hook.

>> @@ -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?

The ones with no checks are in HAVE_WINDOW_SYSTEM and are assumed to
exist, while the ones with checks are outside of HAVE_WINDOW_SYSTEM.
Though it appears that I also checked a few that are in
HAVE_WINDOW_SYSTEM. Would you prefer all of them to be checked, or just
the ones outside of HAVE_WINDOW_SYSTEM?

>> @@ -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.

While it is visible from Lisp, I don't see why anyone would change it
considering that AFAIU it's used as an internal value in frame.c.
frame.c sets it and uses the value of the property to call the
appropriate element in frame_parm_table, which Lisp-code should not rely
on.

Then again, apparently cedet/semantic/util-modes.el accesses this
property, but that could be changed.

>> -    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.

Hmm, frame-inhibit-implied-resize checks the presence of the symbol in
frame-inhibit-implied-resize, so I suppose it probably can't be changed.
Then again, x-set-frame-parameters isn't documented as a valid type in
the docstring of frame-inhibit-implied-resize, so I'm not sure this is a
problem.

>>  /* 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?

In some cases I decided to leave some terminal hooks for a later patch
to implement, just to make it easier to get this one accepted. I can add
a hook for this if you'd like.

>>  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_* ?

My intention is for the HAVE_WINDOW_SYSTEM-only procedures to be gui_*
and the others to be image_*. I decided on this later on, so I may have
a few gui_* procedures that should be image_*.

>> -    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?

This one could as well.

>> @@ -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.

I believe frame_size_history_add only uses the symbols as a
visual/debugging aid, so I don't believe this is, meaningfully, 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.

These are likewise just used by frame_size_history_add.

>> 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?

The implementations are defined in the *menu.c files, but are added as
terminal hooks in the *term.c files.

>> +/* 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.

I wanted to avoid that (see my earlier thread about ns_defined_color),
but if you prefer it I'll change it.