Merging the pgtk branch

Merging the pgtk branch

[Eli Zaretskii]

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.

Re: Merging the pgtk branch

[Yuuki Harano]

On Sun, 01 Aug 2021 11:53:16 +0300,
	Eli Zaretskii <eliz@gnu.org> wrote:
>> -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?

Yes, it is installed if gsettings is enabled, even if non-PGTK builds.


>> +    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?

When emacs did not support imagemagick 7 and I had imagemagick both 6 and 7,
I wanted to use imagemagick 6, ...I think.

I think the code can be removed.


>> +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.

Yes, PGTK build requires cairo.
I'll add to the description.


>> +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?

PGTK build supports harfbuzz.

That code is followed by this code:
----
if test "${HAVE_HARFBUZZ}" = "yes" ; then
  FONT_OBJ="$FONT_OBJ hbfont.o"
fi
----


>> 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.

OK, I'll add WAYLAND_DISPLAY to the manual.


>> @@ -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'?

I think no.  But the combination is not harmful.


>> --- 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?

emacsclient tries WAYLAND_DISPLAY, and DISPLAY if the first try is failed.
The code catches the display connection failure, and returns the failure
to emacsclient.


>> +(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?

The code is ported from ns-win.el, but it does not work.
I'll remove it.


>> +  :version "23.1"
> 
> That version tag is definitely incorrect, should be 28.1.

I'll remove the tag with the function definition.


>> +;; 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?

Those are just leftovers.
I'll remove them.


>> +;; 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?

I don't know what fonts people have..
Kai and Trebuchet$MS may be able to be removed from the list.


>> +;; 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.

The code is from NS code, NS's one is not used, and I don't know what it is.
I'll remove it.


>> +  ;; 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?

If you try to suspend PGTK emacs via emacs running on a terminal (-nw),
the try will be rejected.


>> +(defun pgtk-preedit-text (e)
>> +  (interactive "e")
> 
> Please add a meaningful doc string for this command.

OK, I'll add it.


>> --- 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.

OK, I see.


>> -#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.

OK, I'll revert such 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.

I have changed coding style from mine to yours.
The above example should be leftover...
I'll check whole the code again.


>> --- 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.

Without those changes, something doesn't work correctly,
but I forgot what it is.
I'll try later without the changes.


Thanks for the many comments.
I'll answer the latter half questions later.
-- 
Yuuki Harano

Re: Merging the pgtk branch

[Stefan Monnier]

>>> @@ -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'?
>
> I think no.  But the combination is not harmful.

What is the intended difference between `UTF8_STRING`
and `text/plain\;charset=utf-8`?

>>> +  ;; 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?
> If you try to suspend PGTK emacs via emacs running on a terminal (-nw),
> the try will be rejected.

Indeed, `grep suspend-hook` shows that all GUIs have this.
We should consolidate this.


        Stefan

Re: Merging the pgtk branch

[Yuuki Harano]

On Sun, 01 Aug 2021 12:57:15 -0400,
	Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>>>> @@ -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'?
>>
>> I think no.  But the combination is not harmful.
> 
> What is the intended difference between `UTF8_STRING`
> and `text/plain\;charset=utf-8`?

I think there is nothing.
Wayland applications may want to use mime types as
wider type support.

X applications support `UTF8_STRING`.
Wayland applications normally support both of `UTF8_STRING` and `text/plain;charset=utf-8`.
But XWayland seems to support only `text/plain;charset=utf-8`.

Applications on both side of copy/paste negotiate the data type,
and non-acceptable types are just ignored.
X applications don't know `text/plain;charset=utf-8`, and it
is ignored.

So I added it.

-- 
Yuuki Harano

Re: Merging the pgtk branch

[Eli Zaretskii]

> Date: Mon, 02 Aug 2021 01:38:59 +0900 (JST)
> Cc: emacs-devel@gnu.org
> From: Yuuki Harano <masm+emacs@masm11.me>
> 
> 
> On Sun, 01 Aug 2021 11:53:16 +0300,
> 	Eli Zaretskii <eliz@gnu.org> wrote:
> >> -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?
> 
> Yes, it is installed if gsettings is enabled, even if non-PGTK builds.

Is that useful without the PGTK build?  What is that used for?

> >> --- 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?
> 
> emacsclient tries WAYLAND_DISPLAY, and DISPLAY if the first try is failed.
> The code catches the display connection failure, and returns the failure
> to emacsclient.

Hmm... what about window-systems that have neither WAYLAND_DISPLAY nor
DISPLAY? will this always fail?  I feel that I'm missing something
here: why wasn't this needed before?

> I'll answer the latter half questions later.

TIA.

Re: Merging the pgtk branch

[Yuuki Harano]

On Mon, 02 Aug 2021 14:49:37 +0300,
	Eli Zaretskii <eliz@gnu.org> wrote:
>> >> -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?
>> 
>> Yes, it is installed if gsettings is enabled, even if non-PGTK builds.
> 
> Is that useful without the PGTK build?  What is that used for?

No, it is not useful for non-PGTK build.

X build supports X defaults.  But GTK doesn't support it.
Instead, I implemented this hook with gsettings:
  terminal->get_string_resource_hook = pgtk_get_string_resource;

gsettings supports type. Type of each key is written in the schema file
etc/org.gnu.emacs.defaults.gschema.xml.
(I implemented this feature instead of X defaults, so types of all the key
are string.)

However, I'm not sure whether people want Xdefaults-like feature.
Emacs' frame parameters are enough...


>> >> --- 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?
>> 
>> emacsclient tries WAYLAND_DISPLAY, and DISPLAY if the first try is failed.
>> The code catches the display connection failure, and returns the failure
>> to emacsclient.
> 
> Hmm... what about window-systems that have neither WAYLAND_DISPLAY nor
> DISPLAY? will this always fail?  I feel that I'm missing something
> here: why wasn't this needed before?

Sorry, I mistook.

GTK supports GDK_BACKEND environment variable.

If GDK_BACKEND=wayland, GTK tries only wayland connection.
If GDK_BACKEND=x11, GTK tries only X connection.
If GDK_BACKEND=wayland,x11, GTK tries wayland connection, and X
connection if the first try failed.

If GDK_BACKEND=x11 and DISPLAY is not set, then emacs fails to start.
----
luna:objs % unset DISPLAY
luna:objs % GDK_BACKEND=x11 ./src/emacs      

(emacs:322982): Gtk-WARNING **: 00:31:26.986: cannot open display: 
luna:objs % 
----

If GDK_BACKEND=x11 and DISPLAY=:0, then emacs starts.
However, `emacsclient -c` first tries with WAYLAND_DISPLAY and
it fails, because GDK_BACKEND=x11 and emacs itself does not open
wayland connection.

The error is:
----
-error Cannot&_connect&_to&_display&_server&_wayland&-1
----
(In case of WAYLAND_DISPLAY=wayland-1.)

And emacsclient does not retry.

I wanted to work around this error.
I send -window-system-unsupported by that change.
In this case, emacsclient retry with alt_display, which is
DISPLAY by another change.

> what about window-systems that have neither WAYLAND_DISPLAY nor
> DISPLAY? will this always fail?

emacsclient.c:
----
#if defined (NS_IMPL_COCOA)
      alt_display = "ns";
#elif defined (HAVE_NTGUI)
      alt_display = "w32";
#endif
----
  if (!display)
    {
      display = alt_display;
      alt_display = NULL;
    }
----

On ns and w32, display is set.

To make sure whether that change works on ns and w32,
it is needed to test on such machines.
I didn't test on such machines.

-- 
Yuuki Harano

Re: Merging the pgtk branch

[Stefan Monnier]

>>> >> +           (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)))
[...]
> The error is:
> ----
> -error Cannot&_connect&_to&_display&_server&_wayland&-1
> ----
> (In case of WAYLAND_DISPLAY=wayland-1.)

Can we then change the `condition-case` so it doesn't catch all `error`s
but only the "Cannot connect to display server" one?


        Stefan

Re: Merging the pgtk branch

[Yuuki Harano]

On Tue, 03 Aug 2021 09:37:06 -0400,
	Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>>>> >> +           (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)))
> [...]
>> The error is:
>> ----
>> -error Cannot&_connect&_to&_display&_server&_wayland&-1
>> ----
>> (In case of WAYLAND_DISPLAY=wayland-1.)
> 
> Can we then change the `condition-case` so it doesn't catch all `error`s
> but only the "Cannot connect to display server" one?

Maybe, yes.

-- 
Yuuki Harano

Re: Merging the pgtk branch

[Yuuki Harano]

Hi,
I had a sick so I didn't work on this for a while.

On Mon, 02 Aug 2021 01:38:59 +0900 (JST),
	Yuuki Harano <masm+emacs@masm11.me> wrote:
>>> +    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?
> 
> When emacs did not support imagemagick 7 and I had imagemagick both 6 and 7,
> I wanted to use imagemagick 6, ...I think.
> 
> I think the code can be removed.

Removed.

>>> +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.
> 
> Yes, PGTK build requires cairo.
> I'll add to the description.

Done.

>>> 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.
> 
> OK, I'll add WAYLAND_DISPLAY to the manual.

Where should I describe WAYLAND_DISPLAY?
"C.4.1 General Variables" in info/emacs.info lists many variables but it does
not contain DISPLAY.  Should I add WAYLAND_DISPLAY here?

WAYLAND_DISPLAY is generic for wayland, not specific to pgtk emacs.

>>> +(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?
> 
> The code is ported from ns-win.el, but it does not work.
> I'll remove it.

Done.

>>> +  :version "23.1"
>> 
>> That version tag is definitely incorrect, should be 28.1.
> 
> I'll remove the tag with the function definition.

Done.

>>> +;; 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?
> 
> Those are just leftovers.
> I'll remove them.

Done.

>>> +;; 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?
> 
> I don't know what fonts people have..
> Kai and Trebuchet$MS may be able to be removed from the list.

Removed.

>>> +;; 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.
> 
> The code is from NS code, NS's one is not used, and I don't know what it is.
> I'll remove it.

Done.

>>> +(defun pgtk-preedit-text (e)
>>> +  (interactive "e")
>> 
>> Please add a meaningful doc string for this command.
> 
> OK, I'll add it.

Done.

>>> --- 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.
> 
> OK, I see.

It is in `if defined_HAVE_PGTK` now.

>>> -#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.
> 
> OK, I'll revert such changes.

Done.

>>> +#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.
> 
> I have changed coding style from mine to yours.
> The above example should be leftover...
> I'll check whole the code again.

Done.

>>> --- 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.
> 
> Without those changes, something doesn't work correctly,
> but I forgot what it is.
> I'll try later without the changes.

I tried without those changes, and no problem.
I reverted them.

-- 
Yuuki Harano

Re: Merging the pgtk branch

[Eli Zaretskii]

> Date: Sun, 14 Nov 2021 21:20:10 +0900 (JST)
> Cc: emacs-devel@gnu.org
> From: Yuuki Harano <masm+emacs@masm11.me>
> 
> >> The WAYLAND_DISPLAY variable should be documented in the Emacs manual,
> >> where all the environment variables Emacs uses are documented.
> > 
> > OK, I'll add WAYLAND_DISPLAY to the manual.
> 
> Where should I describe WAYLAND_DISPLAY?
> "C.4.1 General Variables" in info/emacs.info lists many variables but it does
> not contain DISPLAY.  Should I add WAYLAND_DISPLAY here?
> 
> WAYLAND_DISPLAY is generic for wayland, not specific to pgtk emacs.

I think in "Misc Variables".

Thanks.

Re: Merging the pgtk branch

[Yuuki Harano]

On Sun, 14 Nov 2021 15:27:50 +0200,
	Eli Zaretskii <eliz@gnu.org> wrote:
>> >> The WAYLAND_DISPLAY variable should be documented in the Emacs manual,
>> >> where all the environment variables Emacs uses are documented.
>> > 
>> > OK, I'll add WAYLAND_DISPLAY to the manual.
>> 
>> Where should I describe WAYLAND_DISPLAY?
>> "C.4.1 General Variables" in info/emacs.info lists many variables but it does
>> not contain DISPLAY.  Should I add WAYLAND_DISPLAY here?
>> 
>> WAYLAND_DISPLAY is generic for wayland, not specific to pgtk emacs.
> 
> I think in "Misc Variables".

Thanks.  Added.

-- 
Yuuki Harano

Font size (was: Merging the pgtk branch)

[Kévin Le Gouguec]

Hello,

AFAICT the branch builds and runs fine --with-pgtk --with-xwidgets
--with-cairo on Debian 10 (XFCE).

One hiccup: unless I'm mistaken, this build does not consult
~/.Xresources anymore.  That led me to discover --with-gconf and
font-use-system-font (I did not use these options previously); I am now
a bit confused about how font sizes work.

On master, with font-use-system-font set to nil, here is what C-u C-x =
says for various values of emacs.font in ~/.Xresources:

.Xresources¹    C-u C-x =²
9               12
10              13
10.5            14
11              15

¹ "XX" in "emacs.font: DejaVu Sans Mono-XX"
² "XX" in "ftcrhb:-PfEd-DejaVu Sans Mono-normal-normal-normal-*-XX-*-*-*-m-0-iso10646-1"

On master --with-gconf and feature/pgtk, with font-use-system-font set
to t, here is what C-u C-x = says for various GSettings:

GSettings³      C-u C-x =
9               12
10              13
10.5            ERROR (see below)
11              15

³ "XX" in "gsettings org.gnome.desktop.interface monospace-font-name 'DejaVu Sans Mono XX'"

As it happens, "10.5" (reported as "14" inside Emacs) is the size I find
the most comfortable on my screen; unfortunately if I run:

$ gsettings set org.gnome.desktop.interface monospace-font-name 'DejaVu Sans Mono 10.5'

Emacs emits a warning:

> set-face-attribute: Font not available: #<font-spec nil nil DejaVu\
> Sans\ Mono\ 10\.5 nil nil nil nil nil nil nil nil nil ((:name
> . "DejaVu Sans Mono 10.5") (:user-spec . "DejaVu Sans Mono 10.5"))>

Is this expected?  Otherwise, should I report an issue?  (AFAICT it is
not specific to the pgtk branch)

Re: Font size (was: Merging the pgtk branch)

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Cc: Yuuki Harano <masm+emacs@masm11.me>, Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 04 Aug 2021 14:53:04 +0200
> 
> One hiccup: unless I'm mistaken, this build does not consult
> ~/.Xresources anymore.

That's by design, AFAIU.  Yuuki even said that not so long ago.

Re: Font size

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> One hiccup: unless I'm mistaken, this build does not consult
>> ~/.Xresources anymore.
>
> That's by design, AFAIU.  Yuuki even said that not so long ago.

Right.  To recap my other questions:

1. Why do Xresources and GSettings sizes differ from what C-u C-x =
   reports?

2. Why doesn't "10.5" work with GSettings, despite it working with
   Xresources?

3. Is there a bug to report there?

Re: Font size

[Yuri Khan]

On Wed, 4 Aug 2021 at 20:59, Kévin Le Gouguec <kevin.legouguec@gmail.com> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> One hiccup: unless I'm mistaken, this build does not consult
> >> ~/.Xresources anymore.
> >
> > That's by design, AFAIU.  Yuuki even said that not so long ago.
>
> Right.  To recap my other questions:
>
> 1. Why do Xresources and GSettings sizes differ from what C-u C-x =
>    reports?

Likely because the former are specified in points (1 pt = 1/72 in) and
the latter in pixels (1 px = 1/96 in by default).

Re: Font size

[Eli Zaretskii]

> From: Yuri Khan <yuri.v.khan@gmail.com>
> Date: Wed, 4 Aug 2021 22:46:25 +0700
> Cc: Eli Zaretskii <eliz@gnu.org>, masm+emacs@masm11.me, 
> 	Emacs developers <emacs-devel@gnu.org>
> 
> > 1. Why do Xresources and GSettings sizes differ from what C-u C-x =
> >    reports?
> 
> Likely because the former are specified in points (1 pt = 1/72 in) and
> the latter in pixels (1 px = 1/96 in by default).

is this tru also for GSettings font sizes in the X+GTK build of Emacs?
Because then we'd need to document that in the "Fonts" section of the
Emacs manual, where all the different methods of specifying a font are
described.

Re: Font size

[Yuri Khan]

On Wed, 4 Aug 2021 at 23:11, Eli Zaretskii <eliz@gnu.org> wrote:

> > > 1. Why do Xresources and GSettings sizes differ from what C-u C-x =
> > >    reports?
> >
> > Likely because the former are specified in points (1 pt = 1/72 in) and
> > the latter in pixels (1 px = 1/96 in by default).
>
> is this tru also for GSettings font sizes in the X+GTK build of Emacs?
> Because then we'd need to document that in the "Fonts" section of the
> Emacs manual, where all the different methods of specifying a font are
> described.

The org.gnome.desktop.interface GSettings schema does not say
explicitly which units the size is specified in, and whether
fractional values are allowed.

    <key name="font-name" type="s">
      <default>'Cantarell 11'</default>
      <summary>Default font</summary>
      <description>
        Name of the default font used by gtk+.
      </description>
    </key>
    <key name="document-font-name" type="s">
      <default>'Cantarell 11'</default>
      <summary>Document font</summary>
      <description>
        Name of the default font used for reading documents.
      </description>
    </key>
    <key name="monospace-font-name" type="s">
      <default>'Source Code Pro 10'</default>
      <summary>Monospace font</summary>
      <description>
        Name of a monospaced (fixed-width) font for use in locations like
        terminals.
      </description>
    </key>

Re: Font size

[Eli Zaretskii]

> From: Yuri Khan <yuri.v.khan@gmail.com>
> Date: Thu, 5 Aug 2021 00:09:47 +0700
> Cc: Kévin Le Gouguec <kevin.legouguec@gmail.com>, 
> 	masm+emacs@masm11.me, Emacs developers <emacs-devel@gnu.org>
> 
> On Wed, 4 Aug 2021 at 23:11, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > > > 1. Why do Xresources and GSettings sizes differ from what C-u C-x =
> > > >    reports?
> > >
> > > Likely because the former are specified in points (1 pt = 1/72 in) and
> > > the latter in pixels (1 px = 1/96 in by default).
> >
> > is this tru also for GSettings font sizes in the X+GTK build of Emacs?
> > Because then we'd need to document that in the "Fonts" section of the
> > Emacs manual, where all the different methods of specifying a font are
> > described.
> 
> The org.gnome.desktop.interface GSettings schema does not say
> explicitly which units the size is specified in, and whether
> fractional values are allowed.

My question was whether the same units are likely to be used in the
X+GTK build as they are in the PGTK build.  IOW, that your guess of
the units being 1/96 inch is true or false for both of them.

Re: Font size

[Yuri Khan]

On Thu, 5 Aug 2021 at 01:24, Eli Zaretskii <eliz@gnu.org> wrote:

> My question was whether the same units are likely to be used in the
> X+GTK build as they are in the PGTK build.  IOW, that your guess of
> the units being 1/96 inch is true or false for both of them.

I’m not related to GNOME or GTK+ development, but I would be much
surprised if font sizes were specified in anything other than points
(which are 1/72 in, not 1/96 in); and especially if GTK behaved
differently in that regard between X and Wayland and/or depended on
other factors other than poorly configured screen DPI.

Re: Font size

[Robert Pluim]

>>>>> On Thu, 5 Aug 2021 01:33:16 +0700, Yuri Khan <yuri.v.khan@gmail.com> said:

    Yuri> On Thu, 5 Aug 2021 at 01:24, Eli Zaretskii <eliz@gnu.org> wrote:
    >> My question was whether the same units are likely to be used in the
    >> X+GTK build as they are in the PGTK build.  IOW, that your guess of
    >> the units being 1/96 inch is true or false for both of them.

    Yuri> I’m not related to GNOME or GTK+ development, but I would be much
    Yuri> surprised if font sizes were specified in anything other than points
    Yuri> (which are 1/72 in, not 1/96 in); and especially if GTK behaved
    Yuri> differently in that regard between X and Wayland and/or depended on
    Yuri> other factors other than poorly configured screen DPI.

Fontconfig talks about sizes in points (but allows them to be
fractional, the underlying data type is a double)

Robert
-- 

Re: Font size

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  masm+emacs@masm11.me,  Emacs developers
>  <emacs-devel@gnu.org>,  Kévin Le Gouguec
>  <kevin.legouguec@gmail.com>
> Date: Thu, 05 Aug 2021 16:51:57 +0200
> 
> >>>>> On Thu, 5 Aug 2021 01:33:16 +0700, Yuri Khan <yuri.v.khan@gmail.com> said:
> 
>     Yuri> On Thu, 5 Aug 2021 at 01:24, Eli Zaretskii <eliz@gnu.org> wrote:
>     >> My question was whether the same units are likely to be used in the
>     >> X+GTK build as they are in the PGTK build.  IOW, that your guess of
>     >> the units being 1/96 inch is true or false for both of them.
> 
>     Yuri> I’m not related to GNOME or GTK+ development, but I would be much
>     Yuri> surprised if font sizes were specified in anything other than points
>     Yuri> (which are 1/72 in, not 1/96 in); and especially if GTK behaved
>     Yuri> differently in that regard between X and Wayland and/or depended on
>     Yuri> other factors other than poorly configured screen DPI.
> 
> Fontconfig talks about sizes in points (but allows them to be
> fractional, the underlying data type is a double)

I wasn't talking about the Fontconfig format of specifying a font, I
was talking about the GTK format.  They are different, see the node
"Fonts" in the Emacs manual.

Re: Font size (was: Merging the pgtk branch)

[Eli Zaretskii]

> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
> Date: Wed, 04 Aug 2021 14:53:04 +0200
> Cc: Yuuki Harano <masm+emacs@masm11.me>, Eli Zaretskii <eliz@gnu.org>
> 
> $ gsettings set org.gnome.desktop.interface monospace-font-name 'DejaVu Sans Mono 10.5'
> 
> Emacs emits a warning:
> 
> > set-face-attribute: Font not available: #<font-spec nil nil DejaVu\
> > Sans\ Mono\ 10\.5 nil nil nil nil nil nil nil nil nil ((:name
> > . "DejaVu Sans Mono 10.5") (:user-spec . "DejaVu Sans Mono 10.5"))>
> 
> Is this expected?

I don't know if it's expected, but the error message says that the
"10.5" part is not expected there.  What happens if you use "10"
instead of "10.5"?

Re: Font size

[Kévin Le Gouguec]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Kévin Le Gouguec <kevin.legouguec@gmail.com>
>> Date: Wed, 04 Aug 2021 14:53:04 +0200
>> Cc: Yuuki Harano <masm+emacs@masm11.me>, Eli Zaretskii <eliz@gnu.org>
>> 
>> $ gsettings set org.gnome.desktop.interface monospace-font-name 'DejaVu Sans Mono 10.5'
>> 
>> Emacs emits a warning:
>> 
>> > set-face-attribute: Font not available: #<font-spec nil nil DejaVu\
>> > Sans\ Mono\ 10\.5 nil nil nil nil nil nil nil nil nil ((:name
>> > . "DejaVu Sans Mono 10.5") (:user-spec . "DejaVu Sans Mono 10.5"))>
>> 
>> Is this expected?
>
> I don't know if it's expected, but the error message says that the
> "10.5" part is not expected there.  What happens if you use "10"
> instead of "10.5"?

Cf. the paragraph just above:

> On master --with-gconf and feature/pgtk, with font-use-system-font set
> to t, here is what C-u C-x = says for various GSettings:
> 
> GSettings³      C-u C-x =
> 9               12
> 10              13
> 10.5            ERROR (see below)
> 11              15
> 
> ³ "XX" in "gsettings org.gnome.desktop.interface monospace-font-name 'DejaVu Sans Mono XX'"

What I don't understand:

1. How come GSettings/Xresources and Emacs use different "units"?
   (Apologies if the answer lies somewhere in the manual, which it most
   certainly does)

2. Why does 10.5 work with Xresources and not GSettings?

Re: Merging the pgtk branch

[Yuuki Harano]

On Sun, 01 Aug 2021 11:53:16 +0300,
	Eli Zaretskii <eliz@gnu.org> 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

Re: Merging the pgtk branch

[Eli Zaretskii]

> Date: Wed, 11 Aug 2021 00:15:15 +0900 (JST)
> Cc: emacs-devel@gnu.org
> From: Yuuki Harano <masm+emacs@masm11.me>
> 
> > 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.

Yes, that's what I meant.

> #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

Got it, thanks.

> >> +#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.

I hope you will recall it.  Or maybe just disable the change and see
what goes wrong?

I'm not really opposed to the change, I'm just surprised it is needed,
and if indeed it is needed, I would like to have a comment there
explaining why.

> >> +#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.

Ok, but if you do want to leave the workaround, please add a comment
with the above explanation of the reason.

> >> +#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.

OK, but please add a comment near FRAME_X_P's definition to the effect
that FRAME_X_P will return false for PGTK although FRAME_X_DISPLAY is
still valid.  (But see below: perhaps a more significant change
described there will make this a moot point.)

> >> -  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

OK, but we have the same problem with other display types as well.
For example, the MS-Windows display has

  f->output_data.w32->...

The code is still conditional, allright, but the conditions are hidden
in the definitions of FRAME_DISPLAY_INFO, FRAME_FONT, etc.  Can't we
do something similar for PGTK?  If necessary, we could add more
FRAME_xxx macros to hide the conditionals, where the X and the PGTK
members need to be accessed differently.

Thanks!

Re: Merging the pgtk branch

[Yuuki Harano]

On Wed, 11 Aug 2021 00:15:15 +0900 (JST),
	Yuuki Harano <masm+emacs@masm11.me> 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.

Done.


>>> +#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.

Done.


>>> -#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.

Done.


>>> - `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.

Done.


>>> @@ -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.

I thought that gui_set_border_width could change the border width of a frame.
But there is the code:
---
    if (FRAME_NATIVE_WINDOW (f) != 0)
      error ("Cannot change the border width of a frame");
---
I was confused, and added #ifndef around it.

I don't know about the function.
What is it for?


>>> @@ -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.

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.

Reverted.


>>> +#ifndef PGTK_TRACE
>>> +#define PGTK_TRACE(fmt, ...) ((void) 0)
>>> +#endif
>> 
>> Do we still need PGTK_TRACE (and all its calls)?
> 
> Maybe, no need.

All removed.


>>> +#ifdef PGTK_DEBUG
>> 
>> Do we need this PGTK_DEBUG stuff and its callers?
> 
> Maybe, no need.

All removed.


>>> +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.

Done.


>>> +#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.

Done.


>>> +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.

Done.


>>> @@ -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.

Done.

-- 
Yuuki Harano

Re: Merging the pgtk branch

[Po Lu]

Are there still any obstacles to merging the pgtk branch?

If so, which?  I think I might have the time to do some light work to
resolve them, if they're not too major.

Thanks.

Re: Merging the pgtk branch

[Robert Pluim]

>>>>> On Thu, 02 Dec 2021 11:45:02 +0800, Po Lu <luangruo@yahoo.com> said:

    Po> Are there still any obstacles to merging the pgtk branch?
    Po> If so, which?  I think I might have the time to do some light work to
    Po> resolve them, if they're not too major.

M-x menu-set-font is still broken in that branch.

If you can find a solution to frame positioning, that would be
wonderful, but I think thatʼs a losing proposition, given the attitude
of Wayland.

Robert
-- 

Re: Merging the pgtk branch

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

> M-x menu-set-font is still broken in that branch.

I will look into that, thanks!

> If you can find a solution to frame positioning, that would be
> wonderful, but I think thatʼs a losing proposition, given the attitude
> of Wayland.

I doubt it will work on Wayland, sadly.  However, it ought to work if
GDK_BACKEND is x11.

> Robert

Re: Merging the pgtk branch

[Robert Pluim]

>>>>> On Thu, 02 Dec 2021 17:50:05 +0800, Po Lu <luangruo@yahoo.com> said:

    Po> Robert Pluim <rpluim@gmail.com> writes:
    >> If you can find a solution to frame positioning, that would be
    >> wonderful, but I think thatʼs a losing proposition, given the attitude
    >> of Wayland.

    Po> I doubt it will work on Wayland, sadly.  However, it ought to work if
    Po> GDK_BACKEND is x11.

I tend to agree. I did try creating a full-screen invisible top-level
frame, and then creating the ordinary frames as child-frames of that,
but it didnʼt work very well (and would no doubt get broken by some
future change to Wayland and/or the compositors).

Robert
-- 

Re: Merging the pgtk branch

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

> I tend to agree. I did try creating a full-screen invisible top-level
> frame, and then creating the ordinary frames as child-frames of that,
> but it didnʼt work very well (and would no doubt get broken by some

"Full screen surface with attached subsurfaces" is quite against the
design of Wayland, so I doubt it would ever work.

Wayland doesn't really have a single type of surface (window), and
concepts that can be applied to subsurfaces cannot be applied to the
similarly named but entirely different regular surfaces.

Re: Merging the pgtk branch

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

> M-x menu-set-font is still broken in that branch.

I tried to fix it, please test, thanks.

Re: Merging the pgtk branch

[Robert Pluim]

>>>>> On Thu, 02 Dec 2021 18:04:50 +0800, Po Lu <luangruo@yahoo.com> said:

    Po> Robert Pluim <rpluim@gmail.com> writes:
    >> M-x menu-set-font is still broken in that branch.

    Po> I tried to fix it, please test, thanks.

Works fine, thanks.

Robert
-- 

Re: Merging the pgtk branch

[Eli Zaretskii]

> Date: Tue, 30 Nov 2021 00:41:15 +0900 (JST)
> Cc: emacs-devel@gnu.org
> From: Yuuki Harano <masm+emacs@masm11.me>
> 
> >>> @@ -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.
> 
> I thought that gui_set_border_width could change the border width of a frame.
> But there is the code:
> ---
>     if (FRAME_NATIVE_WINDOW (f) != 0)
>       error ("Cannot change the border width of a frame");
> ---
> I was confused, and added #ifndef around it.
> 
> I don't know about the function.
> What is it for?

It's for setting the width of the "external border" of the frame,
something that window managers generally disallow.  Doesn't PGTK
define FRAME_NATIVE_WINDOW to return non-zero for any GUI frame?  If
so, you don't have to #ifdef this, it will work as with any other
window-system.

Thanks.

Re: Merging the pgtk branch

[Yuuki Harano]

On Sun, 05 Dec 2021 16:08:05 +0200,
	Eli Zaretskii <eliz@gnu.org> wrote:
>> I thought that gui_set_border_width could change the border width of a frame.
>> But there is the code:
>> ---
>>     if (FRAME_NATIVE_WINDOW (f) != 0)
>>       error ("Cannot change the border width of a frame");
>> ---
>> I was confused, and added #ifndef around it.
>> 
>> I don't know about the function.
>> What is it for?
> 
> It's for setting the width of the "external border" of the frame,
> something that window managers generally disallow.  Doesn't PGTK
> define FRAME_NATIVE_WINDOW to return non-zero for any GUI frame?  If
> so, you don't have to #ifdef this, it will work as with any other
> window-system.

Thanks.
I reverted those changes.

And I think I did all the fixes which are necessary by review comments by you.
-- 
Yuuki Harano

Re: Merging the pgtk branch

[Eli Zaretskii]

> Date: Mon, 06 Dec 2021 01:01:17 +0900 (JST)
> Cc: emacs-devel@gnu.org
> From: Yuuki Harano <masm+emacs@masm11.me>
> 
> I reverted those changes.
> 
> And I think I did all the fixes which are necessary by review comments by you.

Thanks, I guess it is now up to the people who use(d) this branch: if
they say it's in good shape, we are ready to land it on master.

Re: Merging the pgtk branch

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, I guess it is now up to the people who use(d) this branch: if
> they say it's in good shape, we are ready to land it on master.

That's great news!

Re: Merging the pgtk branch

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> Thanks, I guess it is now up to the people who use(d) this branch: if
>> they say it's in good shape, we are ready to land it on master.
>
> That's great news!

The child frame feature on that branch is totally broken.

Evaluting the following code is enough to make it emit tens of GLib
warning messages (the product of use-after-frees) and eventually crash:

  (make-frame `((parent-frame . ,(selected-frame))))

Most of the code in gtkutil and pgtkterm is also unprepared to deal with
frames that don't have an outer widget, which is the case with pgtk
child frames.  That code will have to be carefully reviewed and
reworked.

Needless to say, unparenting child frames or reparenting child frames to
another frame is even more hopeless.

I plan to rework that feature.  Before that, please do not merge the
pgtk branch.

Re: Merging the pgtk branch

[Yuuki Harano]

On Mon, 06 Dec 2021 12:41:30 +0800,
	Po Lu <luangruo@yahoo.com> wrote:
> Evaluting the following code is enough to make it emit tens of GLib
> warning messages (the product of use-after-frees) and eventually crash:
> 
>   (make-frame `((parent-frame . ,(selected-frame))))

"Tens of GLib warnings messages" does not reproduce for me.  Gtk+ 3.24.30.
Please tell me the detail.

I found a way to crash:
----
(setq f1 (selected-frame))
(setq f2 (make-frame))
(setq f (make-frame `((parent-frame . ,f1) (width . 10) (height . 5))))
(modify-frame-parameters f1 `((parent-frame . ,f2)))
(modify-frame-parameters f1 `((parent-frame . ,nil)))
----
I'll debug this.

-- 
Yuuki Harano

Re: Merging the pgtk branch

[Po Lu]

Yuuki Harano <masm+emacs@masm11.me> writes:

> "Tens of GLib warnings messages" does not reproduce for me.  Gtk+ 3.24.30.
> Please tell me the detail.

I fixed the warning messages.  Turns out it was the code in
`xg_set_special_colors' using FRAME_GTK_OUTER_WIDGET (which is NULL in
pgtk child frames), which makes GTK emit lots of warning messages.

> I found a way to crash:
> ----
> (setq f1 (selected-frame))
> (setq f2 (make-frame))
> (setq f (make-frame `((parent-frame . ,f1) (width . 10) (height . 5))))
> (modify-frame-parameters f1 `((parent-frame . ,f2)))
> (modify-frame-parameters f1 `((parent-frame . ,nil)))
> ----
> I'll debug this.

Thanks.

Re: Merging the pgtk branch

[Yuuki Harano]

On Mon, 06 Dec 2021 21:24:12 +0800,
	Po Lu <luangruo@yahoo.com> wrote:
> I fixed the warning messages.  Turns out it was the code in
> `xg_set_special_colors' using FRAME_GTK_OUTER_WIDGET (which is NULL in
> pgtk child frames), which makes GTK emit lots of warning messages.

Thanks.

>> I found a way to crash:
>> ----
>> (setq f1 (selected-frame))
>> (setq f2 (make-frame))
>> (setq f (make-frame `((parent-frame . ,f1) (width . 10) (height . 5))))
>> (modify-frame-parameters f1 `((parent-frame . ,f2)))
>> (modify-frame-parameters f1 `((parent-frame . ,nil)))
>> ----
>> I'll debug this.
> 
> Thanks.

I tried to fix it.

-- 
Yuuki Harano

Re: Merging the pgtk branch

[Po Lu]

Yuuki Harano <masm+emacs@masm11.me> writes:

> I tried to fix it.

Seems to work here, thanks.  BTW, I think the child frame implementation
needs some sort of "window" manager, so things like setting the Z group
can work with them.

Re: Merging the pgtk branch

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Seems to work here, thanks.  BTW, I think the child frame
> implementation needs some sort of "window" manager, so things like
> setting the Z group can work with them.

I looked through the rest of the code in pgtkterm.c, pgtkfns.c and
gtkutil.c, and barring some bits that don't actually get compiled on
PGTK, it looks good to me now.

So I think we're ready to merge the feature branch to master.  Yuuki?

Thanks.

Re: Merging the pgtk branch

[Yuuki Harano]

On Sat, 18 Dec 2021 15:58:18 +0800,
	Po Lu <luangruo@yahoo.com> wrote:
> So I think we're ready to merge the feature branch to master.  Yuuki?

Thank you for your review and some fixes.
I think so, too.

Could you someone merge it to master?  Or may I do by myself?

-- 
Yuuki Harano

Re: Merging the pgtk branch

[Eli Zaretskii]

> Date: Sat, 18 Dec 2021 21:52:00 +0900 (JST)
> Cc: eliz@gnu.org, emacs-devel@gnu.org
> From: Yuuki Harano <masm+emacs@masm11.me>
> 
> 
> On Sat, 18 Dec 2021 15:58:18 +0800,
> 	Po Lu <luangruo@yahoo.com> wrote:
> > So I think we're ready to merge the feature branch to master.  Yuuki?
> 
> Thank you for your review and some fixes.
> I think so, too.
> 
> Could you someone merge it to master?  Or may I do by myself?

You have write access, so you can do it yourself.

Re: Merging the pgtk branch

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 18 Dec 2021 21:52:00 +0900 (JST)
>> Cc: eliz@gnu.org, emacs-devel@gnu.org
>> From: Yuuki Harano <masm+emacs@masm11.me>
>> 
>> 
>> On Sat, 18 Dec 2021 15:58:18 +0800,
>> 	Po Lu <luangruo@yahoo.com> wrote:
>> > So I think we're ready to merge the feature branch to master.  Yuuki?
>> 
>> Thank you for your review and some fixes.
>> I think so, too.
>> 
>> Could you someone merge it to master?  Or may I do by myself?

> You have write access, so you can do it yourself.

Please wait for me to merge master to feature/pgtk one last time before
merging the pgtk branch -- I just realized that the change to
Fwindow_text_pixel_size's signature needs an adjustment on the pgtk end
to work as well.

Thanks.

Re: Merging the pgtk branch

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Yuuki Harano <masm+emacs@masm11.me>,  emacs-devel@gnu.org
> Date: Sat, 18 Dec 2021 20:56:15 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Date: Sat, 18 Dec 2021 21:52:00 +0900 (JST)
> >> Cc: eliz@gnu.org, emacs-devel@gnu.org
> >> From: Yuuki Harano <masm+emacs@masm11.me>
> >> 
> >> 
> >> On Sat, 18 Dec 2021 15:58:18 +0800,
> >> 	Po Lu <luangruo@yahoo.com> wrote:
> >> > So I think we're ready to merge the feature branch to master.  Yuuki?
> >> 
> >> Thank you for your review and some fixes.
> >> I think so, too.
> >> 
> >> Could you someone merge it to master?  Or may I do by myself?
> 
> > You have write access, so you can do it yourself.
> 
> Please wait for me to merge master to feature/pgtk one last time before
> merging the pgtk branch -- I just realized that the change to
> Fwindow_text_pixel_size's signature needs an adjustment on the pgtk end
> to work as well.

Only if the branch uses that function in places different from the
code on master.  Otherwise, Git will do the job.

Re: Merging the pgtk branch

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Only if the branch uses that function in places different from the
> code on master.

This is the case here; it has been fixed on the feature branch, which
I'll be merging to master shortly.

Re: Merging the pgtk branch

[Po Lu]

Yuuki Harano <masm+emacs@masm11.me> writes:

> Thank you for your review and some fixes.
> I think so, too.
>
> Could you someone merge it to master?  Or may I do by myself?

I don't know if there's any special procedure for merging feature
branches.  Eli?

I verified that it doesn't break the build on any platform I tested (all
possible combinations of X and Haiku).  Someone might have to test on
MS-Windows and macOS though.

Thanks.

Re: Merging the pgtk branch

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: eliz@gnu.org,  emacs-devel@gnu.org
> Date: Sat, 18 Dec 2021 20:54:54 +0800
> 
> Yuuki Harano <masm+emacs@masm11.me> writes:
> 
> > Thank you for your review and some fixes.
> > I think so, too.
> >
> > Could you someone merge it to master?  Or may I do by myself?
> 
> I don't know if there's any special procedure for merging feature
> branches.  Eli?

Just "git merge", and pay attention to the log entry.

After some time, remember to delete the feature branch.

Re: Merging the pgtk branch

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Just "git merge", and pay attention to the log entry.

So I should write a detailed ChangeLog for the merge commit?  I'll get
right on that, thanks.

> After some time, remember to delete the feature branch.

BTW, shouldn't we put an error message somewhere in configure.ac telling
people that the branch is now on master and that the feature branch is
obsolete?

Lots of people are tracking `feature/pgtk' right now, and I don't want a
repeat of the "people are using feature/native-comp after it was merged
to master" situation we had earlier.

Thanks.

Re: Merging the pgtk branch

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> So I should write a detailed ChangeLog for the merge commit?  I'll get
> right on that, thanks.

Nevermind, that doesn't seem to be the precedent here.  Please disregard
this question.

Thanks.

Re: Merging the pgtk branch

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> So I should write a detailed ChangeLog for the merge commit?  I'll get
>> right on that, thanks.
>
> Nevermind, that doesn't seem to be the precedent here.  Please disregard
> this question.
>
> Thanks.

I did the merge.  Could the people who are able to verify that the
MS-Windows build is not broken?

Thanks.

Re: Merging the pgtk branch

[Lars Ingebrigtsen]

Po Lu <luangruo@yahoo.com> writes:

> I did the merge.

Great!

> Could the people who are able to verify that the MS-Windows build is
> not broken?

git on Savannah seems to be very slow (or down?) now, so I haven't been
able to pull on Windows yet, but I get the following warning on Debian
(without pgtk):

In end of data:
net/browse-url.el:832:28: Warning: the function `pgtk-backend-display-class' is not known to be defined.


-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Merging the pgtk branch

[Po Lu via Emacs development discussions.]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> git on Savannah seems to be very slow (or down?) now, so I haven't been
> able to pull on Windows yet, but I get the following warning on Debian
> (without pgtk):

It's slow for me as well.  Many apologies to the Savannah hackers if
this merge caused the problems.

> In end of data:
> net/browse-url.el:832:28: Warning: the function `pgtk-backend-display-class' is not known to be defined.

Thanks, I'll fix that ASAP.

Re: Merging the pgtk branch

[Eli Zaretskii]

> Cc: Eli Zaretskii <eliz@gnu.org>,  masm+emacs@masm11.me,  emacs-devel@gnu.org
> Date: Sat, 18 Dec 2021 21:39:34 +0800
> From:  Po Lu via "Emacs development discussions." <emacs-devel@gnu.org>
> 
> > In end of data:
> > net/browse-url.el:832:28: Warning: the function `pgtk-backend-display-class' is not known to be defined.
> 
> Thanks, I'll fix that ASAP.

I already did.

Re: Merging the pgtk branch

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> git on Savannah seems to be very slow (or down?) now, so I haven't been
> able to pull on Windows yet

And Savannah started working again, and the build went without a hitch
here (Windows 11 with mingw).

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: Merging the pgtk branch

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: masm+emacs@masm11.me,  emacs-devel@gnu.org
> Date: Sat, 18 Dec 2021 21:26:07 +0800
> 
> I did the merge.  Could the people who are able to verify that the
> MS-Windows build is not broken?

Thanks, seems to build and work OK here.

Re: Merging the pgtk branch

[Yuuki Harano]

On Sat, 18 Dec 2021 21:26:07 +0800,
	Po Lu <luangruo@yahoo.com> wrote:
> I did the merge.

Thank you!

-- 
Yuuki Harano

Re: Merging the pgtk branch

[Bozhidar Batsov]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

Quick question - is pgtk supposed to become the default front-end on Unix systems? I'm just wondering if I'll need a custom Emacs build for Linux after Emacs 28 is released officially. In general I'm not even sure if that's a decision for the Emacs dev team or for the various package maintainers. 

[-- Attachment #2: Type: text/html, Size: 464 bytes --]

Re: Merging the pgtk branch

[Eli Zaretskii]

> Date: Sat, 18 Dec 2021 16:15:10 +0200
> From: "Bozhidar Batsov" <bozhidar@batsov.dev>
> 
> Quick question - is pgtk supposed to become the default front-end on Unix systems? I'm just wondering if I'll
> need a custom Emacs build for Linux after Emacs 28 is released officially. In general I'm not even sure if
> that's a decision for the Emacs dev team or for the various package maintainers. 

What's "the default front-end on Unix systems"?  I don't think we have
one now, we just build with whatever we find on the build system, and
rely on the user to specify the options at configure time.

So I'm not sure I understand the question.

Re: Merging the pgtk branch

[Bozhidar Batsov]

[-- Attachment #1: Type: text/plain, Size: 1857 bytes --]

Fair enough, let me try to clarify my question. I had just assumed that all Unix systems (except macOS) default to the old GTK front-end (when it comes to Emacs with GUI, that is) and I was wondering if the expectation was that pgtk would mostly replace it right away (e.g. because of it's improved compatibility with Wayland) once Emacs 28 is released. 

My practical problem is that Emacs doesn't scale properly with Wayland on a HiDPI display, and by reading about pgtk I got the impression that it will address this issue. I installed Emacs-snapshot on Ubuntu, but it seems it's not built using pgtk, so I was wondering if I should ping the package maintainers to consider enabling it, or if there was some general consensus that the old GTK frontend will be replaced by the new one in most use-cases, as in theory it looks superior. I know I can just build Emacs from source however I see fit, but I was curious what's the expectation of the Emacs devs for the uptake of the new frontend in *BSD, Linux, etc. Perhaps I've missed an earlier discussion on the subject and I'd appreciate it if someone simply points me to it.  

On Sat, Dec 18, 2021, at 4:21 PM, Eli Zaretskii wrote:
> > Date: Sat, 18 Dec 2021 16:15:10 +0200
> > From: "Bozhidar Batsov" <bozhidar@batsov.dev>
> > 
> > Quick question - is pgtk supposed to become the default front-end on Unix systems? I'm just wondering if I'll
> > need a custom Emacs build for Linux after Emacs 28 is released officially. In general I'm not even sure if
> > that's a decision for the Emacs dev team or for the various package maintainers. 
> 
> What's "the default front-end on Unix systems"?  I don't think we have
> one now, we just build with whatever we find on the build system, and
> rely on the user to specify the options at configure time.
> 
> So I'm not sure I understand the question.
> 
> 

[-- Attachment #2: Type: text/html, Size: 2385 bytes --]

Re: Merging the pgtk branch

[Philip Kaludercic]

"Bozhidar Batsov" <bozhidar@batsov.dev> writes:

> Fair enough, let me try to clarify my question. I had just assumed
> that all Unix systems (except macOS) default to the old GTK front-end

So what you mean to ask is "will Emacs use PGTK by default when it is
build using GTK"?

> (when it comes to Emacs with GUI, that is) and I was wondering if the
> expectation was that pgtk would mostly replace it right away
> (e.g. because of it's improved compatibility with Wayland) once Emacs
> 28 is released.

Note that this was merged on to the master branch, not the emacs-28
branch (that is being prepared for release).

-- 
	Philip Kaludercic

Re: Merging the pgtk branch

[Eli Zaretskii]

> Date: Sat, 18 Dec 2021 17:12:46 +0200
> From: "Bozhidar Batsov" <bozhidar@batsov.dev>
> 
> Fair enough, let me try to clarify my question. I had just assumed that all Unix systems (except macOS)
> default to the old GTK front-end (when it comes to Emacs with GUI, that is) and I was wondering if the
> expectation was that pgtk would mostly replace it right away (e.g. because of it's improved compatibility with
> Wayland) once Emacs 28 is released. 

AFAIU, you _have_ to specify --with-x-toolkit=VALUE at configure time;
there's no default.  If you don't specify it, you get an X build with
no toolkit.

As for PGTK: we didn't yet decide whether to promote it as the
recommended toolkit.  AFAIU, there are some issues with it (whose root
cause is how Wayland works), and we are not yet sure whether users
will live in peace with those issues.  We need more feedback and more
user experience for that.

Re: Merging the pgtk branch

[Ken Brown]

On 12/18/2021 10:48 AM, Eli Zaretskii wrote:
>> Date: Sat, 18 Dec 2021 17:12:46 +0200
>> From: "Bozhidar Batsov" <bozhidar@batsov.dev>
>>
>> Fair enough, let me try to clarify my question. I had just assumed that all Unix systems (except macOS)
>> default to the old GTK front-end (when it comes to Emacs with GUI, that is) and I was wondering if the
>> expectation was that pgtk would mostly replace it right away (e.g. because of it's improved compatibility with
>> Wayland) once Emacs 28 is released.
> 
> AFAIU, you _have_ to specify --with-x-toolkit=VALUE at configure time;
> there's no default.  If you don't specify it, you get an X build with
> no toolkit.

Currently, if you don't specify it, you get the GTK+ build (assuming the 
appropriate development libraries are installed).

Ken

Re: Merging the pgtk branch

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> AFAIU, you _have_ to specify --with-x-toolkit=VALUE at configure time;
> there's no default.  If you don't specify it, you get an X build with
> no toolkit.

With "./configure", I get GTK here.

Re: Merging the pgtk branch

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 18 Dec 2021 08:05:27 -0800
> Cc: emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > AFAIU, you _have_ to specify --with-x-toolkit=VALUE at configure time;
> > there's no default.  If you don't specify it, you get an X build with
> > no toolkit.
> 
> With "./configure", I get GTK here.

Sorry for posting misinformation, but I guess ./configure --help
"needs work".

Re: Merging the pgtk branch

[Po Lu]

Stefan Kangas <stefankangas@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> AFAIU, you _have_ to specify --with-x-toolkit=VALUE at configure time;
>> there's no default.  If you don't specify it, you get an X build with
>> no toolkit.
>
> With "./configure", I get GTK here.

The X and GTK build (even though flawed in _concept_) is extremely
reliable.  It was written and maintained by Jan Djarv, who was an
extremely skilled expert in this field for ~15 years.

We should not hurry to replace the defaults with anything else.

Re: Merging the pgtk branch

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 18 Dec 2021 17:12:46 +0200
>> From: "Bozhidar Batsov" <bozhidar@batsov.dev>
>> 
>> Fair enough, let me try to clarify my question. I had just assumed that all Unix systems (except macOS)
>> default to the old GTK front-end (when it comes to Emacs with GUI, that is) and I was wondering if the
>> expectation was that pgtk would mostly replace it right away (e.g. because of it's improved compatibility with
>> Wayland) once Emacs 28 is released. 
>
> AFAIU, you _have_ to specify --with-x-toolkit=VALUE at configure time;
> there's no default.  If you don't specify it, you get an X build with
> no toolkit.

> As for PGTK: we didn't yet decide whether to promote it as the
> recommended toolkit.  AFAIU, there are some issues with it (whose root
> cause is how Wayland works), and we are not yet sure whether users
> will live in peace with those issues.  We need more feedback and more
> user experience for that.

There are some other problems with it (by design): it's scantily tested,
and it doesn't respect Z groups in child frames, it doesn't work with
the X resource database (not everyone wants to use GSettings), and it
also lacks several other features.

This situation will take quite a while to improve.  Some of the problem
needs improvements in GTK+3, which will probably not happen due to the
attitude of the GTK developers.  Please don't make it the recommended
toolkit.

Thanks.

Re: Merging the pgtk branch

[Eli Zaretskii]

> Date: Sat, 18 Dec 2021 22:56:24 +0900 (JST)
> From: Yuuki Harano <masm+emacs@masm11.me>
> Cc: eliz@gnu.org, emacs-devel@gnu.org
> 
> 
> On Sat, 18 Dec 2021 21:26:07 +0800,
> 	Po Lu <luangruo@yahoo.com> wrote:
> > I did the merge.
> 
> Thank you!

Btw, PGTK build needs a NEWS entry.  Would someone please write one?

Re: Merging the pgtk branch

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 18 Dec 2021 22:56:24 +0900 (JST)
>> From: Yuuki Harano <masm+emacs@masm11.me>
>> Cc: eliz@gnu.org, emacs-devel@gnu.org
>> 
>> 
>> On Sat, 18 Dec 2021 21:26:07 +0800,
>> 	Po Lu <luangruo@yahoo.com> wrote:
>> > I did the merge.
>> 
>> Thank you!
>
> Btw, PGTK build needs a NEWS entry.  Would someone please write one?

Thanks, I'll add it to the pile of things I will do in an hour or so.

Re: Merging the pgtk branch

[Bozhidar Batsov]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

I've just built Emacs with pgtk and I can confirm it looks and works great with Wayland. Great work! 

On Sun, Dec 19, 2021, at 2:32 AM, Po Lu wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Date: Sat, 18 Dec 2021 22:56:24 +0900 (JST)
> >> From: Yuuki Harano <masm+emacs@masm11.me <mailto:masm%2Bemacs@masm11.me>>
> >> Cc: eliz@gnu.org, emacs-devel@gnu.org
> >> 
> >> 
> >> On Sat, 18 Dec 2021 21:26:07 +0800,
> >> Po Lu <luangruo@yahoo.com> wrote:
> >> > I did the merge.
> >> 
> >> Thank you!
> >
> > Btw, PGTK build needs a NEWS entry.  Would someone please write one?
> 
> Thanks, I'll add it to the pile of things I will do in an hour or so.
> 
> 

[-- Attachment #2: Type: text/html, Size: 1401 bytes --]

Re: Merging the pgtk branch

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: masm+emacs@masm11.me,  emacs-devel@gnu.org
> Date: Sat, 18 Dec 2021 21:26:07 +0800
> 
> I did the merge.  Could the people who are able to verify that the
> MS-Windows build is not broken?

Somebody should clean up the code in PGTK files: there are code
fragments in "#if 0" or commented out, etc.

Re: Merging the pgtk branch

[Stefan Monnier]

Po Lu [2021-12-18 21:26:07] wrote:
> I did the merge.

Yay!


        Stefan

Re: Merging the pgtk branch

[Sean Whitton]

Hello,

On Sat 18 Dec 2021 at 09:26PM +08, Po Lu wrote:

> I did the merge.  Could the people who are able to verify that the
> MS-Windows build is not broken?

Many many thanks to you and Yuuki and the other contributors, this is
great news.

-- 
Sean Whitton

Re: Merging the pgtk branch

[David Koppelman]

I'm glad it's finally on master! I'll report bugs to
the bug mailing list. (Fonts 3x too large. Etc.)


Po Lu <luangruo@yahoo.com> writes:

>
> I did the merge.  Could the people who are able to verify that the
> MS-Windows build is not broken?
>
> Thanks.

Re: Merging the pgtk branch

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: masm+emacs@masm11.me,  emacs-devel@gnu.org
> Date: Sat, 18 Dec 2021 21:01:29 +0800
> 
> BTW, shouldn't we put an error message somewhere in configure.ac telling
> people that the branch is now on master and that the feature branch is
> obsolete?

I'm not sure it's worth the hassle.

> Lots of people are tracking `feature/pgtk' right now, and I don't want a
> repeat of the "people are using feature/native-comp after it was merged
> to master" situation we had earlier.

Messages shown by configure are mostly ignored -- people go for a
coffee while Emacs builds.

Re: Merging the pgtk branch

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: masm+emacs@masm11.me,  emacs-devel@gnu.org
>> Date: Sat, 18 Dec 2021 21:01:29 +0800
>> 
>> BTW, shouldn't we put an error message somewhere in configure.ac telling
>> people that the branch is now on master and that the feature branch is
>> obsolete?
>
> I'm not sure it's worth the hassle.
>
>> Lots of people are tracking `feature/pgtk' right now, and I don't want a
>> repeat of the "people are using feature/native-comp after it was merged
>> to master" situation we had earlier.
>
> Messages shown by configure are mostly ignored -- people go for a
> coffee while Emacs builds.

Which is why I think it should be an error.  At least it would serve to
immediately notify the people building prebuilt packages for common GNU
distributions that they should start tracking master instead.

Thanks.

Re: Merging the pgtk branch

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: masm+emacs@masm11.me,  emacs-devel@gnu.org
> Date: Sat, 18 Dec 2021 21:29:56 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Po Lu <luangruo@yahoo.com>
> >> Cc: masm+emacs@masm11.me,  emacs-devel@gnu.org
> >> Date: Sat, 18 Dec 2021 21:01:29 +0800
> >> 
> >> BTW, shouldn't we put an error message somewhere in configure.ac telling
> >> people that the branch is now on master and that the feature branch is
> >> obsolete?
> >
> > I'm not sure it's worth the hassle.
> >
> >> Lots of people are tracking `feature/pgtk' right now, and I don't want a
> >> repeat of the "people are using feature/native-comp after it was merged
> >> to master" situation we had earlier.
> >
> > Messages shown by configure are mostly ignored -- people go for a
> > coffee while Emacs builds.
> 
> Which is why I think it should be an error.

You want to make configure on the branch to error out? fine by me.

Re: Merging the pgtk branch

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> You want to make configure on the branch to error out? fine by me.

Thanks!

Re: Merging the pgtk branch

[Stefan Monnier]

Po Lu [2021-12-18 21:33:49] wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>> You want to make configure on the branch to error out? fine by me.

You can also rename the branch to `old-branches/pgtk`.


        Stefan

Transitory GUI connections (was Re: Merging the pgtk branch)

[Alex Bennée]

Eli Zaretskii <eliz@gnu.org> writes:

> 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).
>
<snip>

I've switched to running feature/pgtk on my daily driver to kick the
tyres and see how things work. However I wonder about how this new code
deals with a failing desktop environment. When I start up I still get
the warning:

  Warning: due to a long standing Gtk+ bug
  https://gitlab.gnome.org/GNOME/gtk/issues/221
  Emacs might crash when run in daemon mode and the X11 connection is unexpectedly lost.
  Using an Emacs configured with --with-x-toolkit=lucid does not have this problem.

which doesn't inspire confidence. I've just taken down my Wayland/sway
session due to an unrelated bug and while I could see emacs --daemon
processes running the server socket in /run/user/1000/emacs had
disappeared. So a quick question:

  - Is the pgtk branch meant to address this longstanding issue?

Follow-up questions for debugging:

  - How do I check from inside Emacs is the GUI frame is connection via
    GTK/Wayland or is using GTK/X11 (via Xwayland)?

Thanks,

-- 
Alex Bennée

Re: Transitory GUI connections (was Re: Merging the pgtk branch)

[Po Lu]

Alex Bennée <alex.bennee@linaro.org> writes:

> I've switched to running feature/pgtk on my daily driver to kick the
> tyres and see how things work. However I wonder about how this new code
> deals with a failing desktop environment. When I start up I still get
> the warning:
>
>   Warning: due to a long standing Gtk+ bug
>   https://gitlab.gnome.org/GNOME/gtk/issues/221
>   Emacs might crash when run in daemon mode and the X11 connection is unexpectedly lost.
>   Using an Emacs configured with --with-x-toolkit=lucid does not have this problem.
>
> which doesn't inspire confidence. I've just taken down my Wayland/sway
> session due to an unrelated bug and while I could see emacs --daemon
> processes running the server socket in /run/user/1000/emacs had
> disappeared. So a quick question:
>
>   - Is the pgtk branch meant to address this longstanding issue?

I don't think any program can "address" this long-standing issue, as
long as said program uses GTK+, which dies whenever a display connection
closes underneath its nose.

Some time ago, I worked on a port of feature/pgtk to GTK+ 4, which
exhibited the same problem.  I'm not quite confident that situation will
change any time soon.

Re: Transitory GUI connections (was Re: Merging the pgtk branch)

[Robert Pluim]

>>>>> On Wed, 01 Sep 2021 16:06:37 +0100, Alex Bennée <alex.bennee@linaro.org> said:
    Alex> Follow-up questions for debugging:

    Alex>   - How do I check from inside Emacs is the GUI frame is connection via
    Alex>     GTK/Wayland or is using GTK/X11 (via Xwayland)?

C-h f pgtk-backend-display-class

Robert
-- 

Re: Merging the pgtk branch

[Vitaly Ankh]

I'm a current pgtk branch user and I think it's really in good shape now.
The only problem is xwidgets doesn't support pgtk branch, while we
could improve this situation after pgtk branch is merged into master.

Regards,
VitalyR