From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Yuuki Harano Newsgroups: gmane.emacs.devel Subject: Re: Merging the pgtk branch Date: Mon, 02 Aug 2021 01:38:59 +0900 (JST) Organization: Ingage Inc. Message-ID: <20210802.013859.2264148309496126056.masm@luna.pink.masm11.me> References: <835ywpo8ar.fsf@gnu.org> Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="27994"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: eliz@gnu.org Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Aug 01 18:40:04 2021 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1mAEVP-00073P-KT for ged-emacs-devel@m.gmane-mx.org; Sun, 01 Aug 2021 18:40:04 +0200 Original-Received: from localhost ([::1]:55960 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mAEVO-0003Ct-2p for ged-emacs-devel@m.gmane-mx.org; Sun, 01 Aug 2021 12:40:02 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:47872) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mAEUZ-0002Ug-Tj for emacs-devel@gnu.org; Sun, 01 Aug 2021 12:39:13 -0400 Original-Received: from shiro.masm11.me ([150.95.182.25]:57628) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mAEUW-0007NY-Kw; Sun, 01 Aug 2021 12:39:11 -0400 Original-Received: from luna.pink.masm11.me (KD106180014011.au-net.ne.jp [106.180.14.11]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shiro.masm11.me (Postfix) with ESMTPSA id 4FF5DC0086; Mon, 2 Aug 2021 01:39:00 +0900 (JST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=masm11.me; s=202002; t=1627835940; bh=K+WBQ2j/Gd3sIiYtXFqW9DqJSGMNQtbDZd0E2VmsTVM=; h=Date:To:Cc:Subject:From:In-Reply-To:References; b=hTRlt4OT1hgSmzFKq8VrTrmdZvxXz0kez7FLzPHxAAOxFj3HwKwReduRUCTM/SqOr kaq+ZjrnW+Cq+BI7gx+bp8dEf1Ruqj7qB1UbE2eGzJNd5L3LYcBbfPRDae3EzGdYeO xxkEIR/3ulf1x67LK967aOTsL0ypYulSxhSeYaBM= In-Reply-To: <835ywpo8ar.fsf@gnu.org> X-Mailer: Mew version 6.8 on Emacs 28.0 Received-SPF: pass client-ip=150.95.182.25; envelope-from=masm@masm11.me; helo=shiro.masm11.me X-Spam_score_int: -20 X-Spam_score: -2.1 X-Spam_bar: -- X-Spam_report: (-2.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=ham autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:271919 Archived-At: On Sun, 01 Aug 2021 11:53:16 +0300, Eli Zaretskii 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