From: Yuuki Harano <masm+emacs@masm11.me>
To: eliz@gnu.org
Cc: emacs-devel@gnu.org
Subject: Re: Merging the pgtk branch
Date: Mon, 02 Aug 2021 01:38:59 +0900 (JST) [thread overview]
Message-ID: <20210802.013859.2264148309496126056.masm@luna.pink.masm11.me> (raw)
In-Reply-To: <835ywpo8ar.fsf@gnu.org>
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
next prev parent reply other threads:[~2021-08-01 16:38 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 [this message]
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
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
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210802.013859.2264148309496126056.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 public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).