[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
bug#35058: [PATCH] Use display-graphic-p in more cases
From: |
Alex |
Subject: |
bug#35058: [PATCH] Use display-graphic-p in more cases |
Date: |
Sun, 31 Mar 2019 22:15:35 -0600 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) |
Eli Zaretskii <eliz@gnu.org> writes:
>> 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?
I wasn't sure either (I merely noticed the useless memq), but I just
checked the documentation of cua-rectangle-modifier-key, which states:
On non-window systems, always use the meta modifier.
So I changed the eq call to display-graphics-p. Is it okay to follow the
docstring here?
>> 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.
The x-focus-frame here is the GUI-specific code related to frame focus
that calls x_focus_frame, which is only defined when HAVE_WINDOW_SYSTEM
is defined. This is one of the procedures that I'm planning to rename
with a gui prefix, so I believe display-graphic-p makes sense here.
>> @@ -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.
What would (de)iconifying be in non-GUI displays?
If there is indeed no logical relation, then what about a new
display-iconifiable-p that is made an alias?
>> @@ -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.
I'm not sure how much of a problem that would be if the return values of
framep-on-display were listed in its docstring, but I guess it's not
really an issue leaving this one alone.
>> (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.
If all graphical displays support these features currently, and if it's
reasonable to presume that any new graphical display types would also
support them, then I don't see why it would be a problem to use
display-graphics-p in these cases. If the unexpected occurs, then there
would only be a few definitions to change at most.
For example, is it really wrong to presume that all graphical displays
can retrieve pixel height/width?
>> @@ -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.
The definition of blink-cursor mode states:
This command is effective only on graphical frames. On text-only
terminals, cursor blinking is controlled by the terminal."
So it seems reasonable to use display-graphic-p here.
>> 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.
Is multi-font equivalent to supporting invisible text? If so, then I'll
use that and (or ... (eq window-system 'pc)).
>> 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.
The docstring of normal-erase-is-backspace-mode mentions window-system
several times, so I don't see the issue.
>> 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.
Would it be okay to forward-declare display-graphic-p and use it here as
well?
- bug#35058: [PATCH] Use display-graphic-p in more cases,
Alex <=
- bug#35058: [PATCH] Use display-graphic-p in more cases, Eli Zaretskii, 2019/04/01
- bug#35058: [PATCH] Use display-graphic-p in more cases, Drew Adams, 2019/04/01
- bug#35058: [PATCH] Use display-graphic-p in more cases, Alex, 2019/04/02
- bug#35058: [PATCH] Use display-graphic-p in more cases, Eli Zaretskii, 2019/04/02
- bug#35058: [PATCH] Use display-graphic-p in more cases, Alex, 2019/04/02
- bug#35058: [PATCH] Use display-graphic-p in more cases, Eli Zaretskii, 2019/04/02
- bug#35058: [PATCH] Use display-graphic-p in more cases, Alex, 2019/04/03
- bug#35058: [PATCH] Use display-graphic-p in more cases, Eli Zaretskii, 2019/04/03
- bug#35058: [PATCH] Use display-graphic-p in more cases, Alex, 2019/04/03