Re: [elpa] externals/corfu a44c778 1/2: Yet another hack: posn-at-point computation returns wrong y-coordinate on Emacs 28

Re: [elpa] externals/corfu a44c778 1/2: Yet another hack: posn-at-point computation returns wrong y-coordinate on Emacs 28

[Stefan Monnier]

>     Yet another hack: posn-at-point computation returns wrong y-coordinate on Emacs 28
[...]
> -         (pos (posn-x-y (posn-at-point pos))))
> +         ;;; XXX HACK y-coordinate position computation is wrong
> +         ;;; if there exists a flymake underline overlay at that point.
> +         ;;; Take the y coordinate from the current point.
> +         (x (car (posn-x-y (posn-at-point pos))))
> +         (y (cdr (posn-x-y (posn-at-point (point))))))

I would generally recommend against adding workarounds for bugs in
development versions of Emacs (at least not before reporting the
bug and also not before finding out that the bug won't be fixed
promptly).  Also I'd recommend making it clear in the code via
comments and/or via version tests that this is a workaround for
a specific bug in a specific version.


        Stefan


PS: This is a general recommendation.  I don't know if you've filed
a bug report for it yet.

Re: [elpa] externals/corfu a44c778 1/2: Yet another hack: posn-at-point computation returns wrong y-coordinate on Emacs 28

[Daniel Mendler]

On 7/15/21 4:07 PM, Stefan Monnier wrote:
>>     Yet another hack: posn-at-point computation returns wrong y-coordinate on Emacs 28
> [...]
>> -         (pos (posn-x-y (posn-at-point pos))))
>> +         ;;; XXX HACK y-coordinate position computation is wrong
>> +         ;;; if there exists a flymake underline overlay at that point.
>> +         ;;; Take the y coordinate from the current point.
>> +         (x (car (posn-x-y (posn-at-point pos))))
>> +         (y (cdr (posn-x-y (posn-at-point (point))))))
> 
> I would generally recommend against adding workarounds for bugs in
> development versions of Emacs (at least not before reporting the
> bug and also not before finding out that the bug won't be fixed
> promptly).  Also I'd recommend making it clear in the code via
> comments and/or via version tests that this is a workaround for
> a specific bug in a specific version.

I agree with that. However I want to use the package now. There are
already many Emacs 28 users out there.

> PS: This is a general recommendation.  I don't know if you've filed
> a bug report for it yet.

Not yet, for now I am only collecting and marking hacks and workarounds
in the code. There are already many workarounds for bugs in child
frames. The same applies to the Posframe package.

I've just recently discussed the child frame issues with Tassilo
(cc'ed), who stumbled over another issue, where the window manager Sway
ignored the no-accept-focus option, because Wayland does not support it.
Therefore Corfu and Posframe have to work around it. All these hacks and
workarounds are better done directly in the Emacs child frame/display
code, or even better in Gtk (in the case of Emacs pgtk) instead of
affecting every package, which uses the child frame API.

Daniel

On the pains of using child-frames / posframes

[Tassilo Horn]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Not yet, for now I am only collecting and marking hacks and
> workarounds in the code. There are already many workarounds for bugs
> in child frames.  The same applies to the Posframe package.

Indeed, posframe and corfu share a quite a bunch of workarounds for
making child-frames working as intended.  In corfu.el those workarounds
are nicely annotated, so here is a summary:

  The `no-accept-focus' frame parameter doesn't work reliably, e.g., it
doesn't work for me in Sway (a wayland window manager).  I've reported
that as a sway bug, and a sway dev told me that this is probably a bug
in their Xwayland implementation.  But he also said that the GTK
function gtk_window_set_accept_focus used by emacs to indicate that a
frame shouldn't receive focus in gtk builds is a no-op for wayland, and
that wayland doesn't really have a concept of non-focusable windows.  So
no-accept-focus will never work when emacs is built as a native wayland
application, i.e., with the pgtk branch.

Corfu works around that by redirecting focus back to the parent frame
(posframe does the same), and ignoring mouse-clicks in the buffer shown
by the child-frame.

--8<---------------cut here---------------start------------->8---
(defun corfu--popup-redirect-focus ()
  "Redirect focus from popup."
  (redirect-frame-focus corfu--frame (frame-parent corfu--frame)))

    ;;; XXX HACK install redirect focus hook
    (add-hook 'pre-command-hook #'corfu--popup-redirect-focus nil 'local)

(defvar corfu--mouse-ignore-map
  (let ((map (make-sparse-keymap)))
    (dolist (k '(mouse-1 down-mouse-1 drag-mouse-1 double-mouse-1 triple-mouse-1
                 mouse-2 down-mouse-2 drag-mouse-2 double-mouse-2 triple-mouse-2
                 mouse-3 down-mouse-3 drag-mouse-3 double-mouse-3 triple-mouse-3
                 mouse-4 down-mouse-4 drag-mouse-4 double-mouse-4 triple-mouse-4
                 mouse-5 down-mouse-5 drag-mouse-5 double-mouse-5 triple-mouse-5
                 mouse-6 down-mouse-6 drag-mouse-6 double-mouse-6 triple-mouse-6
                 mouse-7 down-mouse-7 drag-mouse-7 double-mouse-7 triple-mouse-7))
      (define-key map (vector k) #'ignore))
    map)
  "Ignore all mouse clicks.")

      ;;; XXX HACK install mouse ignore map
      (use-local-map corfu--mouse-ignore-map)
--8<---------------cut here---------------end--------------->8---

  With GTK builds and Gnome / Cinnamon, one needs the following hack to
make the child-frame resize properly:

--8<---------------cut here---------------start------------->8---
  (let* (...
         (x-gtk-resize-child-frames
          (let ((case-fold-search t))
            (and
             ;; XXX HACK to fix resizing on gtk3/gnome taken from posframe.el
             ;; More information:
             ;; * https://github.com/minad/corfu/issues/17
             ;; * https://gitlab.gnome.org/GNOME/mutter/-/issues/840
             ;; * https://lists.gnu.org/archive/html/emacs-devel/2020-02/msg00001.html
             (string-match-p "gtk3" system-configuration-features)
             (string-match-p "gnome\\|cinnamon" (or (getenv "XDG_CURRENT_DESKTOP")
                                                    (getenv "DESKTOP_SESSION") ""))
             'resize-mode)))
--8<---------------cut here---------------end--------------->8---

  Some workarounds against flicker, and a case in which the order of
setting a face attribute and setting the frame parameters makes a
difference for no obvious reason.

--8<---------------cut here---------------start------------->8---
    ;; XXX HACK Setting the same frame-parameter/face-background is not a nop (BUG!).
    ;; Check explicitly before applying the setting.
    ;; Without the check, the frame flickers on Mac.
    ;; XXX HACK We have to apply the face background before adjusting the frame parameter,
    ;; otherwise the border is not updated (BUG!).
    (let* ((face (if (facep 'child-frame-border) 'child-frame-border 'internal-border))
	   (new (face-attribute 'corfu-border :background)))
      (unless (equal (face-attribute face :background corfu--frame) new)
	(set-face-background face new corfu--frame)))
    (let ((new (face-attribute 'corfu-background :background)))
      (unless (equal (frame-parameter corfu--frame 'background-color) new)
	(set-frame-parameter corfu--frame 'background-color new)))
--8<---------------cut here---------------end--------------->8---

  More flicker avoidance.

--8<---------------cut here---------------start------------->8---
    ;; XXX HACK Make the frame invisible before moving the popup from above to below the line in
    ;; order to avoid flicker.
    (unless (eq (< (cdr (frame-position corfu--frame)) yb) (< y yb))
      (make-frame-invisible corfu--frame))
    (set-frame-size corfu--frame width height t)
    (set-frame-position corfu--frame x y)
    (make-frame-visible corfu--frame)))
--8<---------------cut here---------------end--------------->8---

  It seems like some overlays can make `posn-at-point' return wrong
y-values.

--8<---------------cut here---------------start------------->8---
         ;;; XXX HACK On Emacs 28 y-coordinate position computation is wrong if
         ;;; there exists a flymake underline overlay at that point. Therefore
         ;;; compute the y-coordinate at the line beginning.
         (x (or (car (posn-x-y (posn-at-point pos))) 0))
         (y (save-excursion
              (goto-char pos)
              (beginning-of-line)
              (or (cdr (posn-x-y (posn-at-point))) 0))))
--8<---------------cut here---------------end--------------->8---

  Also, the number of frame parameters one needs to apply for a typical
child-frame whose intent is to display completions, tooltips, or inline
help is quite large and nothing which one can write from the top of
one's head.

--8<---------------cut here---------------start------------->8---
(defvar corfu--frame-parameters
  '((no-accept-focus . t)
    (no-focus-on-map . t)
    (min-width . t)
    (min-height . t)
    (width . 0)
    (height . 0)
    (border-width . 0)
    (child-frame-border-width . 1)
    (left-fringe . 0)
    (right-fringe . 0)
    (vertical-scroll-bars . nil)
    (horizontal-scroll-bars . nil)
    (menu-bar-lines . 0)
    (tool-bar-lines . 0)
    (tab-bar-lines . 0)
    (no-other-frame . t)
    (unsplittable . t)
    (undecorated . t)
    (cursor-type . nil)
    (minibuffer . nil)
    (visibility . nil)
    (no-special-glyphs . t)
    (desktop-dont-save . t))
  "Default child frame parameters.")
--8<---------------cut here---------------end--------------->8---

  Ditto for the buffer one's going to display in the child-frame.

--8<---------------cut here---------------start------------->8---
(defvar corfu--buffer-parameters
  '((mode-line-format . nil)
    (header-line-format . nil)
    (tab-line-format . nil)
    (frame-title-format . "")
    (truncate-lines . t)
    (cursor-in-non-selected-windows . nil)
    (cursor-type . nil)
    (show-trailing-whitespace . nil)
    (display-line-numbers . nil)
    (left-fringe-width . nil)
    (right-fringe-width . nil)
    (left-margin-width . 0)
    (right-margin-width . 0)
    (fringes-outside-margins . 0)
    (buffer-read-only . t))
  "Default child frame buffer parameters.")
--8<---------------cut here---------------end--------------->8---

This all looks to me as if child-frames should probably have some
dedicated API in vanilla emacs so that using them becomes a bit easier
and less verbose.  Also, that packages like corfu and posframe have to
duplicate each other's workarounds for several issues like the
unreliability of `no-accept-focus' doesn't seem right.  If there are
issues on certain supported platforms, those are better addressed in
emacs itself.

Bye,
Tassilo

Re: On the pains of using child-frames / posframes

[martin rudalics]

 >> Not yet, for now I am only collecting and marking hacks and
 >> workarounds in the code. There are already many workarounds for bugs
 >> in child frames.

Please don't call the below "bugs in child frames".  Most of them are
bugs in window managers in the sense that these are either not IICC or
Freedesktop compliant.  It's a sad fact that the only window manager
correctly implementing child windows in that sense is that of Microsoft
Windows.

 >    The `no-accept-focus' frame parameter doesn't work reliably, e.g., it
 > doesn't work for me in Sway (a wayland window manager).  I've reported
 > that as a sway bug, and a sway dev told me that this is probably a bug
 > in their Xwayland implementation.  But he also said that the GTK
 > function gtk_window_set_accept_focus used by emacs to indicate that a
 > frame shouldn't receive focus in gtk builds is a no-op for wayland, and
 > that wayland doesn't really have a concept of non-focusable windows.  So
 > no-accept-focus will never work when emacs is built as a native wayland
 > application, i.e., with the pgtk branch.

I suspect that with Wayland the 'no-focus-on-map' parameter doesn't work
either as intended and a workaround for that would be needed too.  Could
you provide a simple option to hook into x_set_no_accept_focus and
x_set_no_focus_on_map and implement a functionality similar to that of
`corfu--popup-redirect-focus' so that we can avoid hooking into
`pre-command-hook'?  Maybe we should also separate the focus redirection
from the mouse click ignore functionality.

Also, please keep in mind that with Wayland the only frames we may be
able to reliably position on screen ourselves are child frames.  So any
functionality we implement here, may eventually become a functionality
for our normal (non-parented) frames there.

 >    With GTK builds and Gnome / Cinnamon, one needs the following hack to
 > make the child-frame resize properly:

`x-gtk-resize-child-frames' is an awful hack so I would really prefer to
not enable it out of the box on the indicated systems but rather provide
an optional value for `x-gtk-resize-child-frames' that does implement
that.

 >    Some workarounds against flicker, and a case in which the order of
 > setting a face attribute and setting the frame parameters makes a
 > difference for no obvious reason.

Are these flicker problems ...

 >      ;; Without the check, the frame flickers on Mac.

... typical for Macs or have people seen them elsewhere too?

 >    More flicker avoidance.

Don't we get something similar with `x-gtk-resize-child-frames'?  So
maybe we should provide a more general option that simply makes a child
frame invisible whenever people resize or move it?

 >    It seems like some overlays can make `posn-at-point' return wrong
 > y-values.

Is this `posn-at-point' behavior child frame endemic or has it been seen
with normal frames too?

 >    Also, the number of frame parameters one needs to apply for a typical
 > child-frame whose intent is to display completions, tooltips, or inline
 > help is quite large and nothing which one can write from the top of
 > one's head.

If you can provide a good name for this ...

 > (defvar corfu--frame-parameters

... like ...-child-frame-alist, I see no problems.  I never tried with
'width' or 'height' 0, though ...

 >    Ditto for the buffer one's going to display in the child-frame.

Who would apply a generalized version of ...

 > (defvar corfu--buffer-parameters

... - set_window_buffer?

 > This all looks to me as if child-frames should probably have some
 > dedicated API in vanilla emacs so that using them becomes a bit easier
 > and less verbose.  Also, that packages like corfu and posframe have to
 > duplicate each other's workarounds for several issues like the
 > unreliability of `no-accept-focus' doesn't seem right.  If there are
 > issues on certain supported platforms, those are better addressed in
 > emacs itself.

Please try to set up the above as you and Daniel see fit.  I hardly can
implement anything in this area - setting up a Gnome environment for
testing `x-gtk-resize-child-frames' was enough of a nightmare back then.

Thanks in advance, martin

Re: On the pains of using child-frames / posframes

[Tassilo Horn]

Hi Martin,

thanks for the detailed answer.  Daniel is away for some days and then
I'm away for two weeks, so I've proven again to be a specialist in
inadequate timing.  Don't hold your breath but I'm pretty sure we (and
especially Daniel) will pick up the discussion in the nearer future and
come up with a proposal.

Thanks again,
Tassilo

Re: On the pains of using child-frames / posframes

[martin rudalics]

 > Daniel is away for some days and then
 > I'm away for two weeks, so I've proven again to be a specialist in
 > inadequate timing.  Don't hold your breath but I'm pretty sure we (and
 > especially Daniel) will pick up the discussion in the nearer future and
 > come up with a proposal.

Just to elaborate on two aspects:

(1) Any fix we come up with for the `no-accept-focus' misbehavior should
     fix any misbehavior of this for normal top-level frames too and also
     handle any misbehavior of `no-focus-on-map'.  I suppose we should do
     this in handle_one_xevent and/or x_detect_focus_change in xterm.c.

(2) The `posn-at-point' problem with overlays is very likely a separate
     issue and should be reported and fixed separately.

martin