From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Merging the pgtk branch Date: Sun, 01 Aug 2021 11:53:16 +0300 Message-ID: <835ywpo8ar.fsf@gnu.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="29715"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Yuuki Harano Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Aug 01 10:54:13 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 1mA7Eb-0007Y1-07 for ged-emacs-devel@m.gmane-mx.org; Sun, 01 Aug 2021 10:54:13 +0200 Original-Received: from localhost ([::1]:51948 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mA7EZ-0004MT-BC for ged-emacs-devel@m.gmane-mx.org; Sun, 01 Aug 2021 04:54:11 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:60072) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mA7Dw-0003ho-6P for emacs-devel@gnu.org; Sun, 01 Aug 2021 04:53:32 -0400 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]:42336) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mA7Du-00044X-9S; Sun, 01 Aug 2021 04:53:30 -0400 Original-Received: from 84.94.185.95.cable.012.net.il ([84.94.185.95]:3529 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mA7Dt-0006VP-Rh; Sun, 01 Aug 2021 04:53:30 -0400 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:271913 Archived-At: We would like to merge the pgtk branch onto master soon, so that it could be included in Emacs 28.1. Below please find some preliminary comments from reviewing the branch's code. I would ask people here who are interested in this feature to please build the branch, both --with-pgtk and without it, and report any problems they see (using "M-x report-emacs-bug", preferably). Here are my preliminary comments from looking at the branch. Full disclosure: I know almost nothing about GTK, so please forgive me any silly questions/remarks I may have due to this ignorance. > -install: all install-arch-indep install-etcdoc install-arch-dep install-$(NTDIR) blessmail install-eln > +install: all install-arch-indep install-etcdoc install-arch-dep install-$(NTDIR) blessmail install-eln install-gsettings-schemas > @true Does this mean gsettings-schemas will be installed by non-PGTK builds as well? If not, where are the conditions to prevent that? > + if test $HAVE_IMAGEMAGICK != yes; then > + IMAGEMAGICK_MODULE="MagickWand-6.Q16HDRI >= 6.3.5 MagickWand-6.Q16HDRI != 6.8.2 MagickWand-6.Q16HDRI < 7 MagickCore-6.Q16HDRI >= 6.9.9 MagickCore-6.Q16HDRI < 7" > + EMACS_CHECK_MODULES([IMAGEMAGICK], [$IMAGEMAGICK_MODULE]) > + fi What is this change about? is it related to PGTK? > +HAVE_CAIRO=no > +if test "${HAVE_X11}" = "yes" -o "$window_system" = pgtk; then > + if test "${with_cairo}" != "no"; then > + CAIRO_REQUIRED=1.12.0 > + CAIRO_MODULE="cairo >= $CAIRO_REQUIRED" > + EMACS_CHECK_MODULES(CAIRO, $CAIRO_MODULE) > + if test $HAVE_CAIRO = yes; then > + AC_DEFINE(USE_CAIRO, 1, [Define to 1 if using cairo.]) > + else > + AC_MSG_ERROR([cairo requested but not found.]) > + fi > + > + CFLAGS="$CFLAGS $CAIRO_CFLAGS" > + LIBS="$LIBS $CAIRO_LIBS" > + AC_SUBST(CAIRO_CFLAGS) > + AC_SUBST(CAIRO_LIBS) > + fi > +fi Does the PGTK build require Cairo? The above seems to imply it does. If it does, I think the --with-pgtk description should say so. > +if test "${window_system}" = "pgtk"; then > + FONT_OBJ="ftfont.o ftcrfont.o" > +fi Does this mean the PGTK build doesn't support HarfBuzz? Or am I misreading the meaning of the above change? > diff --git a/lib-src/emacsclient.c b/lib-src/emacsclient.c > index 8346524..8fa571f 100644 > --- a/lib-src/emacsclient.c > +++ b/lib-src/emacsclient.c > @@ -603,7 +603,12 @@ decode_options (int argc, char **argv) > alt_display = "w32"; > #endif > +#ifdef HAVE_PGTK > + display = egetenv ("WAYLAND_DISPLAY"); > + alt_display = egetenv ("DISPLAY"); > +#else > display = egetenv ("DISPLAY"); > +#endif The WAYLAND_DISPLAY variable should be documented in the Emacs manual, where all the environment variables Emacs uses are documented. > @@ -165,9 +168,9 @@ gui--selection-value-internal > Call `gui-get-selection' with an appropriate DATA-TYPE argument > decided by `x-select-request-type'. The return value is already > decoded. If `gui-get-selection' signals an error, return nil." > - (let ((request-type (if (eq window-system 'x) > + (let ((request-type (if (memq window-system '(x pgtk)) > (or x-select-request-type > - '(UTF8_STRING COMPOUND_TEXT STRING)) > + '(UTF8_STRING COMPOUND_TEXT STRING text/plain\;charset=utf-8)) > 'STRING)) Is it useful to have "text/plain\;charset=utf-8" here when window-system is 'x', not 'pgtk'? > --- a/lisp/server.el > +++ b/lisp/server.el > @@ -900,12 +900,17 @@ server-create-window-system-frame > ) > (cond (w > - (server--create-frame > - nowait proc > - `((display . ,display) > - ,@(if parent-id > - `((parent-id . ,(string-to-number parent-id)))) > - ,@parameters))) > + (condition-case nil > + (server--create-frame > + nowait proc > + `((display . ,display) > + ,@(if parent-id > + `((parent-id . ,(string-to-number parent-id)))) > + ,@parameters)) > + (error > + (server-log "Window system unsupported" proc) > + (server-send-string proc "-window-system-unsupported \n") > + nil))) Why is this change needed? > +(defcustom pgtk-pop-up-frames 'fresh > + "Non-nil means open files upon request from the Workspace in a new frame. > +If t, always do so. Any other non-nil value means open a new frame > +unless the current buffer is a scratch buffer." > + :type '(choice (const :tag "Never" nil) > + (const :tag "Always" t) > + (other :tag "Except for scratch buffer" fresh)) Is this really PGTK-specific? > + :version "23.1" That version tag is definitely incorrect, should be 28.1. > +;; Set to use font panel instead > +(declare-function pgtk-popup-font-panel "pgtkfns.c" (&optional frame)) > +(defalias 'x-select-font 'pgtk-popup-font-panel "Pop up the font panel. > +This function has been overloaded in Nextstep.") > +(defalias 'mouse-set-font 'pgtk-popup-font-panel "Pop up the font panel. > +This function has been overloaded in Nextstep.") Is Nextstep relevant to the PGTK build? > +;; Default fontset. This is mainly here to show how a fontset > +;; can be set up manually. Ordinarily, fontsets are auto-created whenever > +;; a font is chosen by > +(defvar pgtk-standard-fontset-spec > + ;; Only some code supports this so far, so use uglier XLFD version > + ;; "-pgtk-*-*-*-*-*-10-*-*-*-*-*-fontset-standard,latin:Courier,han:Kai" > + (mapconcat 'identity > + '("-*-Monospace-*-*-*-*-10-*-*-*-*-*-fontset-standard" > + "latin:-*-Courier-*-*-*-*-10-*-*-*-*-*-iso10646-1" > + "han:-*-Kai-*-*-*-*-10-*-*-*-*-*-iso10646-1" > + "cyrillic:-*-Trebuchet$MS-*-*-*-*-10-*-*-*-*-*-iso10646-1") > + ",") > + "String of fontset spec of the standard fontset. > +This defines a fontset consisting of the Courier and other fonts. > +See the documentation of `create-fontset-from-fontset-spec' for the format.") This seems to be copied from ns-win.el? Are the font names relevant to PGTK? > +;; Functions for color panel + drag > +(defun pgtk-face-at-pos (pos) > + (let* ((frame (car pos)) > + (frame-pos (cons (cadr pos) (cddr pos))) > + (window (window-at (car frame-pos) (cdr frame-pos) frame)) > + (window-pos (coordinates-in-window-p frame-pos window)) > + (buffer (window-buffer window)) > + (edges (window-edges window))) > + (cond > + ((not window-pos) > + nil) > + ((eq window-pos 'mode-line) > + 'mode-line) > + ((eq window-pos 'vertical-line) > + 'default) > + ((consp window-pos) > + (with-current-buffer buffer > + (let ((p (car (compute-motion (window-start window) > + (cons (nth 0 edges) (nth 1 edges)) > + (window-end window) > + frame-pos > + (- (window-width window) 1) > + nil > + window)))) > + (cond > + ((eq p (window-point window)) > + 'cursor) > + ((and mark-active (< (region-beginning) p) (< p (region-end))) > + 'region) > + (t > + (let ((faces (get-char-property p 'face window))) > + (if (consp faces) (car faces) faces))))))) > + (t > + nil)))) Is this function needed? It uses compute-motion, which cannot be a good idea, so I wonder what is this needed for. > + ;; Don't let Emacs suspend under PGTK. > + (add-hook 'suspend-hook 'pgtk-suspend-error) What's this about? If PGTK doesn't want to be suspended, why do this via a hook? > +(defun pgtk-preedit-text (e) > + (interactive "e") Please add a meaningful doc string for this command. > --- a/src/.gdbinit > +++ b/src/.gdbinit > @@ -41,6 +41,9 @@ handle SIGUSR2 noprint pass > # debugging. > handle SIGALRM ignore > +# On selection send failed. > +handle SIGPIPE nostop noprint This cannot be unconditional, please condition it on PGTK. > -#ifdef USE_GTK > +#if defined(USE_GTK) I don't understand this and many similar changes that replaced "#ifdef" with "#if defined" and "#ifndef" with "#if !defined", without adding more conditions. Is this a left-over from some debugging? If so, please don't make such redundant changes. > +#ifdef HAVE_PGTK > + mark_pgtkterm(); > +#endif Our style conventions are to leave one space between the function's name and the left parenthesis, like this: mark_pgtkterm (); Please follow this style here and elsewhere in the PGTK code. > --- a/src/atimer.c > +++ b/src/atimer.c > @@ -309,11 +309,13 @@ set_alarm (void) > ispec.it_value = atimers->expiration; > ispec.it_interval.tv_sec = ispec.it_interval.tv_nsec = 0; > # ifdef HAVE_TIMERFD > - if (timerfd_settime (timerfd, TFD_TIMER_ABSTIME, &ispec, 0) == 0) > - { > - add_timer_wait_descriptor (timerfd); > - return; > - } > + if (timerfd >= 0) { > + if (timerfd_settime (timerfd, TFD_TIMER_ABSTIME, &ispec, 0) == 0) > + { > + add_timer_wait_descriptor (timerfd); > + return; > + } > + } Why was this change needed? Also, please use our style conventions for braces: they should be on the next line, as in the original code above. > # ifdef HAVE_TIMERFD > - timerfd_settime (timerfd, TFD_TIMER_ABSTIME, &ispec, 0); > + if (timerfd >= 0) > + timerfd_settime (timerfd, TFD_TIMER_ABSTIME, &ispec, 0); > # endif Same question here: why was this change needed? > +# elif defined HAVE_PGTK > + /* pgtk emacs does not want timerfd. */ > + return true; And this. > -#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 > +#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. > --- 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? > -#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? > - `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. > @@ -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? > @@ -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? > @@ -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. > +#ifndef PGTK_TRACE > +#define PGTK_TRACE(fmt, ...) ((void) 0) > +#endif Do we still need PGTK_TRACE (and all its calls)? > +#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? > - 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. > +#ifdef PGTK_DEBUG Do we need this PGTK_DEBUG stuff and its callers? > +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). > +#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? > +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? > @@ -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? Thanks again for working on this important feature.