Merging the pgtk branch

[Eli Zaretskii <eliz@gnu.org>]

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.