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

[Eli Zaretskii]

> From: Alex <agrambot@gmail.com>
> Cc: 35058@debbugs.gnu.org
> Date: Sun, 31 Mar 2019 22:15:35 -0600
> 
> >>    (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?

No, not IMO.  Doc strings can change because their purpose is
documentation that's useful to users and programmers, whereas the
issue in hand is ease of future maintenance.  Let me tell more about
this.

These functions were written when TTY frames were made to support
faces (colors) during Emacs 21 development.  When we started using
Emacs 21 on TTYs, we found out that various color-related features
didn't work since they were conditioned by window-system values,
because someone assumed that only GUI frames can ever support colors.
The easy solution was to remove the condition, but then it turned out
that many other places had code conditioned on window-system which had
nothing to do with colors or faces.  So a simple removal was not
possible; moreover, careful auditing of each and every use of
window-system was needed to decide, on a per-case basis, which was and
which wasn't related to faces, something that wasn't always obvious,
because some conditions were added ad-hoc, to prevent code from
calling functions undefined in a build --without-x.

We then decided to factor all uses of window-system into several
classes and replace the uses of window-system by the proper predicates
whose _names_ will clearly tell what is the feature being tested.
That's when these display-* functions were written, and that's why the
use of window-system (the variable) was deprecated.

I would like to keep using these functions according to the above
logic, so that when we add support for new capabilities on non-GUI
frames, we will not need to hunt for tricky conditions all over the
code, not again.

However, as you found out, some of the places still use window-system.
At the time, they were left alone, mostly because it sounded gross to
invent additional display-* functions which would have only one or two
callers.  We could do this now, if we consider it important to get rid
of more uses of window-system or framep; I still think it would be
gross, but won't object if others think otherwise.  But let's not
blindly use display-graphics-p to mean "anything that currently only
happens on a GUI frame", because that's not what it stands for.  It
stands for features that can _only_ be ever true on GUI displays, and
can _never_ be implemented on any text-mode frame, and only for
features for which there's no other display-* function that is more
focused, like display-multi-font-p.

> >> 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.

See above: I could envision a feature that moves focus to a TTY frame
as well, some day.

> >> @@ -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?

The display (i.e. the terminal) could be a GUI one, but a frame it
shows could be a TTY frame, for example if you use a terminal emulator
such as xterm.  We all do that all the time.

> If there is indeed no logical relation, then what about a new
> display-iconifiable-p that is made an alias?

As I said above, I consider that slightly gross, but I won't object to
introducing such functions.

> > 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.

I hope you now understand my motivation better.  IME, deciphering
those "few" definitions is not really trivial, especially when they
are used slightly inconsistently through the sources.  Making the
conditions explicit goes a small step in the direction of making that
job easier.

> 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."

That's the _current_ situation.  But what would preclude the xterm
developers, say, from adding a way of controlling that?  Nothing in
particular, I'd say.

> >>              ;; 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?

The comment above talks about fonts; I didn't read the code well
enough to understand what it's about.  Maybe display-multi-font-p is a
better predicate here, not sure.

> > 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.

I hope now you understand my motivation better.

> >> 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?

Not IMO, because this is again about selection with a mouse, something
that can (and is) supported on some TTY frames.  We could use the
hypothetical display-iconifiable-p (but we should then find a better
name, something about selecting/raising frames perhaps).

Thanks.