[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] Renaming non-X x_* identifiers (was: Renaming non-X x_* proc
From: |
Eli Zaretskii |
Subject: |
Re: [PATCH] Renaming non-X x_* identifiers (was: Renaming non-X x_* procedures in xdisp.c (and elsewhere)) |
Date: |
Sat, 13 Apr 2019 20:17:44 +0300 |
> 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.
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.
- [PATCH] Renaming non-X x_* identifiers (was: Renaming non-X x_* procedures in xdisp.c (and elsewhere)), Alex Gramiak, 2019/04/13
- Re: [PATCH] Renaming non-X x_* identifiers (was: Renaming non-X x_* procedures in xdisp.c (and elsewhere)),
Eli Zaretskii <=
- Re: [PATCH] Renaming non-X x_* identifiers, Alex Gramiak, 2019/04/13
- Re: [PATCH] Renaming non-X x_* identifiers, Eli Zaretskii, 2019/04/13
- Re: [PATCH] Renaming non-X x_* identifiers, Alex Gramiak, 2019/04/13
- Re: [PATCH] Renaming non-X x_* identifiers, Eli Zaretskii, 2019/04/14
- Re: [PATCH] Renaming non-X x_* identifiers, Alex Gramiak, 2019/04/14
- Re: [PATCH] Renaming non-X x_* identifiers, Eli Zaretskii, 2019/04/14
- Re: [PATCH] Renaming non-X x_* identifiers, Alex Gramiak, 2019/04/14
- Re: [PATCH] Renaming non-X x_* identifiers, Eli Zaretskii, 2019/04/15
- Re: [PATCH] Renaming non-X x_* identifiers, Alex Gramiak, 2019/04/15
- Re: [PATCH] Renaming non-X x_* identifiers, Eli Zaretskii, 2019/04/15