[PATCH] Let input queue deal gracefully with up-events

[PATCH] Let input queue deal gracefully with up-events

[David Kastrup]

* keyboard.c (apply_modifiers_uncached, parse_solitary_modifier)
(parse_modifiers_uncached): React gracefully to "up-" modifiers:
those may easily be injected by user-level Lisp code.
(read_key_sequence): Discard unbound up-events like unbound
down-events: they are even more likely only relevant for special
purposes.

While Emacs will not produce up-events on its own currently (those are
converted to drag or click events before being converted to
Lisp-readable structures), the input queue can be made to contain them
by synthesizing events to `unread-command-events'.  This patch makes
Emacs deal consistently with such events.
---
 src/ChangeLog  |  9 +++++++++
 src/keyboard.c | 31 ++++++++++++++++++++++---------
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 1df4f6a..f9f53c4 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,12 @@
+2015-01-28  David Kastrup  <dak@gnu.org>
+
+	* keyboard.c (apply_modifiers_uncached, parse_solitary_modifier)
+	(parse_modifiers_uncached): React gracefully to "up-" modifiers:
+	those may easily be injected by user-level Lisp code.
+	(read_key_sequence): Discard unbound up-events like unbound
+	down-events: they are even more likely only relevant for special
+	purposes.
+
 2015-01-27  Eli Zaretskii  <eliz@gnu.org>
 
 	* dired.c (directory_files_internal) [WINDOWSNT]: If readdir
diff --git a/src/keyboard.c b/src/keyboard.c
index 383c109..4b6dc86 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -6202,6 +6202,10 @@ parse_modifiers_uncached (Lisp_Object symbol, ptrdiff_t *modifier_end)
 	case 't':
 	  MULTI_LETTER_MOD (triple_modifier, "triple", 6);
 	  break;
+
+	case 'u':
+	  MULTI_LETTER_MOD (up_modifier, "up", 2);
+	  break;
 #undef MULTI_LETTER_MOD
 
 	}
@@ -6249,16 +6253,18 @@ apply_modifiers_uncached (int modifiers, char *base, int base_len, int base_len_
   /* Since BASE could contain nulls, we can't use intern here; we have
      to use Fintern, which expects a genuine Lisp_String, and keeps a
      reference to it.  */
-  char new_mods[sizeof "A-C-H-M-S-s-down-drag-double-triple-"];
+  char new_mods[sizeof "A-C-H-M-S-s-up-down-drag-double-triple-"];
   int mod_len;
 
   {
     char *p = new_mods;
 
-    /* Only the event queue may use the `up' modifier; it should always
-       be turned into a click or drag event before presented to lisp code.  */
-    if (modifiers & up_modifier)
-      emacs_abort ();
+    /* Mouse events should not exhibit the `up' modifier; it should
+       always be turned into a click or drag event before presented to
+       lisp code.  And there should not be more than one of
+       up/down/click/drag anyway.  But since Lisp events can be
+       synthesized, we don't take exception to unexpected
+       combinations */
 
     if (modifiers & alt_modifier)   { *p++ = 'A'; *p++ = '-'; }
     if (modifiers & ctrl_modifier)  { *p++ = 'C'; *p++ = '-'; }
@@ -6268,6 +6274,7 @@ apply_modifiers_uncached (int modifiers, char *base, int base_len, int base_len_
     if (modifiers & super_modifier) { *p++ = 's'; *p++ = '-'; }
     if (modifiers & double_modifier) p = stpcpy (p, "double-");
     if (modifiers & triple_modifier) p = stpcpy (p, "triple-");
+    if (modifiers & up_modifier) p = stpcpy (p, "up-");
     if (modifiers & down_modifier) p = stpcpy (p, "down-");
     if (modifiers & drag_modifier) p = stpcpy (p, "drag-");
     /* The click modifier is denoted by the absence of other modifiers.  */
@@ -6734,6 +6741,10 @@ parse_solitary_modifier (Lisp_Object symbol)
       MULTI_LETTER_MOD (triple_modifier, "triple", 6);
       break;
 
+    case 'u':
+      MULTI_LETTER_MOD (up_modifier, "up", 2);
+      break;
+
 #undef SINGLE_LETTER_MOD
 #undef MULTI_LETTER_MOD
     }
@@ -9441,14 +9452,16 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
 		   Drags reduce to clicks.
 		   Double-clicks reduce to clicks.
 		   Triple-clicks reduce to double-clicks, then to clicks.
-		   Down-clicks are eliminated.
+		   Up/Down-clicks are eliminated.
 		   Double-downs reduce to downs, then are eliminated.
 		   Triple-downs reduce to double-downs, then to downs,
 		     then are eliminated.  */
-	      if (modifiers & (down_modifier | drag_modifier
+	      if (modifiers & (up_modifier | down_modifier
+			       | drag_modifier
 			       | double_modifier | triple_modifier))
 		{
-		  while (modifiers & (down_modifier | drag_modifier
+		  while (modifiers & (up_modifier | down_modifier
+				      | drag_modifier
 				      | double_modifier | triple_modifier))
 		    {
 		      Lisp_Object new_head, new_click;
@@ -9460,7 +9473,7 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
 			modifiers &= ~drag_modifier;
 		      else
 			{
-			  /* Dispose of this `down' event by simply jumping
+			  /* Dispose of this `up/down' event by simply jumping
 			     back to replay_key, to get another event.
 
 			     Note that if this event came from mock input,
-- 
2.1.0

Re: [PATCH] Let input queue deal gracefully with up-events

[Alan Mackenzie]

Hello, David.

There's a solecism in your patch.  (To be fair, it was also in the
original).  Please insert the word "being" at the place shown, between
"before" and "presented".

Thanks!

On Wed, Jan 28, 2015 at 02:31:23PM +0100, David Kastrup wrote:

>  	}
> @@ -6249,16 +6253,18 @@ apply_modifiers_uncached (int modifiers, char *base, int base_len, int base_len_
>    /* Since BASE could contain nulls, we can't use intern here; we have
>       to use Fintern, which expects a genuine Lisp_String, and keeps a
>       reference to it.  */
> -  char new_mods[sizeof "A-C-H-M-S-s-down-drag-double-triple-"];
> +  char new_mods[sizeof "A-C-H-M-S-s-up-down-drag-double-triple-"];
>    int mod_len;

>    {
>      char *p = new_mods;

> -    /* Only the event queue may use the `up' modifier; it should always
> -       be turned into a click or drag event before presented to lisp code.  */
> -    if (modifiers & up_modifier)
> -      emacs_abort ();
> +    /* Mouse events should not exhibit the `up' modifier; it should
> +       always be turned into a click or drag event before presented to
                                                          ^^^^^
                                                          being

> +       lisp code.  And there should not be more than one of
> +       up/down/click/drag anyway.  But since Lisp events can be
> +       synthesized, we don't take exception to unexpected
> +       combinations */

-- 
Alan Mackenzie (Nuremberg, Germany).

[PATCH v1] Let input queue deal gracefully with up-events

[David Kastrup]

* keyboard.c (apply_modifiers_uncached, parse_solitary_modifier)
(parse_modifiers_uncached): React gracefully to "up-" modifiers:
those may easily be injected by user-level Lisp code.
(read_key_sequence): Discard unbound up-events like unbound
down-events: they are even more likely only relevant for special
purposes.

While Emacs will not produce up-events on its own currently (those are
converted to drag or click events before being converted to
Lisp-readable structures), the input queue can be made to contain them
by synthesizing events to `unread-command-events'.  This patch makes
Emacs deal consistently with such events.

Signed-off-by: David Kastrup <dak@gnu.org>
---
While changing the comment Alan complained about I decided to fix
another comment that was no longer corresponding to the code.

 src/ChangeLog  |  9 +++++++++
 src/keyboard.c | 34 +++++++++++++++++++++++-----------
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/src/ChangeLog b/src/ChangeLog
index 1df4f6a..f9f53c4 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,12 @@
+2015-01-28  David Kastrup  <dak@gnu.org>
+
+	* keyboard.c (apply_modifiers_uncached, parse_solitary_modifier)
+	(parse_modifiers_uncached): React gracefully to "up-" modifiers:
+	those may easily be injected by user-level Lisp code.
+	(read_key_sequence): Discard unbound up-events like unbound
+	down-events: they are even more likely only relevant for special
+	purposes.
+
 2015-01-27  Eli Zaretskii  <eliz@gnu.org>
 
 	* dired.c (directory_files_internal) [WINDOWSNT]: If readdir
diff --git a/src/keyboard.c b/src/keyboard.c
index 383c109..b762e13 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -6202,6 +6202,10 @@ parse_modifiers_uncached (Lisp_Object symbol, ptrdiff_t *modifier_end)
 	case 't':
 	  MULTI_LETTER_MOD (triple_modifier, "triple", 6);
 	  break;
+
+	case 'u':
+	  MULTI_LETTER_MOD (up_modifier, "up", 2);
+	  break;
 #undef MULTI_LETTER_MOD
 
 	}
@@ -6249,16 +6253,18 @@ apply_modifiers_uncached (int modifiers, char *base, int base_len, int base_len_
   /* Since BASE could contain nulls, we can't use intern here; we have
      to use Fintern, which expects a genuine Lisp_String, and keeps a
      reference to it.  */
-  char new_mods[sizeof "A-C-H-M-S-s-down-drag-double-triple-"];
+  char new_mods[sizeof "A-C-H-M-S-s-up-down-drag-double-triple-"];
   int mod_len;
 
   {
     char *p = new_mods;
 
-    /* Only the event queue may use the `up' modifier; it should always
-       be turned into a click or drag event before presented to lisp code.  */
-    if (modifiers & up_modifier)
-      emacs_abort ();
+    /* Mouse events should not exhibit the `up' modifier; it should
+       always be turned into a click or drag event before being
+       presented to lisp code.  And there should not be more than one
+       of up/down/click/drag anyway.  But since Lisp events can be
+       synthesized, we don't take exception to unexpected
+       combinations */
 
     if (modifiers & alt_modifier)   { *p++ = 'A'; *p++ = '-'; }
     if (modifiers & ctrl_modifier)  { *p++ = 'C'; *p++ = '-'; }
@@ -6268,6 +6274,7 @@ apply_modifiers_uncached (int modifiers, char *base, int base_len, int base_len_
     if (modifiers & super_modifier) { *p++ = 's'; *p++ = '-'; }
     if (modifiers & double_modifier) p = stpcpy (p, "double-");
     if (modifiers & triple_modifier) p = stpcpy (p, "triple-");
+    if (modifiers & up_modifier) p = stpcpy (p, "up-");
     if (modifiers & down_modifier) p = stpcpy (p, "down-");
     if (modifiers & drag_modifier) p = stpcpy (p, "drag-");
     /* The click modifier is denoted by the absence of other modifiers.  */
@@ -6387,8 +6394,7 @@ DEFUN ("internal-event-symbol-parse-modifiers", Fevent_symbol_parse_modifiers,
    BASE must be unmodified.
 
    This is like apply_modifiers_uncached, but uses BASE's
-   Qmodifier_cache property, if present.  It also builds
-   Qevent_symbol_elements properties, since it has that info anyway.
+   Qmodifier_cache property, if present.
 
    apply_modifiers copies the value of BASE's Qevent_kind property to
    the modified symbol.  */
@@ -6734,6 +6740,10 @@ parse_solitary_modifier (Lisp_Object symbol)
       MULTI_LETTER_MOD (triple_modifier, "triple", 6);
       break;
 
+    case 'u':
+      MULTI_LETTER_MOD (up_modifier, "up", 2);
+      break;
+
 #undef SINGLE_LETTER_MOD
 #undef MULTI_LETTER_MOD
     }
@@ -9441,14 +9451,16 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
 		   Drags reduce to clicks.
 		   Double-clicks reduce to clicks.
 		   Triple-clicks reduce to double-clicks, then to clicks.
-		   Down-clicks are eliminated.
+		   Up/Down-clicks are eliminated.
 		   Double-downs reduce to downs, then are eliminated.
 		   Triple-downs reduce to double-downs, then to downs,
 		     then are eliminated.  */
-	      if (modifiers & (down_modifier | drag_modifier
+	      if (modifiers & (up_modifier | down_modifier
+			       | drag_modifier
 			       | double_modifier | triple_modifier))
 		{
-		  while (modifiers & (down_modifier | drag_modifier
+		  while (modifiers & (up_modifier | down_modifier
+				      | drag_modifier
 				      | double_modifier | triple_modifier))
 		    {
 		      Lisp_Object new_head, new_click;
@@ -9460,7 +9472,7 @@ read_key_sequence (Lisp_Object *keybuf, int bufsize, Lisp_Object prompt,
 			modifiers &= ~drag_modifier;
 		      else
 			{
-			  /* Dispose of this `down' event by simply jumping
+			  /* Dispose of this `up/down' event by simply jumping
 			     back to replay_key, to get another event.
 
 			     Note that if this event came from mock input,
-- 
2.1.0

Re: [PATCH] Let input queue deal gracefully with up-events

[Stefan Monnier]

> * keyboard.c (apply_modifiers_uncached, parse_solitary_modifier)
> (parse_modifiers_uncached): React gracefully to "up-" modifiers:
> those may easily be injected by user-level Lisp code.

Can you provide some context?  When would Lisp code inject events with
an "up-" modifier?


        Stefan

Re: [PATCH] Let input queue deal gracefully with up-events

[David Kastrup]

[-- Attachment #1: Type: text/plain, Size: 1661 bytes --]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> * keyboard.c (apply_modifiers_uncached, parse_solitary_modifier)
>> (parse_modifiers_uncached): React gracefully to "up-" modifiers:
>> those may easily be injected by user-level Lisp code.
>
> Can you provide some context?  When would Lisp code inject events with
> an "up-" modifier?

How else would you want to call the key release from a piano keyboard?
It is basically auxiliary information that can often be discarded: the
key press event is much more important when using a piano keyboard.

I'll append my current context.  Note that when you remove the code
pre-filling the modifier-cache (and haven't applied the given patch),
Emacs will crash when you press (actually when you release) a key on the
connected Midi keyboard, generating a [Ch1 up-C_4] event (for example).

The long-term goal is to integrate Midi event generation into the Emacs
binary.  But at the current point of time, I am still experimenting
around with figuring out just what information should arrive in what
form, and feeding Emacs with such non-native events should be possible
as well.

If you don't have actual Midi hardware, install vmpk (a virtual Midi
keyboard) and do

sudo insmod snd_virmidi

then use the "Connections" menu of vmpk for connecting the keyboard
output to the first virtual Midi device, load the attached file, and
then do M-x midikbd-open RET RET in order to have keypresses and
releases from the keyboard arrive in Emacs.  If the "... is undefined"
messages are not helpful enough, try

M-: (while t (message "%S" (read-event))) RET

to see more details.  End with C-g of course.

-- 
David Kastrup

[-- Attachment #2: midi-kbd3.el --]
[-- Type: application/emacs-lisp, Size: 8974 bytes --]

Re: [PATCH] Let input queue deal gracefully with up-events

[Stefan Monnier]

> How else would you want to call the key release from a piano keyboard?
> It is basically auxiliary information that can often be discarded: the
> key press event is much more important when using a piano keyboard.

I see, that makes sense.


        Stefan

Re: [PATCH] Let input queue deal gracefully with up-events

[Stefan Monnier]

> -		   Down-clicks are eliminated.
> +		   Up/Down-clicks are eliminated.

Actually, up-clicks (in the sens of *mouse* clicks) are not eliminated.

I think it's be better to explain that we auto-eliminate both the
"down-<foo>" and "up-<foo>" events such that, depending on which of the
up or down part of an event is the important one, we use "down-<foo> and
<foo>" or "<foo> and up-<foo>"


        Stefan

Re: [PATCH] Let input queue deal gracefully with up-events

[David Kastrup]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> How else would you want to call the key release from a piano keyboard?
>> It is basically auxiliary information that can often be discarded: the
>> key press event is much more important when using a piano keyboard.
>
> I see, that makes sense.

Well, at least there does not seem to be much of a point of Emacs having
only half-baked support of "up-" with optional segfaults when the event
queue is writeable for program-generated events.  While I want to add
native Midi support eventually (it will make the timing info have much
less jitter and should make portability to Windows etc easier), this
would require the same sort of changes anyway, and so will other
attempts at playing with synthetic key events.

Discarding unbound up-events without error like down-events are
currently discarded is, of course, a somewhat arbitrary design decision.
However, if up-xxx is a modified version of xxx, chances are that its
state is comparable to down-xxx.

For Midi, I am still vacillating between using xxx/up-xxx or
down-xxx/up-xxx.  The former (where an unbound xxx leads to a complaint
and an unbound up-xxx is silently discarded) actually corresponds pretty
well with the normal semantics expected from a keyboard and would
probably also operate nicer in connection with M-x local-set-key RET.

My first approach was not messing with Emacs current modifier handling
at all and using xxx-down for keypress and xxx for keyrelease.  But that
was not really satisfactory since then M-x local-set-key RET binds the
keyrelease if you press a key.

Pre-stuffing the modifier caches is also only half-satisfactory since if
you then press a piano key, type C-x @ m and release the key again,
Emacs will still emacs_abort (since the modifier cache was not
pre-filled for M-up-xxx).

So at least in the long run, I think it makes good sense to make the
keyboard-queue backend not complain about data that could not have
originated from the keyboard-queue frontend.  It's not like the crashes
and inconsistencies serve any useful purpose.

Arguably, even if some problem is caused by unexpected things happening
in the keyboard-queue frontend, you may be better served with the
keyboard-queue backend delivering a quite descriptive and printable
strange event rather than crashing: after all, the actual program
location that could have _generated_ the data is no longer connected
with the call stack when emacs_abort is called.

-- 
David Kastrup

Re: [PATCH] Let input queue deal gracefully with up-events

[David Kastrup]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> -		   Down-clicks are eliminated.
>> +		   Up/Down-clicks are eliminated.
>
> Actually, up-clicks (in the sens of *mouse* clicks) are not
> eliminated.

Well, they are at _this_ location.  *mouse* clicks have been converted
to click/drag at that point.

> I think it's be better to explain that we auto-eliminate both the
> "down-<foo>" and "up-<foo>" events such that, depending on which of the
> up or down part of an event is the important one, we use "down-<foo> and
> <foo>" or "<foo> and up-<foo>"

Well, it is not really comparable since down-xxx + up-xxx for the mouse
are combined into one xxx or drag-xxx event: in this case the
"eliminated" down-xxx event still has its timing and area data
integrated into the resulting event.

When discarding an up-xxx event, the corresponding xxx event has already
been delivered.  I've thought about _modifying_ the original xxx event
to add the up-xxx info, but in the case where you need it, you need
something to trigger the related action anyway, and if such event-fixup
needs to be done, it can be done by the separately bound up-xxx code
under user control rather than silently modifying an already processed
event.

So the xxx-down/xxx combination has to work differently from xxx/xxx-up.
But the point is that this difference is in the keyboard queue
_frontend_ (the one creating the Lisp event, whether natively or
synthetically).  The keyboard queue backend can ignore this difference.

-- 
David Kastrup

Re: [PATCH] Let input queue deal gracefully with up-events

[Stefan Monnier]

> Well, it is not really comparable since down-xxx + up-xxx for the mouse
> are combined into one xxx or drag-xxx event.

That's one way to look at it.  If you look at it w.r.t XEmacs, where
they have `button1' and `button1up' (IIRC), I prefer to consider that our
`mouse-1' is an up event (and we simply define the click as happening
on the up event).

The point here is just that the way the comment is written makes it
sound like we'd throw away all events (both up and down), whereas we
only throw away the "companion event" (either the up or the down one,
depending on the specific kind of event).


        Stefan

Re: [PATCH] Let input queue deal gracefully with up-events

[David Kastrup]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Well, it is not really comparable since down-xxx + up-xxx for the mouse
>> are combined into one xxx or drag-xxx event.
>
> That's one way to look at it.  If you look at it w.r.t XEmacs, where
> they have `button1' and `button1up' (IIRC), I prefer to consider that
> our `mouse-1' is an up event (and we simply define the click as
> happening on the up event).

Whether _you_ prefer to look at it that way does not change how Emacs'
internals look at it.

> The point here is just that the way the comment is written makes it
> sound like we'd throw away all events (both up and down), whereas we
> only throw away the "companion event" (either the up or the down one,
> depending on the specific kind of event).

I don't think it helps anybody if the code comments live in a different
universe than the code.

Emacs basically has two layers in the event code.  One layer deals with
events in C structures.  Where the C structures are converted into Lisp,
the basic up/click/drag machinery is implemented.  It is here that the
"up" modifier for mouse events disappears.  But that's before even
entering any user-visible data structures.  At the Lisp level (barring
Lisp-accessible semi-internals like the modifier-cache), the "up-"
modifier does not currently exist consistently (usually the tables have
it and the code doesn't).

Now for the Midi stuff (which should end up in Emacs some day),
implementing something like a "off-" or "release-" modifier does not
seem to make a lot of sense when the C layer already has symmetric
up/down, the Lisp layer has a half-implemented "up-", and the semantics
are quite similar to the mouse "down-".

I digress.  What I am getting up is that the Lisp layer of the event
code (namely where things are taken out of the Lisp event queues again
and read and/or looked up in keymaps and executed) does not know
anything about the origins of click/drag events and any prospective
relations to up-events in the C code.  And it does not _need_ to know
either.  That's stuff that happened before things went _into_ the queue.

This is basically the level where keymaps are consulted, where macro
recording and replay happens, and where synthetic events like from
xt-mouse.el gets inserted.

And at this level, there is _no_ relation between up events and
click/drag.  Comments suggesting otherwise will not help with
understanding the code.

-- 
David Kastrup

Re: [PATCH] Let input queue deal gracefully with up-events

[Stefan Monnier]

Sorry, David,

I actually hadn't noticed I was arguing with you, which of course is
known to be a waste of time.  I'll just change the comment after you
install the patch.


        Stefan

Re: [PATCH] Let input queue deal gracefully with up-events

[David Kastrup]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

> Sorry, David,
>
> I actually hadn't noticed I was arguing with you, which of course is
> known to be a waste of time.  I'll just change the comment after you
> install the patch.

I am not an Emacs developer so I don't have any influence over any
changes in the patch you may want to do before pushing it even if it
means a divergence of code and comments.

-- 
David Kastrup

Re: [PATCH v1] Let input queue deal gracefully with up-events

[David Kastrup]

David Kastrup <dak@gnu.org> writes:

> * keyboard.c (apply_modifiers_uncached, parse_solitary_modifier)
> (parse_modifiers_uncached): React gracefully to "up-" modifiers:
> those may easily be injected by user-level Lisp code.
> (read_key_sequence): Discard unbound up-events like unbound
> down-events: they are even more likely only relevant for special
> purposes.
>
> While Emacs will not produce up-events on its own currently (those are
> converted to drag or click events before being converted to
> Lisp-readable structures), the input queue can be made to contain them
> by synthesizing events to `unread-command-events'.  This patch makes
> Emacs deal consistently with such events.

I've created bug report
<URL:http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19746>.  Judging from
the number of wishlist items in the tracker including a patch, that does
not appear to increase its chances of getting applied but at least it is
then rotting in the proper place.

-- 
David Kastrup

Re: [PATCH v1] Let input queue deal gracefully with up-events

[Stefan Monnier]

> <URL:http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19746>.  Judging from
> the number of wishlist items in the tracker including a patch, that does
> not appear to increase its chances of getting applied but at least it is
> then rotting in the proper place.

What would increase the chances, would be you requesting write-access,
of course ;-)
Then you'd just have to wait for David to install your patch,


        Stefan

Re: [PATCH v1] Let input queue deal gracefully with up-events

[Ivan Shmakov]

>>>>> David Kastrup <dak@gnu.org> writes:

[…]

 > I've created bug report
 > <URL:http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19746>.  Judging
 > from the number of wishlist items in the tracker including a patch,

	It’s a very rough metric at best.  For one thing, the chances of
	a patch being applied tend to depend highly on the number of the
	Emacs developers interested in a particular subsystem or package
	at the given time.

 > that does not appear to increase its chances of getting applied but
 > at least it is then rotting in the proper place.

	One of these days I may actually decide to build an X-capable
	Emacs.  Should it still be possible at that point to apply the
	patch cleanly (and assuming it won’t crash), – I’d happily push
	it.  (Combined with that trivial improvement suggested during
	the preceding review, I guess.)

	As an aside, I was unable to try the midi-kbd.el package myself
	(hardly a trivial thing when you use a tty-only Emacs), but if
	it indeed works as described, – I’d happily give my vote to it
	for the Emacs Contribution of the Month challenge.  (Assuming
	we’d care to run such a thing, anyway.)

[1] news:87lhlbmx7l.fsf@fencepost.gnu.org
    http://permalink.gmane.org/gmane.emacs.devel/181124

-- 
FSF associate member #7257  np. Lock up the Wolves — Dio  3013 B6A0 230E 334A

Re: [PATCH v1] Let input queue deal gracefully with up-events

[David Kastrup]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> <URL:http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19746>.  Judging from
>> the number of wishlist items in the tracker including a patch, that does
>> not appear to increase its chances of getting applied but at least it is
>> then rotting in the proper place.
>
> What would increase the chances, would be you requesting write-access,
> of course ;-)

Basically you say that the patch submission and vetting process is
fundamentally broken and useless and that people should ignore the
developer list and bug tracker and just dump their code into the
repository instead and see whether others want to fix it.

In fact, you explicitly recommended this way of operation in
<URL:http://lists.gnu.org/archive/html/emacs-devel/2015-01/msg01065.html>.
On the other hand, this list has recently seen considerable rebuke
against a developer doing exactly that.  I am not interested in the role
of a black sheep so I prefer using the submission paths intended for
untrusted developers.

> Then you'd just have to wait for David to install your patch,

As long as he cannot be trusted to act responsibly and not commit
patches that need others to clean up after him, I cannot see it as
desirable to grant him push access.

He has been judged in clear need of vetting, and the tool for that are
the mailing list and the bug tracker.

-- 
David Kastrup

Re: [PATCH v1] Let input queue deal gracefully with up-events

[Ivan Shmakov]

>>>>> David Kastrup <dak@gnu.org> writes:
>>>>> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

 >>> <URL:http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19746>.  Judging
 >>> from the number of wishlist items in the tracker including a patch,
 >>> that does not appear to increase its chances of getting applied but
 >>> at least it is then rotting in the proper place.

 >> What would increase the chances, would be you requesting
 >> write-access, of course ;-)

 > Basically you say that the patch submission and vetting process is
 > fundamentally broken and useless and that people should ignore the
 > developer list and bug tracker and just dump their code into the
 > repository instead and see whether others want to fix it.

	I’m unsure if this comment of mine will help or not, but I /do/
	see the difference between “the change is OK and I will install
	the patch” and “the change is OK, but I will /not/ install the
	patch (because of…)” as the outcomes of the review process.

	In this particular case, the review process (AIUI) resulted in
	the latter, due to the disagreement on the wording of a single
	comment in the code.  However, given the “the change is OK”
	part, I see no reason for an interested party to refrain from
	pushing the change, either simply changing that single line of
	contention (so to say) him- or herself along the way, – or
	leaving it to the party interested in /that/ change.

	Should the review process result in the “the change is NOT OK”
	outcome, it would indeed be inappropriate for a developer to
	push the change.  But that’s not the case for #19746.

[…]

-- 
FSF associate member #7257  http://boycottsystemd.org/  … 3013 B6A0 230E 334A

Re: [PATCH v1] Let input queue deal gracefully with up-events

[David Kastrup]

Ivan Shmakov <ivan@siamics.net> writes:

>>>>>> David Kastrup <dak@gnu.org> writes:
>>>>>> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
>
>  >>> <URL:http://debbugs.gnu.org/cgi/bugreport.cgi?bug=19746>.  Judging
>  >>> from the number of wishlist items in the tracker including a patch,
>  >>> that does not appear to increase its chances of getting applied but
>  >>> at least it is then rotting in the proper place.
>
>  >> What would increase the chances, would be you requesting
>  >> write-access, of course ;-)
>
>  > Basically you say that the patch submission and vetting process is
>  > fundamentally broken and useless and that people should ignore the
>  > developer list and bug tracker and just dump their code into the
>  > repository instead and see whether others want to fix it.
>
> 	I’m unsure if this comment of mine will help or not, but I /do/
> 	see the difference between “the change is OK and I will install
> 	the patch” and “the change is OK, but I will /not/ install the
> 	patch (because of…)” as the outcomes of the review process.
>
> 	In this particular case, the review process (AIUI) resulted in
> 	the latter, due to the disagreement on the wording of a single
> 	comment in the code.

Not at all.  Comments are part of a change, and the review process
resulted in "the change is not OK.  Should anybody be so unreasonable to
install it in this form, it will be necessary for Stefan to clean up
afterwards".  Since the original author refuses to make the suggested
changes (as opposed to other change suggestions he heeded) because of
being inaccessible to reason, these changes have to be made by
reasonable people instead.

A developer unwilling to keep the Emacs repository in acceptable state
should not have write access.  I can, of course, offer to make the
requested change myself, in a separate commit specifying Stefan's
authorship, with a commit message of his choosing, and then push both
commits.  However, I would then also file a bug report suggesting to
revert the commit attributed to Stefan, referring to my explanation why
it is wrong.

I think we are better off sparing ourselves such silliness.

I am able to work around this shortcoming in the binary by messing with
the modifier cache internals from the Lisp side.  It's ugly and does not
lend itself to further extensibility, but it's not an ultimate
showstopper.

> 	Should the review process result in the “the change is NOT OK”
> 	outcome, it would indeed be inappropriate for a developer to
> 	push the change.  But that’s not the case for #19746.

Comments are an important part of code crucially affecting its
maintainability.  So it _is_ the case.

-- 
David Kastrup