[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Merging the pgtk branch
|
From: |
Yuuki Harano |
|
Subject: |
Re: Merging the pgtk branch |
|
Date: |
Sun, 14 Nov 2021 21:20:10 +0900 (JST) |
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
- Re: Merging the pgtk branch,
Yuuki Harano <=