bug#43379: [PATCH] Double-click events can occur without preceding single-click events

[Daniel Koning]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Koning <dk@danielkoning.com>
>> Date: Sun, 13 Sep 2020 12:00:10 -0500
>>
>> ++++
>> +** Repeat events are now produced only when the modifier keys are the same.
>
> What will this do if the modifier key is "simulated" using "C-x @"?

As before, you can't get a repeat tag on an event with a simulated
modifier key, because the raw keyboard events representing "C-x @" break
the chain, and double and triple modifiers get set before a keymap can
see events and reinterpret them. Kind of a shame, but better than
getting spurious tags. I guess you could change simple.el to do
something akin to what xterm-mouse-mode does, but it doesn't strike me
as necessary.

"C-x @ m mouse-1 mouse-1" *will* call the commands for <M-mouse-1> and
<double-mouse-1>. As with the case of clicks in different windows, even
though my patch doesn't concern itself with this, I do think it's a
flaw worth fixing at some point.

> More generally, for a change this deep and wide, I'd like to
> understand better what exactly are the problems being solved,

That the program's actual behavior doesn't match the documentation,
which describes behavior that reflects how double clicks work on every
other GUI I've ever used.

The docs could just be rewritten to match the program, but imagine what
they'd look like: "Design the command binding of the double click event
to assume that the single-click command has already run, or at least
*some* single-click command using the same mouse button but potentially
different modifier keys, or if not that, then a single-click command for
a different mouse button which is now being held down but hasn't been
held down for as long as this button has. But one of those for sure!"

> and also how can we be sure (by testing or otherwise) we are not
> getting regressions in some use cases. Could you please clarify that?

Unfortunately, I don't think it's possible to add automated regression
tests for this stuff, at least not with ERT. You'd need to have some
programmatic way of generating events from Lisp that get fed into the
src/keyboard.c functions. Without serious additions to the underlying C
layer (or maybe a module?), I don't see how that could happen. And in
general, I think that's a sensible limitation; what if event X got
mapped to a Lisp command which generated a fake event X, which generated
another etc. etc.?

Here's how I've been testing it manually: feed Emacs (patched and
unpatched) as many combinations of mouse actions as I can think up, then
check the lossage file to see if it's interpreting them right. Or better
yet, visit *scratch* in a split frame with another buffer called
*Events*, and eval this form:

    (cl-loop
     (let ((event (read-event)))
       (with-current-buffer "*Events*"
         (insert (prin1-to-string event) "\n\n")
         (goto-char (point-min))))))

You'll see the full (pre-keymap, pre-input-method) event representation
appear immediately for everything you do. Bind 'double-click-time' and
'double-click-fuzz' around it as you please for more test cases.

(If you click around in the continually updating *Events* window, you'll
get a lot of drags, because the text is moving under the cursor between
down and release. This is not new, and a preexisting comment says it's
by design.)

There's no way around the fact that it needs line-by-line scrutiny. But
I went to great lengths to make it lucid and direct so that it won't be
onerous to give it that scrutiny.

> Beyond theoretical (un)cleanliness, what other practical problems did
> you find with the current code and fix in these patches?

I didn't change the code in order to make it cleaner; if I'd needed to
make it dirtier in order to fix the bug, that's what I would have done.
It just happened that the most straightforward approach to fixing it
meant making it cleaner, which I think is nice. (Modifying the code in
place is not straightforward, since the pertinent sections of the
switch/case labyrinth -- which are almost but not quite identical --
inevitably end up having to interact with each other when the more
stringent criteria come into play. File-level static variables start
multiplying out of control at that point.)

As for patch #1, the big one, I explained what it addresses above:
Emacs was doing the wrong thing both according to its documentation and
according to my intuition. Now it does the right thing for modifier keys
and interleaved clicks, and (I contend) it does everything else the same
as before.

Here's what the other five patches address, in order:

2/6 mentions an existing limitation on 'double-click-fuzz' in that
variable's manual entry.

3/6 changes some expressions to use the POSN_WINDOW and POSN_POSN
macros that are defined to expand to the same things. (Not very
important, but it makes the expressions readable instead of opaque.)

4/6 closes a weird loophole in the detection of drag events, surely
unintended. See discussion below.

5/6 fixes the Cocoa bug where trackpad swipes got retroactively modified
by keys that weren't pressed yet when the user swiped.

6/6 is a small elaboration on an existing FIXME comment to explain why
the issue is hard to fix. (Also not too important.)

All the patches are self-contained. They can be applied in any
combination.

>
>>  This variable is also the threshold for motion of the mouse to count
>> -as a drag.
>> +as a drag.  (But if the mouse moves from one screen position to
>> +another while the button is held down, it always counts as a drag, no
>> +matter the value of @code{double-click-fuzz}.)
>
> Isn't this an incompatible change?

It's not a change; it already worked that way. And it makes sense that
it works that way -- otherwise, if you had 'double-click-fuzz' set too
high, you couldn't drag to select a region of two adjacent characters.
But the exception to the rule isn't obvious without looking at the code.

My related change (4/6) was to look at windows per se as well as window
positions. Before, a drag starting at position 1 of one window, and
ending at position 1 of another window, wasn't tagged as a drag because
the position values were the same. I think this one should be
uncontroversial (as should the trackpad thing in src/nsterm.m, 5/6).

Daniel