bug#35058: [PATCH] Use display-graphic-p in more cases

[Alex]

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?