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: Sun, 14 Nov 2021 21:20:10 +0900 (JST) Organization: Ingage Inc. Message-ID: <20211114.212010.2167504856050535233.masm@luna.pink.masm11.me> References: <835ywpo8ar.fsf@gnu.org> <20210802.013859.2264148309496126056.masm@luna.pink.masm11.me> 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="35855"; 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 Nov 14 13:21:56 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 1mmEWB-00093B-Ht for ged-emacs-devel@m.gmane-mx.org; Sun, 14 Nov 2021 13:21:56 +0100 Original-Received: from localhost ([::1]:34630 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1mmEW9-0004uz-MJ for ged-emacs-devel@m.gmane-mx.org; Sun, 14 Nov 2021 07:21:53 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:60662) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mmEVU-0004Dd-B6 for emacs-devel@gnu.org; Sun, 14 Nov 2021 07:21:12 -0500 Original-Received: from shiro.masm11.me ([150.95.182.25]:56384) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mmEVQ-0005Dz-LB; Sun, 14 Nov 2021 07:21:12 -0500 Original-Received: from luna.pink.masm11.me (i153-144-34-242.s41.a033.ap.plala.or.jp [153.144.34.242]) (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 E610CC011D; Sun, 14 Nov 2021 21:21:00 +0900 (JST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=masm11.me; s=202002; t=1636892461; bh=BgmGB1M52dZZsUEJq/ayqXfhSMljM4Ba2/nsEfL6i18=; h=Date:To:Cc:Subject:From:In-Reply-To:References; b=zsJlxflZM2xotfiCXfpwo3U5OZO/g4g9KukNCZRYPBKHq6+7hWa6Ey7L9yCoAmnBV I3Z+Iwv5lChueokKw6eIij6u6TpfDF4FPzkCs1fkmR2qmabbn4S0hkrTDkYnGFlZ1a EX+Q8COBP31kQMGDbcsacpLihyp3OQhT0O1dW9s8= In-Reply-To: <20210802.013859.2264148309496126056.masm@luna.pink.masm11.me> X-Mailer: Mew version 6.8 on Emacs 29.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.29 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:279414 Archived-At: 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 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