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