all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Yuuki Harano <masm+emacs@masm11.me>
To: eliz@gnu.org
Cc: emacs-devel@gnu.org
Subject: Re: Merging the pgtk branch
Date: Sun, 14 Nov 2021 21:20:10 +0900 (JST)	[thread overview]
Message-ID: <20211114.212010.2167504856050535233.masm@luna.pink.masm11.me> (raw)
In-Reply-To: <20210802.013859.2264148309496126056.masm@luna.pink.masm11.me>

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



  parent reply	other threads:[~2021-11-14 12:20 UTC|newest]

Thread overview: 85+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-01  8:53 Merging the pgtk branch Eli Zaretskii
2021-08-01 16:38 ` Yuuki Harano
2021-08-01 16:57   ` Stefan Monnier
2021-08-02 14:17     ` Yuuki Harano
2021-08-02 11:49   ` Eli Zaretskii
2021-08-03 12:52     ` Yuuki Harano
2021-08-03 13:37       ` Stefan Monnier
2021-08-04 13:43         ` Yuuki Harano
2021-11-14 12:20   ` Yuuki Harano [this message]
2021-11-14 13:27     ` Eli Zaretskii
2021-11-14 14:55       ` Yuuki Harano
2021-08-04 12:53 ` Font size (was: Merging the pgtk branch) Kévin Le Gouguec
2021-08-04 13:45   ` Eli Zaretskii
2021-08-04 13:58     ` Font size Kévin Le Gouguec
2021-08-04 15:46       ` Yuri Khan
2021-08-04 16:11         ` Eli Zaretskii
2021-08-04 17:09           ` Yuri Khan
2021-08-04 18:23             ` Eli Zaretskii
2021-08-04 18:33               ` Yuri Khan
2021-08-05 14:51                 ` Robert Pluim
2021-08-05 16:19                   ` Eli Zaretskii
2021-08-04 13:55   ` Font size (was: Merging the pgtk branch) Eli Zaretskii
2021-08-04 14:15     ` Font size Kévin Le Gouguec
2021-08-10 15:15 ` Merging the pgtk branch Yuuki Harano
2021-08-10 16:19   ` Eli Zaretskii
2021-11-29 15:41   ` Yuuki Harano
2021-12-02  3:45     ` Po Lu
2021-12-02  9:47       ` Robert Pluim
2021-12-02  9:50         ` Po Lu
2021-12-02 10:06           ` Robert Pluim
2021-12-02 10:12             ` Po Lu
2021-12-02 10:04         ` Po Lu
2021-12-02 11:34           ` Robert Pluim
2021-12-05 14:08     ` Eli Zaretskii
2021-12-05 16:01       ` Yuuki Harano
2021-12-05 16:06         ` Eli Zaretskii
2021-12-06  0:56           ` Po Lu
2021-12-06  4:41             ` Po Lu
2021-12-06 13:12               ` Yuuki Harano
2021-12-06 13:24                 ` Po Lu
2021-12-06 16:01                   ` Yuuki Harano
2021-12-09  5:01                     ` Po Lu
2021-12-18  7:58                       ` Po Lu
2021-12-18 12:52                         ` Yuuki Harano
2021-12-18 12:53                           ` Eli Zaretskii
2021-12-18 12:56                             ` Po Lu
2021-12-18 13:00                               ` Eli Zaretskii
2021-12-18 13:02                                 ` Po Lu
2021-12-18 12:54                           ` Po Lu
2021-12-18 12:58                             ` Eli Zaretskii
2021-12-18 13:01                               ` Po Lu
2021-12-18 13:24                                 ` Po Lu
2021-12-18 13:26                                   ` Po Lu
2021-12-18 13:37                                     ` Lars Ingebrigtsen
2021-12-18 13:39                                       ` Po Lu via Emacs development discussions.
2021-12-18 14:18                                         ` Eli Zaretskii
2021-12-18 13:45                                       ` Lars Ingebrigtsen
2021-12-18 13:40                                     ` Eli Zaretskii
2021-12-18 13:56                                     ` Yuuki Harano
2021-12-18 14:15                                       ` Bozhidar Batsov
2021-12-18 14:21                                         ` Eli Zaretskii
2021-12-18 15:12                                           ` Bozhidar Batsov
2021-12-18 15:37                                             ` Philip Kaludercic
2021-12-18 15:48                                             ` Eli Zaretskii
2021-12-18 15:56                                               ` Ken Brown
2021-12-18 16:05                                               ` Stefan Kangas
2021-12-18 16:10                                                 ` Eli Zaretskii
2021-12-19  0:30                                                 ` Po Lu
2021-12-19  0:28                                               ` Po Lu
2021-12-18 16:15                                       ` Eli Zaretskii
2021-12-19  0:32                                         ` Po Lu
2021-12-19  9:39                                           ` Bozhidar Batsov
2021-12-18 14:15                                     ` Eli Zaretskii
2021-12-18 19:10                                     ` Stefan Monnier
2021-12-18 21:50                                     ` Sean Whitton
2021-12-18 23:15                                     ` David Koppelman
2021-12-18 13:26                                 ` Eli Zaretskii
2021-12-18 13:29                                   ` Po Lu
2021-12-18 13:32                                     ` Eli Zaretskii
2021-12-18 13:33                                       ` Po Lu
2021-12-18 19:08                                         ` Stefan Monnier
2021-09-01 15:06 ` Transitory GUI connections (was Re: Merging the pgtk branch) Alex Bennée
2021-09-02  6:22   ` Po Lu
2021-09-17 15:09   ` Robert Pluim
  -- strict thread matches above, loose matches on Subject: below --
2021-12-05 19:14 Merging the pgtk branch Vitaly Ankh

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211114.212010.2167504856050535233.masm@luna.pink.masm11.me \
    --to=masm+emacs@masm11.me \
    --cc=eliz@gnu.org \
    --cc=emacs-devel@gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.