From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Yuuki Harano Newsgroups: gmane.emacs.devel Subject: Re: Merging the pgtk branch Date: Wed, 11 Aug 2021 00:15:15 +0900 (JST) Organization: Ingage Inc. Message-ID: <20210811.001515.1312870178782255892.masm@luna.pink.masm11.me> References: <835ywpo8ar.fsf@gnu.org> Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="14577"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: eliz@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Tue Aug 10 17:16:42 2021 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mDTUf-0003Xo-Ok for ged-emacs-devel@m.gmane-mx.org; Tue, 10 Aug 2021 17:16:42 +0200 Original-Received: from localhost ([::1]:51106 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mDTUe-0007Ev-MU for ged-emacs-devel@m.gmane-mx.org; Tue, 10 Aug 2021 11:16:40 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:39948) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mDTTb-0006Pj-UK for emacs-devel@gnu.org; Tue, 10 Aug 2021 11:15:36 -0400 Original-Received: from shiro.masm11.me ([150.95.182.25]:57636) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mDTTW-0007Yz-7G; Tue, 10 Aug 2021 11:15:35 -0400 Original-Received: from luna.pink.masm11.me (KD106180012180.au-net.ne.jp [106.180.12.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by shiro.masm11.me (Postfix) with ESMTPSA id B3909C0136; Wed, 11 Aug 2021 00:15:20 +0900 (JST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=masm11.me; s=202002; t=1628608521; bh=1Tj6yNh4rlxGmkYZhmd/9OGeH7ko6c/5hmuqoqkOgtY=; h=Date:To:Cc:Subject:From:In-Reply-To:References; b=gRas6qb2D/erf7wfUVWbPa0xwTUAe1Y2tYvQxAYHlGe6dqK3ELUIqCLupENTKDctt Fhq1CUEFmn9cECVy6UuE8nNlzM2ViEvU684C7Od1Qf/DfkmkgKGZF0R6scxLHJXdUN 60tYU6lH/MSkfaMGS5iANOj2oSGGjTVA9bX8OgWE= In-Reply-To: <835ywpo8ar.fsf@gnu.org> X-Mailer: Mew version 6.8 on Emacs 28.0 Received-SPF: pass client-ip=150.95.182.25; envelope-from=masm@masm11.me; helo=shiro.masm11.me X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 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-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:272285 Archived-At: On Sun, 01 Aug 2021 11:53:16 +0300, Eli Zaretskii wrote: >> -#ifdef USE_GTK >> +#if defined(USE_GTK) >> +#ifndef HAVE_PGTK > > This could be done in a single conditional: > > #if defined USE_GTK && !defined HAVE_PGTK Thanks. >> +#define EMACS_TYPE_FIXED (emacs_fixed_get_type ()) >> +#define EMACS_IS_FIXED(obj) (G_TYPE_CHECK_INSTANCE_TYPE ((obj), EMACS_TYPE_FIXED)) > > Why is this unconditional? > >> +extern GType emacs_fixed_get_type (void); > > And this. Since emacsgtkfixed.[ch] are extending a GTK's class, there should be those lines. But OK, they should be enclosed in #ifdef HAVE_PGTK. >> --- a/src/font.c >> +++ b/src/font.c >> @@ -5584,7 +5584,11 @@ syms_of_font (void) >> syms_of_xftfont (); >> #endif /* HAVE_XFT */ >> #endif /* not USE_CAIRO */ >> -#endif /* HAVE_X_WINDOWS */ >> +#else /* not HAVE_X_WINDOWS */ >> +#ifdef USE_CAIRO >> + syms_of_ftcrfont (); >> +#endif >> +#endif /* not HAVE_X_WINDOWS */ > > Why was this needed? how did the Cairo build do this initialization > until now? ---- #ifdef HAVE_WINDOW_SYSTEM #ifdef HAVE_FREETYPE syms_of_ftfont (); #ifdef HAVE_X_WINDOWS syms_of_xfont (); #ifdef USE_CAIRO syms_of_ftcrfont (); /* <------------ There is this call since before */ #else #ifdef HAVE_XFT syms_of_xftfont (); #endif /* HAVE_XFT */ #endif /* not USE_CAIRO */ #else /* not HAVE_X_WINDOWS */ #ifdef USE_CAIRO syms_of_ftcrfont (); /* <------------- I added this call. */ #endif #endif /* not HAVE_X_WINDOWS */ ... ---- >> -#if defined (HAVE_XFT) || defined (HAVE_FREETYPE) || defined (HAVE_NS) >> +#if defined (HAVE_XFT) || defined (HAVE_FREETYPE) || defined (HAVE_NS) || defined(HAVE_PGTK) >> extern Lisp_Object font_build_object (int, Lisp_Object, Lisp_Object, double); >> #endif > > Doesn't the PGTK build define HAVE_XFT and HAVE_FREETYPE? PGTK build defines HAVE_FREETYPE. I'll revert the change. >> - `pc' for a direct-write MS-DOS frame. >> + `pc' for a direct-write MS-DOS frame, >> + `pgtk' for an Emacs frame running entirely in GTK. > > Since you call this "pure GTK", let's say so here as well. OK. >> @@ -4775,10 +4779,17 @@ gui_set_border_width (struct frame *f, Lisp_Object arg, Lisp_Object oldval) >> if (border_width == f->border_width) >> return; > >> +#ifndef HAVE_PGTK >> if (FRAME_NATIVE_WINDOW (f) != 0) >> error ("Cannot change the border width of a frame"); >> +#endif > >> f->border_width = border_width; >> + >> +#ifdef HAVE_PGTK >> + if (FRAME_TERMINAL (f)->frame_rehighlight_hook) >> + (*FRAME_TERMINAL (f)->frame_rehighlight_hook) (f); >> +#endif > > Why is the above done only for PGTK? I'm sorry. I forgot the reason. >> @@ -6367,7 +6386,12 @@ focus (where a frame immediately loses focus when it's left by the mouse >> to set the size of a frame in pixels, to maximize frames or to make them >> fullscreen. To resize your initial frame pixelwise, set this option to >> a non-nil value in your init file. */); >> +#ifndef HAVE_PGTK >> frame_resize_pixelwise = 0; >> +#else >> + /* https://gitlab.gnome.org/GNOME/mutter/-/issues/396 */ >> + frame_resize_pixelwise = true; >> +#endif > > Why the PGTK-specific setting here? To work around the weird behavior when resizing with top-left corner. It is GNOME mutter's bug, which seems to be already fixed. The workaround may be able to be reverted. >> @@ -132,7 +136,9 @@ ftcrfont_open (struct frame *f, Lisp_Object entity, int pixel_size) >> filename = XCAR (val); >> size = XFIXNUM (AREF (entity, FONT_SIZE_INDEX)); >> if (size == 0) >> + { >> size = pixel_size; >> + } > > These braces are redundant here. Indeed. >> +#ifndef PGTK_TRACE >> +#define PGTK_TRACE(fmt, ...) ((void) 0) >> +#endif > > Do we still need PGTK_TRACE (and all its calls)? Maybe, no need. >> +#ifndef HAVE_PGTK >> if (FRAME_X_DISPLAY (f) != DEFAULT_GDK_DISPLAY ()) >> { >> GdkDisplay *gdpy = gdk_x11_lookup_xdisplay (FRAME_X_DISPLAY (f)); >> @@ -137,6 +150,17 @@ xg_set_screen (GtkWidget *w, struct frame *f) >> else >> gtk_window_set_screen (GTK_WINDOW (w), gscreen); >> } >> +#else >> + if (FRAME_X_DISPLAY(f) != DEFAULT_GDK_DISPLAY ()) > > So FRAME_X_P returns zero for PGTK, but FRAME_X_DISPLAY is still > relevant for it? Isn't that confusing? ---- pgtkterm.h:#define FRAME_X_DISPLAY(f) (FRAME_DISPLAY_INFO(f)->gdpy) ---- It's GTK's display. >> - f->output_data.x->hint_flags = 0; >> + f->output_data.xp->hint_flags = 0; > > Why did you need this change (and others like it)? The "x" part here > has an important mnemonic value. ---- #include "xterm.h" #define xp x typedef struct x_output xp_output; #else #define xp pgtk typedef struct pgtk_output xp_output; #endif ---- When X-GTK build, xp is x. When PGTK build, xp is pgtk. Without xp, all of them need to be conditional. e.g. ---- #ifndef HAVE_PGTK f->output_data.x->hint_flags = 0; #else f->output_data.pgtk->hint_flags = 0; #endif ---- >> +#ifdef PGTK_DEBUG > > Do we need this PGTK_DEBUG stuff and its callers? Maybe, no need. >> +static struct redisplay_interface pgtk_redisplay_interface = { >> + pgtk_frame_parm_handlers, >> + gui_produce_glyphs, >> + gui_write_glyphs, >> + gui_insert_glyphs, >> + gui_clear_end_of_line, >> + pgtk_scroll_run, >> + pgtk_after_update_window_line, >> + NULL, // gui_update_window_begin, >> + NULL, // gui_update_window_end, > > Please use C-style comments, not C++-style comments (here and > elsewhere). OK. >> +#define XFillRectangle(d, win, gc, x, y, w, h) \ >> + ( cairo_rectangle (cr, x, y, w, h), cairo_fill (cr) ) > > I wonder why you need this XFillRectangle macro in code that is pure > PGTK? To avoid enbugging, I wanted to use XFillRectangle calls as is. But there are the callers only here. I should replace them with cairo. >> +static int draw_glyphs_debug(const char *file, int lineno, >> + struct window *w, int x, struct glyph_row *row, >> + enum glyph_row_area area, ptrdiff_t start, ptrdiff_t end, >> + enum draw_glyphs_face hl, int overlaps) >> +{ >> + return draw_glyphs(w, x, row, area, start, end, hl, overlaps); >> +} >> +#define draw_glyphs(w, x, r, a, s, e, h, o) \ >> + draw_glyphs_debug(__FILE__, __LINE__, w, x, r, a, s, e, h, o) >> + > > The above looks like some left-over from debugging? Do we still need > it? No need! I'll revert the change. >> @@ -32748,7 +32763,7 @@ mouse_face_from_buffer_pos (Lisp_Object window, >> hlinfo->mouse_face_face_id >> = face_at_buffer_position (w, mouse_charpos, &ignore, >> mouse_charpos + 1, >> - !hlinfo->mouse_face_hidden, -1, 0); >> + !hlinfo->mouse_face_hidden, -1, 0); > > This looks like whitespace change? I'll revert it. -- Yuuki Harano