From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Renaming non-X x_* identifiers Date: Sat, 13 Apr 2019 22:00:16 +0300 Message-ID: <83muktk9xb.fsf@gnu.org> 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> <87pnppu4ox.fsf@gmail.com> Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="159718"; mail-complaints-to="usenet@blaine.gmane.org" Cc: alan@idiocy.org, emacs-devel@gnu.org To: Alex Gramiak Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sat Apr 13 21:01:14 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 1hFNtp-000fHA-R3 for ged-emacs-devel@m.gmane.org; Sat, 13 Apr 2019 21:01:14 +0200 Original-Received: from localhost ([127.0.0.1]:54910 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hFNti-0008NF-2n for ged-emacs-devel@m.gmane.org; Sat, 13 Apr 2019 15:01:06 -0400 Original-Received: from eggs.gnu.org ([209.51.188.92]:47544) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hFNtY-0008MV-AT for emacs-devel@gnu.org; Sat, 13 Apr 2019 15:00:57 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:54696) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1hFNtL-0003MU-ER; Sat, 13 Apr 2019 15:00:48 -0400 Original-Received: from [176.228.60.248] (port=1361 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1hFNtH-00027o-SH; Sat, 13 Apr 2019 15:00:41 -0400 In-reply-to: <87pnppu4ox.fsf@gmail.com> (message from Alex Gramiak on Sat, 13 Apr 2019 12:43:10 -0600) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] 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:235408 Archived-At: > From: Alex Gramiak > Cc: emacs-devel@gnu.org, alan@idiocy.org > Date: Sat, 13 Apr 2019 12:43:10 -0600 > > > 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? Not sure I understand what you mean by "policy". Rebasing or not in general is up to you, but maybe you are asking about something more specific. > I pushed the branch as scratch/x_emacs. That's okay, thanks. > > 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? If every window-system is required to provide these hooks, then I think it will be enough to test only those which also have implementations on TTY frames. > >> 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. Anything that gets put into frame-parameters can have some Lisp out there using it. So I think we have 2 alternatives: 1) leave those symbols alone 2) declare them obsolete, but meanwhile put both the new and the old symbols into frame-parameters The above assumes that if a Lisp program does something with one of these parameters, that will have no effect, i.e. that these parameters are one-way communications from the Emacs internals to Lisp, as far as Lisp programs are concerned. If the communications are two-way, then I don't see how we can change these names; do you have any ideas? > >> /* 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. It's okay to do that in a followup, but please do that soon. I don't want to risk leaving an unfinished job in the sources. > > 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_*. I think using image_* for all the functions in image.c would be better. > >> - (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. Are they in frame-parameters? If so, they are visible. > >> 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. I'm not sure I understand the answer. I didn't ask about the implementations, I asked about the prototypes. Since these are hooks, their names are not visible outside the corresponding *term.c file, right? Then why do we need the prototypes of w32_activate_menubar, ns_activate_menubar, etc. in menu.h? > >> +/* 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. Yes, I think avoiding it makes the code less clean than having an unused argument. Thanks.