From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Alex Gramiak Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Renaming non-X x_* identifiers Date: Sat, 13 Apr 2019 12:43:10 -0600 Message-ID: <87pnppu4ox.fsf@gmail.com> References: <87wokp4okn.fsf@gmail.com> <83ef6xpo6b.fsf@gnu.org> <0f4be9a6-6e09-f55d-9f58-2a15aef264cd@cs.ucla.edu> <837ecpplw8.fsf@gnu.org> <871s2w510a.fsf@gmail.com> <922F9B91-2E9E-45F6-BB96-66CAE5E9FB81@gnu.org> <87k1goqpnn.fsf@gmail.com> <83imw8nspc.fsf@gnu.org> <87ftrcqg5j.fsf@gmail.com> <83bm20nm62.fsf@gnu.org> <87d0men4jx.fsf@gmail.com> <83o95sisk7.fsf@gnu.org> <87mulcnui4.fsf@gmail.com> <83bm1si7lf.fsf@gnu.org> <87ef6ont03.fsf@gmail.com> <83a7hci44l.fsf@gnu.org> <87a7hcndtc.fsf@gmail.com> <831s2nhza8.fsf@gnu.org> <87d0lpvq6n.fsf_-_@gmail.com> <83r2a5keo7.fsf@gnu.org> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="85174"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) Cc: alan@idiocy.org, emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Apr 13 20:43:58 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:256) (Exim 4.89) (envelope-from ) id 1hFNd7-000M0v-D1 for ged-emacs-devel@m.gmane.org; Sat, 13 Apr 2019 20:43:57 +0200 Original-Received: from localhost ([127.0.0.1]:54767 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hFNd6-0004Lp-FJ for ged-emacs-devel@m.gmane.org; Sat, 13 Apr 2019 14:43:56 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:45318) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hFNcV-0004LZ-D4 for emacs-devel@gnu.org; Sat, 13 Apr 2019 14:43:20 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1hFNcS-0001aF-JH for emacs-devel@gnu.org; Sat, 13 Apr 2019 14:43:18 -0400 Original-Received: from mail-pg1-x542.google.com ([2607:f8b0:4864:20::542]:34841) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1hFNcR-0001ZD-JA; Sat, 13 Apr 2019 14:43:15 -0400 Original-Received: by mail-pg1-x542.google.com with SMTP id g8so6712396pgf.2; Sat, 13 Apr 2019 11:43:15 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=9eCMymyrDowdRGfqvG4I6A7gch+ji+V4nSZ8Jz1r9nY=; b=k6Iraj7D0AG4iNC2ufJEtAGqNdwgFXChNSfTktiy2ASZy4mrz017Gs9d0Gw48CKjlu CrWS4d8s1YtslHkjvlSIkZa8U3oQWjJQcACiO8BvydiZnnYjqsAzKy83uPehHGL5Ycln 5ZiSrhGU5ra5yuhG1TPQdRPFybNEfJ+z8LpmMb2vBphlVJ3nSTCPI22IwZkBAit4X7Gy /cLwWL1XZkfsQg6oQ9o58Vp/Bi+fZclbIIWfjLXsz0q0rsPlURsVeJE234im3AHmukOY ReD4aocPrW5qCWtWUs4ndLWuXasn45HPDnmi3JnBCPxT2jCtbGxI9jHHhLJcCuOwRny6 f+Fg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=9eCMymyrDowdRGfqvG4I6A7gch+ji+V4nSZ8Jz1r9nY=; b=ArqSwXe9eKc7EIJJd43j6xFfW2MCo7ocxbQF7mcaMntgee5utZhIOjpfJ+vi2IV+wU 2ga/81/yxGuXxCqcIW0RpCcn5Yeoo29EtGatm/D3HWefOnExpK4Xl0Od/LZzyU6v9tzd UOXvMu0wfWZ83yTb3CLkq0W6W9ZQzFgspuj/tihPDyKF/NQTnV7MxHWB77yOwBWr3xtU v8+7l26UdfO2Ye/oDoUE7qy/zOdWa9Ib7EQ/CencdgpnEyLscnA16mbPyNYbOJgvEXwJ 09N7oJby7Xko8yhZyvjTso3g5tYszDSJ2LF0iE/qTDuuOjyQJF3IJXp+NsJHOVDReJFy cG1Q== X-Gm-Message-State: APjAAAVO+HbWaP9BnCS8582tcsGd7si7NV1TJWZnAEeSblsKZX9p/K2v oiC+bWVdyddxm1x/ajgGVUvgXEEf X-Google-Smtp-Source: APXvYqyZz3BLeVMTbl4mimoZJDLIgkuS+CU0IsfOMyJpxOsfTRKjsczlOBSJvrWXv1OKMMNX4XtbQA== X-Received: by 2002:a62:6e05:: with SMTP id j5mr63062427pfc.5.1555180994137; Sat, 13 Apr 2019 11:43:14 -0700 (PDT) Original-Received: from lylat ([2604:3d09:e37f:1500:1a72:4878:e793:7302]) by smtp.gmail.com with ESMTPSA id f1sm58119250pgl.35.2019.04.13.11.43.12 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 13 Apr 2019 11:43:13 -0700 (PDT) In-Reply-To: <83r2a5keo7.fsf@gnu.org> (Eli Zaretskii's message of "Sat, 13 Apr 2019 20:17:44 +0300") X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4864:20::542 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:235407 Archived-At: Eli Zaretskii writes: >> From: Alex Gramiak >> Cc: emacs-devel@gnu.org, Alan Third >> 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.