[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#35058: [PATCH] Use display-graphic-p in more cases
|
From: |
Eli Zaretskii |
|
Subject: |
bug#35058: [PATCH] Use display-graphic-p in more cases |
|
Date: |
Sun, 31 Mar 2019 18:37:57 +0300 |
> From: Alex <agrambot@gmail.com>
> Date: Sat, 30 Mar 2019 17:38:16 -0600
>
> >From b760955517f9c39b13353bdf5ae49f25fbadc66f Mon Sep 17 00:00:00 2001
> From: Alexander Gramiak <agrambot@gmail.com>
> Date: Sat, 30 Mar 2019 16:53:11 -0600
> Subject: [PATCH] Use display-graphic-p in more cases
Thanks for working on this. In general, most places where we don't
use display-graphic-p do that for a reason, see the comments below.
The general idea of display-graphic-p and other similar predicates and
low-level APIs is that they hide these details from the callers, and
their names explain themselves. This is why we have several
predicates whose implementation is almost identical: each one of them
is supposed to provide a test for a different kind of functionalities,
even if currently the same frame types return non-nil from all of
these predicates. The point is to allow easier changes in the future
when additional frame types acquire capabilities they don't have
today. We already had such episodes with the mouse and menus.
But some of the places you found can indeed use display-graphic-p
nowadays.
> diff --git a/lisp/disp-table.el b/lisp/disp-table.el
> index 476c0cb986..a3e8892c51 100644
> --- a/lisp/disp-table.el
> +++ b/lisp/disp-table.el
> @@ -176,7 +176,7 @@ standard-display-g1
> "Display character C as character SC in the g1 character set.
> This function assumes that your terminal uses the SO/SI characters;
> it is meaningless for an X frame."
> - (if (memq window-system '(x w32 ns))
> + (if (display-graphic-p)
> (error "Cannot use string glyphs in a windowing system"))
> (or standard-display-table
> (setq standard-display-table (make-display-table)))
> @@ -188,7 +188,7 @@ standard-display-graphic
> "Display character C as character GC in graphics character set.
> This function assumes VT100-compatible escapes; it is meaningless for an
> X frame."
> - (if (memq window-system '(x w32 ns))
> + (if (display-graphic-p)
> (error "Cannot use string glyphs in a windowing system"))
> (or standard-display-table
> (setq standard-display-table (make-display-table)))
> @@ -276,7 +276,7 @@ standard-display-european
> (progn
> (standard-display-default
> (unibyte-char-to-multibyte 160) (unibyte-char-to-multibyte 255))
> - (unless (or (memq window-system '(x w32 ns)))
> + (unless (display-graphic-p)
> (and (terminal-coding-system)
> (set-terminal-coding-system nil))))
>
> @@ -289,7 +289,7 @@ standard-display-european
> ;; unless some other has been specified.
> (if (equal current-language-environment "English")
> (set-language-environment "latin-1"))
> - (unless (or noninteractive (memq window-system '(x w32 ns)))
> + (unless (or noninteractive (display-graphic-p))
> ;; Send those codes literally to a character-based terminal.
> ;; If we are using single-byte characters,
> ;; it doesn't matter which coding system we use.
This part is fine by me.
> diff --git a/lisp/emulation/cua-base.el b/lisp/emulation/cua-base.el
> index 302ef12386..461c4ea9aa 100644
> --- a/lisp/emulation/cua-base.el
> +++ b/lisp/emulation/cua-base.el
> @@ -1238,7 +1238,7 @@ cua--init-keymaps
> ;; Cache actual rectangle modifier key.
> (setq cua--rectangle-modifier-key
> (if (and cua-rectangle-modifier-key
> - (memq window-system '(x)))
> + (eq window-system 'x))
> cua-rectangle-modifier-key
> 'meta))
> ;; C-return always toggles rectangle mark
Not sure about this one: what does a modifier key have to do with GUI
display?
> diff --git a/lisp/faces.el b/lisp/faces.el
> index ab6c384c80..fa526c3506 100644
> --- a/lisp/faces.el
> +++ b/lisp/faces.el
> @@ -55,6 +55,7 @@ term-file-aliases
> :group 'terminals
> :version "25.1")
>
> +(declare-function display-graphic-p "frame" (&optional display))
> (declare-function xw-defined-colors "term/common-win" (&optional frame))
>
> (defvar help-xref-stack-item)
> @@ -1239,7 +1240,7 @@ read-face-attribute
> ;; explicitly in VALID, using color approximation code
> ;; in tty-colors.el.
> (when (and (memq attribute '(:foreground :background))
> - (not (memq (window-system frame) '(x w32 ns)))
> + (not (display-graphic-p frame))
> (not (member new-value
> '("unspecified"
> "unspecified-fg" "unspecified-bg"))))
> @@ -1833,7 +1834,7 @@ defined-colors
> The value may be different for frames on different display types.
> If FRAME doesn't support colors, the value is nil.
> If FRAME is nil, that stands for the selected frame."
> - (if (memq (framep (or frame (selected-frame))) '(x w32 ns))
> + (if (display-graphic-p frame)
> (xw-defined-colors frame)
> (mapcar 'car (tty-color-alist frame))))
> (defalias 'x-defined-colors 'defined-colors)
> @@ -1877,7 +1878,7 @@ color-defined-p
>
> If FRAME is omitted or nil, use the selected frame."
> (unless (member color '(unspecified "unspecified-bg" "unspecified-fg"))
> - (if (member (framep (or frame (selected-frame))) '(x w32 ns))
> + (if (display-graphic-p frame)
> (xw-color-defined-p color frame)
> (numberp (tty-color-translate color frame)))))
> (defalias 'x-color-defined-p 'color-defined-p)
> @@ -1903,7 +1904,7 @@ color-values
> (cond
> ((member color '(unspecified "unspecified-fg" "unspecified-bg"))
> nil)
> - ((memq (framep (or frame (selected-frame))) '(x w32 ns))
> + ((display-graphic-p frame)
> (xw-color-values color frame))
> (t
> (tty-color-values color frame))))
> @@ -1917,7 +1918,7 @@ display-color-p
> The optional argument DISPLAY specifies which display to ask about.
> DISPLAY should be either a frame or a display name (a string).
> If omitted or nil, that stands for the selected frame's display."
> - (if (memq (framep-on-display display) '(x w32 ns))
> + (if (display-graphic-p display)
> (xw-display-color-p display)
> (tty-display-color-p display)))
> (defalias 'x-display-color-p 'display-color-p)
> @@ -1928,12 +1929,9 @@ display-grayscale-p
> "Return non-nil if frames on DISPLAY can display shades of gray.
> DISPLAY should be either a frame or a display name (a string).
> If omitted or nil, that stands for the selected frame's display."
> - (let ((frame-type (framep-on-display display)))
> - (cond
> - ((memq frame-type '(x w32 ns))
> - (x-display-grayscale-p display))
> - (t
> - (> (tty-color-gray-shades display) 2)))))
> + (if (display-graphic-p display)
> + (x-display-grayscale-p display)
> + (> (tty-color-gray-shades display) 2)))
>
> (defun read-color (&optional prompt convert-to-RGB allow-empty-name msg)
> "Read a color name or RGB triplet.
These are okay, I think.
> diff --git a/lisp/frame.el b/lisp/frame.el
> index 6cb1247372..f5ad3152a0 100644
> --- a/lisp/frame.el
> +++ b/lisp/frame.el
> @@ -974,7 +974,7 @@ select-frame-set-input-focus
> (select-frame frame norecord)
> (raise-frame frame)
> ;; Ensure, if possible, that FRAME gets input focus.
> - (when (memq (window-system frame) '(x w32 ns))
> + (when (display-graphic-p frame)
> (x-focus-frame frame))
I don't see what display being GUI have to do with frame focus
redirection.
> @@ -1027,11 +1027,11 @@ suspend-frame
> "Do whatever is right to suspend the current frame.
> Calls `suspend-emacs' if invoked from the controlling tty device,
> `suspend-tty' from a secondary tty device, and
> -`iconify-or-deiconify-frame' from an X frame."
> +`iconify-or-deiconify-frame' from a graphical frame."
> (interactive)
> (let ((type (framep (selected-frame))))
> (cond
> - ((memq type '(x ns w32)) (iconify-or-deiconify-frame))
> + ((display-graphic-p display) (iconify-or-deiconify-frame))
> ((eq type t)
> (if (controlling-tty-p)
> (suspend-emacs)
Likewise here: there's no reason apriori for any logical relation to
exist between GUI display and iconifying/deiconifying a frame.
> @@ -1895,7 +1895,7 @@ display-graphic-p
> that use a window system such as X, and false for text-only terminals.
> DISPLAY can be a display name, a frame, or nil (meaning the selected
> frame's display)."
> - (not (null (memq (framep-on-display display) '(x w32 ns)))))
> + (not (memq (framep-on-display display) '(nil t pc))))
I prefer the current variant, as it shows the frame types explicitly.
Doing that for frames that are NOT something means people will have
problems looking up these dependencies when they need to.
> (defun display-images-p (&optional display)
> "Return non-nil if DISPLAY can display images.
> @@ -1933,12 +1933,9 @@ display-screens
> "Return the number of screens associated with DISPLAY.
> DISPLAY should be either a frame or a display name (a string).
> If DISPLAY is omitted or nil, it defaults to the selected frame's display."
> - (let ((frame-type (framep-on-display display)))
> - (cond
> - ((memq frame-type '(x w32 ns))
> - (x-display-screens display))
> - (t
> - 1))))
> + (if (display-graphic-p display)
> + (x-display-screens display)
> + 1))
>
> (declare-function x-display-pixel-height "xfns.c" (&optional terminal))
>
> @@ -1953,12 +1950,9 @@ display-pixel-height
> refers to the pixel height for all physical monitors associated
> with DISPLAY. To get information for each physical monitor, use
> `display-monitor-attributes-list'."
> - (let ((frame-type (framep-on-display display)))
> - (cond
> - ((memq frame-type '(x w32 ns))
> - (x-display-pixel-height display))
> - (t
> - (frame-height (if (framep display) display (selected-frame)))))))
> + (if (display-graphic-p display)
> + (x-display-pixel-height display)
> + (frame-height (if (framep display) display (selected-frame)))))
>
> (declare-function x-display-pixel-width "xfns.c" (&optional terminal))
>
> @@ -1973,12 +1967,9 @@ display-pixel-width
> refers to the pixel width for all physical monitors associated
> with DISPLAY. To get information for each physical monitor, use
> `display-monitor-attributes-list'."
> - (let ((frame-type (framep-on-display display)))
> - (cond
> - ((memq frame-type '(x w32 ns))
> - (x-display-pixel-width display))
> - (t
> - (frame-width (if (framep display) display (selected-frame)))))))
> + (if (display-graphic-p display)
> + (x-display-pixel-width display)
> + (frame-width (if (framep display) display (selected-frame)))))
>
> (defcustom display-mm-dimensions-alist nil
> "Alist for specifying screen dimensions in millimeters.
> @@ -2013,7 +2004,7 @@ display-mm-height
> refers to the height in millimeters for all physical monitors
> associated with DISPLAY. To get information for each physical
> monitor, use `display-monitor-attributes-list'."
> - (and (memq (framep-on-display display) '(x w32 ns))
> + (and (display-graphic-p display)
> (or (cddr (assoc (or display (frame-parameter nil 'display))
> display-mm-dimensions-alist))
> (cddr (assoc t display-mm-dimensions-alist))
> @@ -2034,7 +2025,7 @@ display-mm-width
> refers to the width in millimeters for all physical monitors
> associated with DISPLAY. To get information for each physical
> monitor, use `display-monitor-attributes-list'."
> - (and (memq (framep-on-display display) '(x w32 ns))
> + (and (display-graphic-p display)
> (or (cadr (assoc (or display (frame-parameter nil 'display))
> display-mm-dimensions-alist))
> (cadr (assoc t display-mm-dimensions-alist))
> @@ -2078,12 +2069,12 @@ display-planes
> If DISPLAY is omitted or nil, it defaults to the selected frame's display."
> (let ((frame-type (framep-on-display display)))
> (cond
> - ((memq frame-type '(x w32 ns))
> - (x-display-planes display))
> ((eq frame-type 'pc)
> 4)
> + ((memq frame-type '(nil t))
> + (truncate (log (length (tty-color-alist)) 2)))
> (t
> - (truncate (log (length (tty-color-alist)) 2))))))
> + (x-display-planes display)))))
>
> (declare-function x-display-color-cells "xfns.c" (&optional terminal))
>
> @@ -2093,12 +2084,12 @@ display-color-cells
> If DISPLAY is omitted or nil, it defaults to the selected frame's display."
> (let ((frame-type (framep-on-display display)))
> (cond
> - ((memq frame-type '(x w32 ns))
> - (x-display-color-cells display))
> ((eq frame-type 'pc)
> 16)
> + ((memq frame-type '(nil t))
> + (tty-display-color-cells display))
> (t
> - (tty-display-color-cells display)))))
> + (x-display-color-cells display)))))
>
> (declare-function x-display-visual-class "xfns.c" (&optional terminal))
>
> @@ -2110,13 +2101,13 @@ display-visual-class
> If DISPLAY is omitted or nil, it defaults to the selected frame's display."
> (let ((frame-type (framep-on-display display)))
> (cond
> - ((memq frame-type '(x w32 ns))
> - (x-display-visual-class display))
> + ((not frame-type)
> + 'static-gray)
> ((and (memq frame-type '(pc t))
> (tty-display-color-p display))
> 'static-color)
> (t
> - 'static-gray))))
> + (x-display-visual-class display)))))
>
> (declare-function x-display-monitor-attributes-list "xfns.c"
> (&optional terminal))
I prefer not to change these. These APIs are the lowest level of
testing for the respective features, so I prefer them to show
explicitly on what types of frames we support them and how. Adding
indirection through display-graphic-p doesn't help here, because these
interfaces are siblings of display-graphic-p.
> @@ -2546,7 +2537,7 @@ blink-cursor-mode
> :init-value (not (or noninteractive
> no-blinking-cursor
> (eq system-type 'ms-dos)
> - (not (memq window-system '(x w32 ns)))))
> + (not (display-graphic-p))))
Why would we want to connect blinking cursor with GUI displays? These
two are unrelated.
> diff --git a/lisp/info.el b/lisp/info.el
> index f2a064abb6..4954916969 100644
> --- a/lisp/info.el
> +++ b/lisp/info.el
> @@ -4768,7 +4768,7 @@ Info-fontify-node
> ;; This is a serious problem for trying to handle multiple
> ;; frame types at once. We want this text to be invisible
> ;; on frames that can display the font above.
> - (when (memq (framep (selected-frame)) '(x pc w32 ns))
> + (unless (memq (framep (selected-frame)) '(nil t))
> (add-text-properties (1- (match-beginning 2)) (match-end 2)
> '(invisible t front-sticky nil
> rear-nonsticky t))))))
Not sure here, but if this about fonts, then display-multi-font-p is a
better test.
> diff --git a/lisp/simple.el b/lisp/simple.el
> index f76f31ad14..6651ee2fc5 100644
> --- a/lisp/simple.el
> +++ b/lisp/simple.el
> @@ -8682,7 +8682,7 @@ normal-erase-is-backspace-setup-frame
> (and (not noninteractive)
> (or (memq system-type '(ms-dos windows-nt))
> (memq window-system '(w32 ns))
> - (and (memq window-system '(x))
> + (and (eq window-system 'x)
> (fboundp 'x-backspace-delete-keys-p)
> (x-backspace-delete-keys-p))
> ;; If the terminal Emacs is running on has erase char
> @@ -8728,7 +8728,7 @@ normal-erase-is-backspace-mode
> (let ((enabled (eq 1 (terminal-parameter
> nil 'normal-erase-is-backspace))))
>
> - (cond ((or (memq window-system '(x w32 ns pc))
> + (cond ((or window-system
> (memq system-type '(ms-dos windows-nt)))
> (let ((bindings
> '(([M-delete] [M-backspace])
normal-erase-is-backspace-mode is entirely unrelated to display being
GUI, so I don't think this is right.
> diff --git a/lisp/window.el b/lisp/window.el
> index b769be0633..b622debd51 100644
> --- a/lisp/window.el
> +++ b/lisp/window.el
> @@ -9351,7 +9351,7 @@ handle-select-window
> ;; we might get two windows with an active cursor.
> (select-window window)
> (cond
> - ((or (not (memq (window-system frame) '(x w32 ns)))
> + ((or (memq (window-system frame) '(nil t pc))
> (not focus-follows-mouse)
> ;; Focus FRAME if it's either a child frame or an ancestor
> ;; of the frame switched from.
This is again a reversion of logic which I think is a change for the
worse.
Thanks.