bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Hello,

I'm using Debian Gnu/Linux here.

Eval

(catch 'tag
  (let ((throw-on-input 'tag))
    (while t)))

and switch to a different (X) workspace.  The loop is exited
immediately.

I don't think it is useful to count switching workspaces as input.  I
hope we can change this.

Some background: in Helm, we use `while-no-input' around the code
performing the matching against the candidates.  This way, we can
immediately react when new input arrives - we avoid a delay caused by
finishing matching candidates against an obsolete input pattern.

But currently, when you switch to a different workspace while matching
is performed, Helm stops matching.

Of course we could work around this and restart matching anew in such a
case, but it would be better if switching desktops would not tangle
Emacs at all.  It is counter-intuitive, at least in this scenario.  The
doc of throw-on-input only speaks about "keyboard input".


Thanks,

Michael.



In GNU Emacs 25.0.50.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.14.5)
 of 2015-01-08 on drachen
Windowing system distributor `The X.Org Foundation', version 11.0.11602901
System Description:	Debian GNU/Linux 8.0 (jessie)

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND DBUS GSETTINGS NOTIFY
LIBXML2 FREETYPE XFT ZLIB

Important settings:
  value of $LC_ALL: de_DE.utf8
  value of $LC_COLLATE: C
  value of $LC_TIME: C
  value of $LANG: de_DE.utf8
  locale-coding-system: utf-8-unix

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Fri, 09 Jan 2015 16:46:58 +0100
> 
> (catch 'tag
>   (let ((throw-on-input 'tag))
>     (while t)))
> 
> and switch to a different (X) workspace.  The loop is exited
> immediately.
> 
> I don't think it is useful to count switching workspaces as input.  I
> hope we can change this.
> 
> Some background: in Helm, we use `while-no-input' around the code
> performing the matching against the candidates.  This way, we can
> immediately react when new input arrives - we avoid a delay caused by
> finishing matching candidates against an obsolete input pattern.
> 
> But currently, when you switch to a different workspace while matching
> is performed, Helm stops matching.
> 
> Of course we could work around this and restart matching anew in such a
> case, but it would be better if switching desktops would not tangle
> Emacs at all.  It is counter-intuitive, at least in this scenario.  The
> doc of throw-on-input only speaks about "keyboard input".

Documentation problems aside, you can't usefully define which events
will be considered as input and which won't, in a way that would DTRT
for everyone.  The decision is specific to the application, and
while-no-input cannot know that.

It could be that this particular event can be filtered out, but that's
just the top of the iceberg.  There are way too many different events
flowing into Emacs on a typical modern system, keyboard being just a
small part of that.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> It could be that this particular event can be filtered out, but that's
> just the top of the iceberg.  There are way too many different events
> flowing into Emacs on a typical modern system, keyboard being just a
> small part of that.

Mmh, I expected that this could be the case.  But is there no more or
less reasonable way to classify events?  Probably not...

If there is nothing that can be done about this report - do you see a
solution for the problem we see in Helm?


Michael.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 19547@debbugs.gnu.org
> Date: Fri, 09 Jan 2015 22:48:25 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It could be that this particular event can be filtered out, but that's
> > just the top of the iceberg.  There are way too many different events
> > flowing into Emacs on a typical modern system, keyboard being just a
> > small part of that.
> 
> Mmh, I expected that this could be the case.  But is there no more or
> less reasonable way to classify events?  Probably not...

If you are thinking about a general-purpose infrastructure to ignore
some specific classes of events, we could try considering that.  Of
course, any such filtering can always be done on the application
level, and the question then is how more useful will it be to have it
in the infrastructure.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Stefan Monnier]

> (catch 'tag
>   (let ((throw-on-input 'tag))
>     (while t)))
>
> and switch to a different (X) workspace.  The loop is exited
> immediately.

Can you check with C-h l what event Emacs received?

> If there is nothing that can be done about this report -

That's probably the case.  The intention of while-no-input is to abort
in response any input that might need execution of Elisp code.  So if
the event you receive upon workspace-switch might trigger execution of
Elisp code, it's probably right for while-no-input to abort.

> do you see a solution for the problem we see in Helm?

I don't understand exactly the problem you're seeing.  Why is it
a problem to stop computing the matching candidates when the user
switches to another workspace?  If "switches to another workspace"
means that the selected Emacs frame disappears (a likely scenario), then
it seems harmless to stop computing the candidates.


        Stefan

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

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

> > (catch 'tag
> >   (let ((throw-on-input 'tag))
> >     (while t)))
> >
> > and switch to a different (X) workspace.  The loop is exited
> > immediately.
>
> Can you check with C-h l what event Emacs received?

The event isn't listed there.  I think it is focus-out or something like
that.

> I don't understand exactly the problem you're seeing.  Why is it
> a problem to stop computing the matching candidates when the user
> switches to another workspace?  If "switches to another workspace"
> means that the selected Emacs frame disappears (a likely scenario)

Exactly.

> then it seems harmless to stop computing the candidates.

I switch to a different workspace (or focus a different application) to
do something useful until matching has finished.

Imagine you have opened some large page on firefox, switch to another
app to do something else until the page is ready, and when you come back
to firefox, it shows a blank page, because it considers losing focus as
an important user input and just stops rendering.  That's what Helm
currently does with candidate matching cause it relies (and needs to
rely) on while-no-input.

Note that Helm can't just continue performing matching later, because
throw-on-input made Emacs jump out of the code.  Which is what we want
of cause when there was any "real" user input, i.e., when the user added
text to the matching pattern for example.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> The event isn't listed there.  I think it is focus-out or something like
> that.

Switching to a different workspace lets Emacs create an iconify-frame
event.  That one is probably the one letting throw-on-input fire,
because it is the first event generated.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Sat, 10 Jan 2015 02:26:32 +0100
> Cc: 19547@debbugs.gnu.org
> 
> Michael Heerdegen <michael_heerdegen@web.de> writes:
> 
> > The event isn't listed there.  I think it is focus-out or something like
> > that.
> 
> Switching to a different workspace lets Emacs create an iconify-frame
> event.  That one is probably the one letting throw-on-input fire,
> because it is the first event generated.

I suggest to establish exactly which event causes that, then we could
focus on how to filter that out, either in general or in your specific
situation.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> I suggest to establish exactly which event causes that, then we could
> focus on how to filter that out, either in general or in your specific
> situation.

Ok, I did the following, hoping it's good enough.

In keyboard.c, near the end of the defun of kbd_buffer_store_event_hold,
there is an exclusion list of events ignored by throw_on_input:

if (!NILP (Vthrow_on_input)
      && event->kind != FOCUS_IN_EVENT
      && event->kind != HELP_EVENT
      && event->kind != DEICONIFY_EVENT)
    {...
    ...}

When I add the two lines

      && event->kind != FOCUS_OUT_EVENT
      && event->kind != ICONIFY_EVENT

my problem is solved and throw-on-input does not fire when switching
the workspaces.  Helm does not stop matching anymore.  Also the

(catch 'tag
  (let ((throw-on-input 'tag))
    (while t)))

example is not intercepted.

When I only add one of either lines, the behavior is not different from
what we have now.

Michael.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Stefan Monnier]

> if (!NILP (Vthrow_on_input)
>       && event->kind != FOCUS_IN_EVENT
>       && event->kind != HELP_EVENT
>       && event->kind != DEICONIFY_EVENT)
>     {...
>     ...}

> When I add the two lines

>       && event->kind != FOCUS_OUT_EVENT
>       && event->kind != ICONIFY_EVENT

Adding FOCUS_OUT_EVENT seems definitely safe since we already handle
FOCUS_IN_EVENT like that.  As for ICONIFY_EVENT, it seems a bit more
risky, but I think we can give it a try.


        Stefan

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

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

> >       && event->kind != FOCUS_OUT_EVENT
> >       && event->kind != ICONIFY_EVENT
>
> Adding FOCUS_OUT_EVENT seems definitely safe since we already handle
> FOCUS_IN_EVENT like that.  As for ICONIFY_EVENT, it seems a bit more
> risky, but I think we can give it a try.

Eli, would that change be acceptable in your eyes?

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Tue, 20 Jan 2015 03:46:57 +0100
> 
> Stefan Monnier <monnier@IRO.UMontreal.CA> writes:
> 
> > >       && event->kind != FOCUS_OUT_EVENT
> > >       && event->kind != ICONIFY_EVENT
> >
> > Adding FOCUS_OUT_EVENT seems definitely safe since we already handle
> > FOCUS_IN_EVENT like that.  As for ICONIFY_EVENT, it seems a bit more
> > risky, but I think we can give it a try.
> 
> Eli, would that change be acceptable in your eyes?

I don't see any immediate problems, but then I'm not an expert on GUI
events.  So I agree with Stefan: let's try.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> I don't see any immediate problems, but then I'm not an expert on GUI
> events.  So I agree with Stefan: let's try.

Great.  Can please someone make the change?  I cloned the new git repo
today, but am far away from understanding the committing shibboleths
(apart from the fact that I probably don't have write access).

Thanks!

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 19547@debbugs.gnu.org
> Date: Thu, 29 Jan 2015 20:59:42 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I don't see any immediate problems, but then I'm not an expert on GUI
> > events.  So I agree with Stefan: let's try.
> 
> Great.  Can please someone make the change?

I did that now (commit eaea02c on master).  Please run the test suite
and see if there are any regressions.  I see a failure in
timer-tests-debug-timer-check, but I'm not sure this wasn't happening
before the change.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> I did that now (commit eaea02c on master).

Thanks.  Works as expected.

>  Please run the test suite and see if there are any regressions.

I'll do this.  Before that I have to learn how to do it, though.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 19547@debbugs.gnu.org
> Date: Sun, 01 Feb 2015 15:25:56 +0100
> 
> >  Please run the test suite and see if there are any regressions.
> 
> I'll do this.  Before that I have to learn how to do it, though.

Like this:

 $ cd tests/automated && make check

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Like this:
>
>  $ cd tests/automated && make check

(You probably meant test/automated)

Seems I get one error: vc-test-svn03-working-revision.  Probably
unrelated to this issue here.


Michael.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 19547@debbugs.gnu.org
> Date: Sun, 01 Feb 2015 18:21:48 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Like this:
> >
> >  $ cd tests/automated && make check
> 
> (You probably meant test/automated)

Yes, sorry.

> Seems I get one error: vc-test-svn03-working-revision.  Probably
> unrelated to this issue here.

Thanks for testing.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks for testing.

FWIW, we have been bitten by this problem again, for example (just for
reference, no need to read the complete thread):

    https://github.com/emacs-helm/helm/issues/1157

 Most or all of the time the problem was related to a tool called
"autocutsel".  It makes Emacs receiving a SELECTION_REQUEST_EVENT that
breaks `while-no-input'.

The OP found that this patch:

@@ -3542,7 +3542,8 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
       && event->kind != FOCUS_OUT_EVENT
       && event->kind != HELP_EVENT
       && event->kind != ICONIFY_EVENT
-      && event->kind != DEICONIFY_EVENT)
+      && event->kind != DEICONIFY_EVENT
+      && event->kind != SELECTION_REQUEST_EVENT)
     {
       Vquit_flag = Vthrow_on_input;
       /* If we're inside a function that wants immediate quits,

fixes the problem for him.

Dunno if it is a good idea to install that.  We kind of misuse
`while-no-input' in Helm, but due to the lack of a better alternative.
What we would need would be something like `while-no-keyboard-input'
that only rewinds when the user hits a key on his keyboard.  Any other
event breaking `while-no-input' just leaves the user wondering why no
candidates have been calculated.


Thanks,

Michael.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[John Wiegley]

>>>>> Michael Heerdegen <michael_heerdegen@web.de> writes:

> Dunno if it is a good idea to install that. We kind of misuse
> `while-no-input' in Helm, but due to the lack of a better alternative. What
> we would need would be something like `while-no-keyboard-input' that only
> rewinds when the user hits a key on his keyboard. Any other event breaking
> `while-no-input' just leaves the user wondering why no candidates have been
> calculated.

So is the current status that you're suggesting we create
`while-no-keyboard-input'? That sounds like it could be a useful distinction.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

John Wiegley <jwiegley@gmail.com> writes:

> So is the current status that you're suggesting we create
> `while-no-keyboard-input'? That sounds like it could be a useful
> distinction.

It would be very useful, I think, but my knowledge about that C part
(all C parts, to be honest) of Emacs is nearly zero.

But I think Eli already mentioned that this would not be trivial AFAIR.
I guess Emacs can't clearly decide whether an event is a keyboard event.


Michael.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[John Wiegley]

>>>>> Michael Heerdegen <michael_heerdegen@web.de> writes:

> But I think Eli already mentioned that this would not be trivial AFAIR. I
> guess Emacs can't clearly decide whether an event is a keyboard event.

What are your thoughts now, Eli?

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: John Wiegley <jwiegley@gmail.com>
>> Cc: Michael Heerdegen <michael_heerdegen@web.de>,  19547@debbugs.gnu.org,  Thierry Volpiatto <thierry.volpiatto@gmail.com>
>> Date: Thu, 10 Dec 2015 15:01:58 -0800
>> 
>> >>>>> Michael Heerdegen <michael_heerdegen@web.de> writes:
>> 
>> > But I think Eli already mentioned that this would not be trivial AFAIR. I
>> > guess Emacs can't clearly decide whether an event is a keyboard event.
>> 
>> What are your thoughts now, Eli?
>
> About what?  About adding SELECTION_REQUEST_EVENT to the list of
> events ignored by while-no-input, or about adding
> while-no-keyboard-input?
>
> For the former, I guess we could try that on master and see if it
> breaks something.

Agree, think it would be a good start.

> I'm much less sure about the latter: why does it make sense to single
> out keyboard events in Emacs?

I am not sure the latter i.e while-no-keyboard-input would help for
helm.

> Yet another possibility would be for Helm to finally get its act
> together

Not sure to understand this sentence, sorry for by bad english.

> and filter the events it doesn't want to handle on the application
> level.

If you provide an API for this yes, otherwise how am I supposed to do
this ?

> I mean, how far should we go on such a low level to support a single
> package?

This issue is not about a single package, but for all code using
while-no-input.

> All it takes for help is to define its own proxy for while-no-input,
> and add a list of events that should not cause that proxy to return.

You mean a list of events usable on lisp ?

-- 
Thierry
https://emacs-helm.github.io/helm/

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> > This issue is not about a single package, but for all code using
> > while-no-input.
>
> No other package complained for quite some time.  The last 2 or 3
> complaints were all due to Helm, AFAIR.

Maybe we are the only one who succeeded in debugging that problem?  It
was not trivial to find what caused this rare kind of breakage, and
it was hard to reproduce.

I'm open for all kinds of solutions, and understand if you don't want to
invest a lot of time into that.

OTOH I don't think that letting Emacs do something until the user hits a
key is such an obscure thing.  Maybe Helm is the only place where
developers noticed that this kind of thing can be done easily at all?


Michael.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> My suggestion is to make the list of events that input-pending-p
> ignores customizable from Lisp, and then Helm could use that to have
> its way.

We should not put aside this thread and try to reconstruct the issue
months later one more time.

If nobody has a better idea - Eli's suggestion solves our problem in an
easy way, so I would say: let's please try this!

I only reluctantly would want to ask Eli to implement this.  I can't do
it, however.


Regards,

Michael.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: thierry.volpiatto@gmail.com,  jwiegley@gmail.com,  19547@debbugs.gnu.org
> Date: Sat, 26 Dec 2015 01:37:29 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > My suggestion is to make the list of events that input-pending-p
> > ignores customizable from Lisp, and then Helm could use that to have
> > its way.
> 
> We should not put aside this thread and try to reconstruct the issue
> months later one more time.
> 
> If nobody has a better idea - Eli's suggestion solves our problem in an
> easy way, so I would say: let's please try this!
> 
> I only reluctantly would want to ask Eli to implement this.  I can't do
> it, however.

My priorities are currently to finish updating the docs by going
through NEWS, which is a huge job.  So it would be best if someone
else could take a shot at this.  It shouldn't be too hard, I think.

bug#19547: 25.0.50; throw-on-input "fires" when switching workspace

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Sat, 10 Jan 2015 01:00:50 +0100
> Cc: 19547@debbugs.gnu.org
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
> > > (catch 'tag
> > >   (let ((throw-on-input 'tag))
> > >     (while t)))
> > >
> > > and switch to a different (X) workspace.  The loop is exited
> > > immediately.
> >
> > Can you check with C-h l what event Emacs received?
> 
> The event isn't listed there.  I think it is focus-out or something like
> that.

You should be able to see it in a C-level debugger, then.

bug#19547: Patch for this bug

[Reuben Thomas]

[-- Attachment #1.1: Type: text/plain, Size: 705 bytes --]

I attach a patch for this bug that takes the simple approach of adding
SELECTION_REQUEST_EVENT to the list of events to ignore in
kbd_buffer_store_buffered_event.

This seems simple and safe (it's not something one would normally consider
an input event).

Further, at present it would not help helm to implement Eli's suggestion of
a list of events for input-pending-p to ignore, as Helm currently does not
use that (it has a custom version of while-no-input that does not call
input-pending-p).

As a helm user with an external clipboard manager, I was experiencing this
bug. I'm using Emacs with this patch currently; it seems fine, and I don't
have the Helm problem any more.

-- 
http://rrt.sc3d.org

[-- Attachment #1.2: Type: text/html, Size: 1311 bytes --]

[-- Attachment #2: 0001-Ignore-SELECTION_REQUEST_EVENT-in-while-no-input.patch --]
[-- Type: text/x-patch, Size: 983 bytes --]

From 640dfc4b6165b5b1968b3b84f1c99cc7351377a3 Mon Sep 17 00:00:00 2001
From: Reuben Thomas <rrt@sc3d.org>
Date: Tue, 8 Nov 2016 18:01:37 +0000
Subject: [PATCH] Ignore SELECTION_REQUEST_EVENT in while-no-input

* src/keyboard.c (kbd_buffer_store_buffered_event): Do not treat
SELECTION_REQUEST_EVENT as an input event (Bug#19547).
---
 src/keyboard.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index f24d86e..a1cf677 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -3559,7 +3559,8 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
       && event->kind != FOCUS_OUT_EVENT
       && event->kind != HELP_EVENT
       && event->kind != ICONIFY_EVENT
-      && event->kind != DEICONIFY_EVENT)
+      && event->kind != DEICONIFY_EVENT
+      && event->kind != SELECTION_REQUEST_EVENT)
     {
       Vquit_flag = Vthrow_on_input;
       /* If we're inside a function that wants immediate quits,
-- 
2.7.4

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Reuben Thomas <rrt@sc3d.org>
> Date: Tue, 8 Nov 2016 18:28:28 +0000
> 
> Further, at present it would not help helm to implement Eli's suggestion of a list of events for input-pending-p
> to ignore, as Helm currently does not use that (it has a custom version of while-no-input that does not call
> input-pending-p).

The suggestion was not that specific.  The idea was to let Lisp
programs specify which special events they would like to consider as
input.  E.g., define a variable that holds a list of such events, and
test the value of that variable in the same place where you propose to
add yet another event to those ignored for the purposes of
throw-on-input (IMO, the same list should be looked at in
readable_events, which will then make input-pending-p consistent with
while-no-input and any other similar functionality).  It shouldn't be
too hard to implement that, and we gain a certain peace of mind in
that we don't have to worry about some hypothetical application that
would like to do stuff differently from Helm.

IOW, since this is going to go on master, I see no reason to hurry
with a simple solution, and would prefer a slightly more complex one,
but one that is more future-proof.  Can you do that?

Thanks.

bug#19547: Patch for this bug

[Reuben Thomas]

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

On 8 November 2016 at 20:40, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Reuben Thomas <rrt@sc3d.org>
> > Date: Tue, 8 Nov 2016 18:28:28 +0000
> >
> > Further, at present it would not help helm to implement Eli's suggestion
> of a list of events for input-pending-p
> > to ignore, as Helm currently does not use that (it has a custom version
> of while-no-input that does not call
> > input-pending-p).
>
> The suggestion was not that specific.  The idea was to let Lisp
> programs specify which special events they would like to consider as
> input.  E.g., define a variable that holds a list of such events, and
> test the value of that variable in the same place where you propose to
> add yet another event to those ignored for the purposes of
> throw-on-input (IMO, the same list should be looked at in
> readable_events, which will then make input-pending-p consistent with
> while-no-input and any other similar functionality).  It shouldn't be
> too hard to implement that, and we gain a certain peace of mind in
> that we don't have to worry about some hypothetical application that
> would like to do stuff differently from Helm.
>
> IOW, since this is going to go on master, I see no reason to hurry
> with a simple solution, and would prefer a slightly more complex one,
> but one that is more future-proof.  Can you do that?
>

​I'm in the middle of a long list of Emacs bugs I am trying to fix, and
this one came up by chance at the same time, because I was suffering from
the bug.

I'd rather have a simple fix that can also go on the emacs-25 branch, and I
don't really have more time to work on a proper fix, sorry.

On the other hand, I think it would be sad to have to wait (perhaps
forever!) for a "good" fix, when an acceptable fix is available. In
particular, no reasonable application is going to expect a request for the
selection to trigger a keyboard input event. So indeed future applications
may need the "good" fix, but no (reasonable) application will be adversely
affected by this fix.

-- 
http://rrt.sc3d.org

[-- Attachment #2: Type: text/html, Size: 3012 bytes --]

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Reuben Thomas <rrt@sc3d.org>
> Date: Tue, 8 Nov 2016 22:20:11 +0000
> Cc: 19547@debbugs.gnu.org
> 
> ​I'm in the middle of a long list of Emacs bugs I am trying to fix, and this one came up by chance at the same
> time, because I was suffering from the bug.
> 
> I'd rather have a simple fix that can also go on the emacs-25 branch, and I don't really have more time to work
> on a proper fix, sorry.

This change will not be on emacs-25 (as this issue existed forever),
so there's no rush in implementing it.  You can do it whenever you
have time.  If you cannot afford working more on this issue, then I
guess we will have to wait for someone else to implement my suggestion
(or something similar).

> On the other hand, I think it would be sad to have to wait (perhaps forever!) for a "good" fix, when an
> acceptable fix is available. In particular, no reasonable application is going to expect a request for the selection
> to trigger a keyboard input event. So indeed future applications may need the "good" fix, but no (reasonable)
> application will be adversely affected by this fix.

I don't think I asked for something so complicated and hard to
accomplish that it would justify partial fixes on master.  I'm very
uneasy with having the list of hard-coded events we ignore for this
purpose continue to grow one event at a time.  Making this controlled
by a Lisp variable will make its future maintenance much easier,
including when someone, perhaps you, comes up complaining about yet
another obscure event.

Thanks.

bug#19547: Patch for this bug

[Reuben Thomas]

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

On 9 November 2016 at 19:43, Eli Zaretskii <eliz@gnu.org> wrote:

>
> I don't think I asked for something so complicated and hard to
> accomplish that it would justify partial fixes on master.  I'm very
> uneasy with having the list of hard-coded events we ignore for this
> purpose continue to grow one event at a time.  Making this controlled
> by a Lisp variable will make its future maintenance much easier,
> including when someone, perhaps you, comes up complaining about yet
> another obscure event.
>

​I already showed I can't see what the correct solution is: I thought you
made a precise suggestion, which I discussed (a list of events for
input-pending-p to ignore), but then you said you did not mean anything so
precise.

So, if you could indicate a precise design that would be acceptable, I will
implement it. That seems to me a good medium between extending a hard fix
that I agree is inelegant, and my spending lots of time trying to
understand yet another area of Emacs.

-- 
http://rrt.sc3d.org

[-- Attachment #2: Type: text/html, Size: 1745 bytes --]

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Reuben Thomas <rrt@sc3d.org>
> Date: Wed, 9 Nov 2016 22:03:34 +0000
> Cc: 19547@debbugs.gnu.org
> 
> So, if you could indicate a precise design that would be acceptable, I will implement it. That seems to me a
> good medium between extending a hard fix that I agree is inelegant, and my spending lots of time trying to
> understand yet another area of Emacs.

How about the following:

Use DEFVAR_LISP to define a Lisp variable with initial value of Qnil.

In some preloaded Lisp file, say subr.el, give that variable a value,
a list of symbols that name the events we currently don't consider
relevant for throw-on-input.  (Those symbols will have to be invented,
e.g. 'focus-in' for FOCUS_IN_EVENT, etc.)

In kbd_buffer_store_buffered_event, where we set Vquit_flag depending
on Vthrow_on_input, examine the value of that list variable, and
ignore any events that have their symbols in the list.

Thanks.

bug#19547: Patch for this bug

[Thierry Volpiatto]

As no body is working on this, I started looking at this as I would like
to have a fix for this for helm, however I have difficulties as my C
level is 0.
Could you please bear with me on some points ?

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Reuben Thomas <rrt@sc3d.org>
>> Date: Wed, 9 Nov 2016 22:03:34 +0000
>> Cc: 19547@debbugs.gnu.org
>> 
>> So, if you could indicate a precise design that would be acceptable, I will implement it. That seems to me a
>> good medium between extending a hard fix that I agree is inelegant, and my spending lots of time trying to
>> understand yet another area of Emacs.
>
> How about the following:
>
> Use DEFVAR_LISP to define a Lisp variable with initial value of Qnil.

Ok for this one (I think).

> In some preloaded Lisp file, say subr.el, give that variable a value,
> a list of symbols that name the events we currently don't consider
> relevant for throw-on-input.

Ok.

> (Those symbols will have to be invented, e.g. 'focus-in' for
> FOCUS_IN_EVENT, etc.)

It is here I don't understand, how do I make the correspondance with e.g
focus-in (the lisp symbol) and FOCUS_IN_EVENT ?

> In kbd_buffer_store_buffered_event, where we set Vquit_flag depending
> on Vthrow_on_input, examine the value of that list variable, and
> ignore any events that have their symbols in the list.

Ok, well you will tell me when I send patch...

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Thierry Volpiatto <thierry.volpiatto@gmail.com>
> Cc: Reuben Thomas <rrt@sc3d.org>,  19547@debbugs.gnu.org
> Date: Fri, 25 Nov 2016 18:10:15 +0100
> 
> As no body is working on this, I started looking at this as I would like
> to have a fix for this for helm, however I have difficulties as my C
> level is 0.
> Could you please bear with me on some points ?

Sure, and thanks for working on this.

> > In some preloaded Lisp file, say subr.el, give that variable a value,
> > a list of symbols that name the events we currently don't consider
> > relevant for throw-on-input.
> 
> Ok.
> 
> > (Those symbols will have to be invented, e.g. 'focus-in' for
> > FOCUS_IN_EVENT, etc.)
> 
> It is here I don't understand, how do I make the correspondance with e.g
> focus-in (the lisp symbol) and FOCUS_IN_EVENT ?

You can do that with a 'switch' in C, or, better, with a C array that
holds the symbols and their corresponding C event_kind values, like
this:

 struct event_value {
   Lisp_Object event_symbol;
   enum event_kind event_kind;
 };
 static struct event_value symbol_to_kind[] = {
   { Qfocus_in, FOCUS_IN_EVENT },
   { Qhelp, HELP_EVENT },
   { Qiconify, ICONIFY_EVENT },
   ...
 };

Then, for each symbol, you can find the corresponding event value by
walking the array until you find a match.

bug#19547: Patch for this bug

[Andreas Schwab]

On Nov 26 2016, Eli Zaretskii <eliz@gnu.org> wrote:

>  struct event_value {
>    Lisp_Object event_symbol;
>    enum event_kind event_kind;
>  };
>  static struct event_value symbol_to_kind[] = {
>    { Qfocus_in, FOCUS_IN_EVENT },
>    { Qhelp, HELP_EVENT },
>    { Qiconify, ICONIFY_EVENT },

Qfoo isn't a compile time constant, so you cannot use it for
initializing a static variable in C.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Andreas Schwab <schwab@linux-m68k.org>
> Cc: Thierry Volpiatto <thierry.volpiatto@gmail.com>,  rrt@sc3d.org,  19547@debbugs.gnu.org
> Date: Sat, 26 Nov 2016 09:39:17 +0100
> 
> On Nov 26 2016, Eli Zaretskii <eliz@gnu.org> wrote:
> 
> >  struct event_value {
> >    Lisp_Object event_symbol;
> >    enum event_kind event_kind;
> >  };
> >  static struct event_value symbol_to_kind[] = {
> >    { Qfocus_in, FOCUS_IN_EVENT },
> >    { Qhelp, HELP_EVENT },
> >    { Qiconify, ICONIFY_EVENT },
> 
> Qfoo isn't a compile time constant, so you cannot use it for
> initializing a static variable in C.

OK, then this array should be filled at startup time.

bug#19547: Patch for this bug

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

> You can do that with a 'switch' in C, or, better, with a C array that
> holds the symbols and their corresponding C event_kind values, like
> this:
>
>  struct event_value {
>    Lisp_Object event_symbol;
>    enum event_kind event_kind;
>  };
>  static struct event_value symbol_to_kind[] = {
>    { Qfocus_in, FOCUS_IN_EVENT },
>    { Qhelp, HELP_EVENT },
>    { Qiconify, ICONIFY_EVENT },
>    ...
>  };
>
> Then, for each symbol, you can find the corresponding event value by
> walking the array until you find a match.

Thanks, I will try to learn how to do this with both methods, only able
to produce errors for now :-)

I tried using Fmemq to check if event (translated with switch) is in new
variable Vwhile_no_input_ignore_events, but without success.

Will tell you as soon as I can end up with something more or less
usable.

Thanks again.

-- 
Thierry

bug#19547: Patch for this bug

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

> You can do that with a 'switch' in C, or, better, with a C array that
> holds the symbols and their corresponding C event_kind values, like
> this:
>
>  struct event_value {
>    Lisp_Object event_symbol;
>    enum event_kind event_kind;
>  };
>  static struct event_value symbol_to_kind[] = {
>    { Qfocus_in, FOCUS_IN_EVENT },
>    { Qhelp, HELP_EVENT },
>    { Qiconify, ICONIFY_EVENT },
>    ...
>  };
>
> Then, for each symbol, you can find the corresponding event value by
> walking the array until you find a match.

I finally wrote a patch for keyboard.c which is compiling without error,
I used switch because I still not understanding how to use the code
above (seems that is approach is too advanced for my skills compared to
the usage of switch).
The patch seems to work fine with helm, though i didn't test with our
bug (https://github.com/emacs-helm/helm/issues/1157), but I now don't
know where to initialize our variable (while-no-input-ignore-events), I
just setq it in scratch buffer after starting helm with a minimal
installation (script emacs-helm.sh).

Here the patch (please look at it carefully this is the first C code I write):

diff --git a/src/keyboard.c b/src/keyboard.c
index 65938a5..3caf506 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -3567,14 +3567,22 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
 #endif	/* subprocesses */
     }
 
+  Lisp_Object ignore_event;
+
+  switch (event->kind)
+    {
+    case FOCUS_IN_EVENT: ignore_event = Qfocus_in;
+    case FOCUS_OUT_EVENT: ignore_event = Qfocus_out;
+    case HELP_EVENT: ignore_event = Qhelp;
+    case ICONIFY_EVENT: ignore_event = Qiconify;
+    case DEICONIFY_EVENT: ignore_event = Qdeiconify;
+    case SELECTION_REQUEST_EVENT: ignore_event = Qselection_request;
+    }
+
   /* If we're inside while-no-input, and this event qualifies
      as input, set quit-flag to cause an interrupt.  */
   if (!NILP (Vthrow_on_input)
-      && event->kind != FOCUS_IN_EVENT
-      && event->kind != FOCUS_OUT_EVENT
-      && event->kind != HELP_EVENT
-      && event->kind != ICONIFY_EVENT
-      && event->kind != DEICONIFY_EVENT)
+      && !NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
     {
       Vquit_flag = Vthrow_on_input;
       /* If we're inside a function that wants immediate quits,
@@ -11164,6 +11172,10 @@ syms_of_keyboard (void)
   DEFSYM (Qiconify_frame, "iconify-frame");
   DEFSYM (Qmake_frame_visible, "make-frame-visible");
   DEFSYM (Qselect_window, "select-window");
+  DEFSYM (Qhelp, "help");
+  DEFSYM (Qiconify, "iconify");
+  DEFSYM (Qdeiconify, "deiconify");
+  DEFSYM (Qselection_request, "selection-request");
   {
     int i;
 
@@ -11822,6 +11834,12 @@ signals.  */);
 
   /* Create the initial keyboard.  Qt means 'unset'.  */
   initial_kboard = allocate_kboard (Qt);
+
+  DEFVAR_LISP ("while-no-input-ignore-events",
+               Vwhile_no_input_ignore_events,
+               doc: /* Ignored events from while-no-input.  */);
+  Vwhile_no_input_ignore_events = Qnil;
+    /* = listn (Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request); */
 }
 
 void


Thanks.

-- 
Thierry

bug#19547: Patch for this bug

[npostavs]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>  
> +  Lisp_Object ignore_event;
> +
> +  switch (event->kind)
> +    {
> +    case FOCUS_IN_EVENT: ignore_event = Qfocus_in;
> +    case FOCUS_OUT_EVENT: ignore_event = Qfocus_out;
> +    case HELP_EVENT: ignore_event = Qhelp;
> +    case ICONIFY_EVENT: ignore_event = Qiconify;
> +    case DEICONIFY_EVENT: ignore_event = Qdeiconify;
> +    case SELECTION_REQUEST_EVENT: ignore_event = Qselection_request;

You need a break at the end of each case, otherwise all events would be
treated as SELECTION_REQUEST_EVENT.

    case FOCUS_IN_EVENT: ignore_event = Qfocus_in; break;

> +  Vwhile_no_input_ignore_events = Qnil;
> +    /* = listn (Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request); */

I think something like this should work:

    listn (CONSTYPE_PURE, 6, Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request);

bug#19547: Patch for this bug

[Thierry Volpiatto]

npostavs@users.sourceforge.net writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
>>  
>> +  Lisp_Object ignore_event;
>> +
>> +  switch (event->kind)
>> +    {
>> +    case FOCUS_IN_EVENT: ignore_event = Qfocus_in;
>> +    case FOCUS_OUT_EVENT: ignore_event = Qfocus_out;
>> +    case HELP_EVENT: ignore_event = Qhelp;
>> +    case ICONIFY_EVENT: ignore_event = Qiconify;
>> +    case DEICONIFY_EVENT: ignore_event = Qdeiconify;
>> +    case SELECTION_REQUEST_EVENT: ignore_event = Qselection_request;
>
> You need a break at the end of each case, otherwise all events would be
> treated as SELECTION_REQUEST_EVENT.
>
>     case FOCUS_IN_EVENT: ignore_event = Qfocus_in; break;

You answer exactly at what I was wondering about, is break needed or not?


>> +  Vwhile_no_input_ignore_events = Qnil;
>> +    /* = listn (Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request); */
>
> I think something like this should work:
>
>     listn (CONSTYPE_PURE, 6, Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request);

Same here, I had to use Qnil because I always had errors when trying to
feed this variable here, I will try tonight if that works.
I wondered also if instead the variable could be feeded from lisp as
suggested by Eli, but I don't know yet where is the good place for this
and how.

Many thanks for your help.

-- 
Thierry

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Thierry Volpiatto <thierry.volpiatto@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 19547@debbugs.gnu.org, rrt@sc3d.org
> Date: Sun, 27 Nov 2016 15:53:58 +0100
> 
> 
> npostavs@users.sourceforge.net writes:
> 
> > Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
> >>  
> >> +  Lisp_Object ignore_event;
> >> +
> >> +  switch (event->kind)
> >> +    {
> >> +    case FOCUS_IN_EVENT: ignore_event = Qfocus_in;
> >> +    case FOCUS_OUT_EVENT: ignore_event = Qfocus_out;
> >> +    case HELP_EVENT: ignore_event = Qhelp;
> >> +    case ICONIFY_EVENT: ignore_event = Qiconify;
> >> +    case DEICONIFY_EVENT: ignore_event = Qdeiconify;
> >> +    case SELECTION_REQUEST_EVENT: ignore_event = Qselection_request;
> >
> > You need a break at the end of each case, otherwise all events would be
> > treated as SELECTION_REQUEST_EVENT.
> >
> >     case FOCUS_IN_EVENT: ignore_event = Qfocus_in; break;
> 
> You answer exactly at what I was wondering about, is break needed or not?

In addition to using 'break', you need to initialize ignore_event with
some value, otherwise it will hold garbage, which could accidentally
be one of the values you want to filter.  Alternatively, add a
'default' case to the switch.

> >> +  Vwhile_no_input_ignore_events = Qnil;
> >> +    /* = listn (Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request); */
> >
> > I think something like this should work:
> >
> >     listn (CONSTYPE_PURE, 6, Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request);
> 
> Same here, I had to use Qnil because I always had errors when trying to
> feed this variable here, I will try tonight if that works.

I'd suggest to use Flist instead, it should be closer to Lisp (if you
want to do this from C).

> I wondered also if instead the variable could be feeded from lisp as
> suggested by Eli, but I don't know yet where is the good place for this
> and how.

Something like simple.el should be fine.

Thanks.

bug#19547: Patch for this bug

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

> In addition to using 'break', you need to initialize ignore_event with
> some value, otherwise it will hold garbage, which could accidentally
> be one of the values you want to filter.  Alternatively, add a
> 'default' case to the switch.

Is Qnil ok as default value ?

> I'd suggest to use Flist instead, it should be closer to Lisp (if you
> want to do this from C).

Didn't succeed with Flist:

keyboard.c:11842:5: error: too many arguments to function ‘Flist’
     = Flist (Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request)
     ^

>> I wondered also if instead the variable could be feeded from lisp as
>> suggested by Eli, but I don't know yet where is the good place for this
>> and how.
>
> Something like simple.el should be fine.

I would prefer setting the variable from lisp.
ok for simple.el, but what is the recommended way for setting it (I
don't think throwing a (setq while-no-input-ignore-events [...])
anywhere in simple.el is the way to do) ?

Also on my laptop with gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3)
I have no warnings, but on an other computer with gcc version 5.4.0
20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4), I have these warnings (but it
compiled fine):

keyboard.c: In function ‘kbd_buffer_store_buffered_event’:
keyboard.c:3572:3: warning: enumeration value ‘NO_EVENT’ not handled in switch [-Wswitch]
   switch (event->kind)
   ^
keyboard.c:3572:3: warning: enumeration value ‘ASCII_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘MULTIBYTE_CHAR_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘NON_ASCII_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘TIMER_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘MOUSE_CLICK_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘WHEEL_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘HORIZ_WHEEL_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘SCROLL_BAR_CLICK_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘HORIZONTAL_SCROLL_BAR_CLICK_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘SELECTION_CLEAR_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘BUFFER_SWITCH_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘DELETE_WINDOW_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘MENU_BAR_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘MENU_BAR_ACTIVATE_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘DRAG_N_DROP_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘USER_SIGNAL_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘TOOL_BAR_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘SELECT_WINDOW_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘SAVE_SESSION_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘GPM_CLICK_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘DBUS_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘CONFIG_CHANGED_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3572:3: warning: enumeration value ‘FILE_NOTIFY_EVENT’ not handled in switch [-Wswitch]
keyboard.c:3585:11: warning: ‘ignore_event’ may be used uninitialized in this function [-Wmaybe-uninitialized]
       && !NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
           ^

What can I do to avoid this ? Or should I just ignore this ?

Thanks.

-- 
Thierry

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Thierry Volpiatto <thierry.volpiatto@gmail.com>
> Cc: npostavs@users.sourceforge.net, 19547@debbugs.gnu.org, rrt@sc3d.org
> Date: Sun, 27 Nov 2016 18:59:37 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > In addition to using 'break', you need to initialize ignore_event with
> > some value, otherwise it will hold garbage, which could accidentally
> > be one of the values you want to filter.  Alternatively, add a
> > 'default' case to the switch.
> 
> Is Qnil ok as default value ?

Yes, it means no events are filtered out initially.

> > I'd suggest to use Flist instead, it should be closer to Lisp (if you
> > want to do this from C).
> 
> Didn't succeed with Flist:
> 
> keyboard.c:11842:5: error: too many arguments to function ‘Flist’
>      = Flist (Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request)
>      ^

My advice is to look at how a function is used elsewhere in the
sources, before using it in your code.  In this case, you will find
stuff like this:

	      arg = Flist (nargs - i, &arg_vector[i]);

IOW, the first argument to Flist is the number of arguments, and the
second argument is an array of that dimension.

> >> I wondered also if instead the variable could be feeded from lisp as
> >> suggested by Eli, but I don't know yet where is the good place for this
> >> and how.
> >
> > Something like simple.el should be fine.
> 
> I would prefer setting the variable from lisp.
> ok for simple.el, but what is the recommended way for setting it (I
> don't think throwing a (setq while-no-input-ignore-events [...])
> anywhere in simple.el is the way to do) ?

Why not?  You will see quite a few examples of such stuff there.
E.g.:

  (setq undo-outer-limit-function 'undo-outer-limit-truncate)

> Also on my laptop with gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3)
> I have no warnings, but on an other computer with gcc version 5.4.0
> 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4), I have these warnings (but it
> compiled fine):
> 
> keyboard.c: In function ‘kbd_buffer_store_buffered_event’:
> keyboard.c:3572:3: warning: enumeration value ‘NO_EVENT’ not handled in switch [-Wswitch]
>    switch (event->kind)
>    ^
> keyboard.c:3572:3: warning: enumeration value ‘ASCII_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘MULTIBYTE_CHAR_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]

That's because you don't have a "case default".  You should, as I
explained earlier.

> keyboard.c:3585:11: warning: ‘ignore_event’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>        && !NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
>            ^
> 
> What can I do to avoid this ?

Like I said: initialize ignore_event and add a "case default".

> Or should I just ignore this ?

Not a good idea, at least as long as you don't feel at home with C
programming.

Thanks.

bug#19547: Patch for this bug

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

> My advice is to look at how a function is used elsewhere in the
> sources, before using it in your code.  In this case, you will find
> stuff like this:
>
> 	      arg = Flist (nargs - i, &arg_vector[i]);
>
> IOW, the first argument to Flist is the number of arguments, and the
> second argument is an array of that dimension.

Ok, I understand now why there is also a warning about 2 args or
something like this, great, thanks for explanations.

> Why not?  You will see quite a few examples of such stuff there.

Ok no problem, 2 last questions:

- What about like suggested by Noam to put this just above the
  definition of while-no-input in subr.el ?

- IIRC you didn't want to add by default selection-request aka
  SELECTION_REQUEST_EVENT, is this always true ?

>> keyboard.c: In function ‘kbd_buffer_store_buffered_event’:
>> keyboard.c:3572:3: warning: enumeration value ‘NO_EVENT’ not handled in switch [-Wswitch]
>>    switch (event->kind)
>>    ^
>> keyboard.c:3572:3: warning: enumeration value ‘ASCII_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]
>> keyboard.c:3572:3: warning: enumeration value ‘MULTIBYTE_CHAR_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]
>
> That's because you don't have a "case default".  You should, as I
> explained earlier.

Ok so this should be fixed now.

Thanks.

-- 
Thierry

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Thierry Volpiatto <thierry.volpiatto@gmail.com>
> Cc: npostavs@users.sourceforge.net, 19547@debbugs.gnu.org, rrt@sc3d.org
> Date: Sun, 27 Nov 2016 20:03:10 +0100
> 
> > Why not?  You will see quite a few examples of such stuff there.
> 
> Ok no problem, 2 last questions:
> 
> - What about like suggested by Noam to put this just above the
>   definition of while-no-input in subr.el ?

No objections here.

> - IIRC you didn't want to add by default selection-request aka
>   SELECTION_REQUEST_EVENT, is this always true ?

I had no firm opinion on that.  However, since it will be so easy to
add and remove events to/from the list, I see no problem in having
SELECTION_REQUEST_EVENT there by default: an application that doesn't
want that can easily have what it wants.

bug#19547: Patch for this bug

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

> No objections here.
>
>> - IIRC you didn't want to add by default selection-request aka
>>   SELECTION_REQUEST_EVENT, is this always true ?
>
> I had no firm opinion on that.  However, since it will be so easy to
> add and remove events to/from the list, I see no problem in having
> SELECTION_REQUEST_EVENT there by default: an application that doesn't
> want that can easily have what it wants.

Ok, good, can I push changes on master ?

-- 
Thierry

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Thierry Volpiatto <thierry.volpiatto@gmail.com>
> Cc: npostavs@users.sourceforge.net, 19547@debbugs.gnu.org, rrt@sc3d.org
> Date: Sun, 27 Nov 2016 20:54:00 +0100
> 
> Ok, good, can I push changes on master ?

I think this variable should be mentioned in the ELisp manual.  And in
NEWS.  Other than that, yes.

Thanks.

bug#19547: Patch for this bug

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

> I think this variable should be mentioned in the ELisp manual.

Done.

> And in NEWS.

Done.

> Other than that, yes.

Ok I pushed changes.

Many thanks to you and Noam for your help.

Let me know if something wrong or missing, I will correct.

-- 
Thierry

bug#19547: Patch for this bug

[Thierry Volpiatto]

Eli Zaretskii <eliz@gnu.org> writes:

> Other than that, yes.

Eli, I did a big mistake in this patch, I used !NILP instead of NILP.

!NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))

should be instead:

NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))

No objection to push the fix ?

-- 
Thierry

bug#19547: Patch for this bug

[Eli Zaretskii]

> From: Thierry Volpiatto <thierry.volpiatto@gmail.com>
> Cc: npostavs@users.sourceforge.net, 19547@debbugs.gnu.org, rrt@sc3d.org
> Date: Wed, 30 Nov 2016 21:27:04 +0100
> 
> !NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
> 
> should be instead:
> 
> NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
> 
> No objection to push the fix ?

How can I object to fixing a mistake?  Go ahead, of course.

bug#19547: Patch for this bug

[npostavs]

tags 19547 fixed
close 19547 26.1
quit

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Thierry Volpiatto <thierry.volpiatto@gmail.com>
>> Cc: npostavs@users.sourceforge.net, 19547@debbugs.gnu.org, rrt@sc3d.org
>> Date: Wed, 30 Nov 2016 21:27:04 +0100
>> 
>> !NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
>> 
>> should be instead:
>> 
>> NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
>> 
>> No objection to push the fix ?
>
> How can I object to fixing a mistake?  Go ahead, of course.

Relevant commits are [319bafc9b2], [d9dd884c7c], and [43ec6efa2b].

[d9dd884c7c]: 2016-11-27 21:48:07 +0100
  Allow configuring which event throw-on-input should ignore (bug#19547).
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=d9dd884c7c1940cacfcc2d86d47220b40c520bb5
[43ec6efa2b]: 2016-11-28 06:59:49 +0100
  Reuse already existing lisp symbols for ignore_event (bug#19547).
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=43ec6efa2b41b43a2e55be16434f64bba644271e
[319bafc9b2]: 2016-11-30 21:22:04 +0100
  Fix Condition in kbd_buffer_store_buffered_event (bug#19547).
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=319bafc9b28bd5bffb92a97a8ab53b9a3b97e6fd

bug#19547: Patch for this bug

[npostavs]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> In addition to using 'break', you need to initialize ignore_event with
>> some value, otherwise it will hold garbage, which could accidentally
>> be one of the values you want to filter.  Alternatively, add a
>> 'default' case to the switch.
>
> Is Qnil ok as default value ?
>
>> I'd suggest to use Flist instead, it should be closer to Lisp (if you
>> want to do this from C).
>
> Didn't succeed with Flist:
>
> keyboard.c:11842:5: error: too many arguments to function ‘Flist’
>      = Flist (Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request)
>      ^

You have to use CALLN for functions taking a non-fixed amount of arguments:

    CALLN (Flist, Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request);

>
>>> I wondered also if instead the variable could be feeded from lisp as
>>> suggested by Eli, but I don't know yet where is the good place for this
>>> and how.
>>
>> Something like simple.el should be fine.
>
> I would prefer setting the variable from lisp.
> ok for simple.el, but what is the recommended way for setting it (I
> don't think throwing a (setq while-no-input-ignore-events [...])
> anywhere in simple.el is the way to do) ?

I think that should be fine.  Though maybe in subr.el next to the
definition of `while-no-input' would be a better place?

>
> Also on my laptop with gcc version 4.8.4 (Ubuntu 4.8.4-2ubuntu1~14.04.3)
> I have no warnings, but on an other computer with gcc version 5.4.0
> 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4), I have these warnings (but it
> compiled fine):

That should be fixed by adding a default case as Eli suggested, but does
it make sense to handle any of those other events?

>
> keyboard.c: In function ‘kbd_buffer_store_buffered_event’:
> keyboard.c:3572:3: warning: enumeration value ‘NO_EVENT’ not handled in switch [-Wswitch]
>    switch (event->kind)
>    ^
> keyboard.c:3572:3: warning: enumeration value ‘ASCII_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘MULTIBYTE_CHAR_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘NON_ASCII_KEYSTROKE_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘TIMER_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘MOUSE_CLICK_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘WHEEL_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘HORIZ_WHEEL_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘SCROLL_BAR_CLICK_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘HORIZONTAL_SCROLL_BAR_CLICK_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘SELECTION_CLEAR_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘BUFFER_SWITCH_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘DELETE_WINDOW_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘MENU_BAR_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘MENU_BAR_ACTIVATE_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘DRAG_N_DROP_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘USER_SIGNAL_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘TOOL_BAR_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘SELECT_WINDOW_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘SAVE_SESSION_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘GPM_CLICK_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘DBUS_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘CONFIG_CHANGED_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3572:3: warning: enumeration value ‘FILE_NOTIFY_EVENT’ not handled in switch [-Wswitch]
> keyboard.c:3585:11: warning: ‘ignore_event’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>        && !NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
>            ^
>
> What can I do to avoid this ? Or should I just ignore this ?
>
> Thanks.

bug#19547: Patch for this bug

[Thierry Volpiatto]

npostavs@users.sourceforge.net writes:

> Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:
> You have to use CALLN for functions taking a non-fixed amount of arguments:
>
>     CALLN (Flist, Qfocus_in, Qfocus_out, Qhelp, Qiconify, Qdeiconify, Qselection_request);

Great, good to know as alternative in addition of Eli's explanation.
Thanks.

> I think that should be fine.  Though maybe in subr.el next to the
> definition of `while-no-input' would be a better place?

Yes, I like this.

> That should be fixed by adding a default case as Eli suggested,

Yes done.

> but does it make sense to handle any of those other events?

Don't know why debugger mentionned those events, I never add any of
them.

Thanks.

-- 
Thierry

bug#19547: Patch for this bug

[Thierry Volpiatto]

I have installed Parcellite (a clipboard manager).
When it is running I can reproduce the bug at every time when running
helm-find-files on a very large directory, but with the patch applied
and 
(setq while-no-input-ignore-events '(focus-in focus-out help iconify
deiconify selection-request))

it is now working as expected, the listing of directory popup
immediately.

-- 
Thierry

bug#19547: Patch for this bug

[Reuben Thomas]

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

On 25 November 2016 at 17:10, Thierry Volpiatto <thierry.volpiatto@gmail.com
> wrote:

>
> As no body is working on this, I started looking at this as I would like
> to have a fix for this for helm


​This was still on my to-do list, but thanks very much, Thierry, for taking
over.​

[-- Attachment #2: Type: text/html, Size: 725 bytes --]

bug#19547: Patch for this bug

[Thierry Volpiatto]

Reuben Thomas <rrt@sc3d.org> writes:

> ​This was still on my to-do list, but thanks very much, Thierry, for taking
> over.​

You are welcome ;-) Don't hesitate to correct my poor C code, thanks.

So here the final patch I could push if Eli is ok after looking the
comments in previous mail; I had prefered also adding the changes to
simple.el in same commit when i know how to do, basically it is just how/where
to add:

(setq while-no-input-ignore-events 
      '(focus-in focus-out help iconify deiconify selection-request))


Here the last patch:

1 file changed, 22 insertions(+), 5 deletions(-)
src/keyboard.c | 27 ++++++++++++++++++++++-----

modified   src/keyboard.c
@@ -3567,14 +3567,22 @@ kbd_buffer_store_buffered_event (union buffered_input_event *event,
 #endif	/* subprocesses */
     }
 
+  Lisp_Object ignore_event = Qnil;
+
+  switch (event->kind)
+    {
+    case FOCUS_IN_EVENT: ignore_event = Qfocus_in; break;
+    case FOCUS_OUT_EVENT: ignore_event = Qfocus_out; break;
+    case HELP_EVENT: ignore_event = Qhelp; break;
+    case ICONIFY_EVENT: ignore_event = Qiconify; break;
+    case DEICONIFY_EVENT: ignore_event = Qdeiconify; break;
+    case SELECTION_REQUEST_EVENT: ignore_event = Qselection_request; break;
+    }
+
   /* If we're inside while-no-input, and this event qualifies
      as input, set quit-flag to cause an interrupt.  */
   if (!NILP (Vthrow_on_input)
-      && event->kind != FOCUS_IN_EVENT
-      && event->kind != FOCUS_OUT_EVENT
-      && event->kind != HELP_EVENT
-      && event->kind != ICONIFY_EVENT
-      && event->kind != DEICONIFY_EVENT)
+      && !NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
     {
       Vquit_flag = Vthrow_on_input;
       /* If we're inside a function that wants immediate quits,
@@ -11164,6 +11172,10 @@ syms_of_keyboard (void)
   DEFSYM (Qiconify_frame, "iconify-frame");
   DEFSYM (Qmake_frame_visible, "make-frame-visible");
   DEFSYM (Qselect_window, "select-window");
+  DEFSYM (Qhelp, "help");
+  DEFSYM (Qiconify, "iconify");
+  DEFSYM (Qdeiconify, "deiconify");
+  DEFSYM (Qselection_request, "selection-request");
   {
     int i;
 
@@ -11822,6 +11834,11 @@ signals.  */);
 
   /* Create the initial keyboard.  Qt means 'unset'.  */
   initial_kboard = allocate_kboard (Qt);
+
+  DEFVAR_LISP ("while-no-input-ignore-events",
+               Vwhile_no_input_ignore_events,
+               doc: /* Ignored events from while-no-input.  */);
+  Vwhile_no_input_ignore_events = Qnil;
 }
 
 void

-- 
Thierry

bug#19547: Patch for this bug

[Johan Bockgård]

Thierry Volpiatto <thierry.volpiatto@gmail.com> writes:

> +    case FOCUS_IN_EVENT: ignore_event = Qfocus_in; break;
> +    case FOCUS_OUT_EVENT: ignore_event = Qfocus_out; break;
> +    case HELP_EVENT: ignore_event = Qhelp; break;
> +    case ICONIFY_EVENT: ignore_event = Qiconify; break;
> +    case DEICONIFY_EVENT: ignore_event = Qdeiconify; break;
> +    case SELECTION_REQUEST_EVENT: ignore_event = Qselection_request; break;
> +    }
> +
>    /* If we're inside while-no-input, and this event qualifies
>       as input, set quit-flag to cause an interrupt.  */
>    if (!NILP (Vthrow_on_input)
> -      && event->kind != FOCUS_IN_EVENT
> -      && event->kind != FOCUS_OUT_EVENT
> -      && event->kind != HELP_EVENT
> -      && event->kind != ICONIFY_EVENT
> -      && event->kind != DEICONIFY_EVENT)
> +      && !NILP (Fmemq (ignore_event, Vwhile_no_input_ignore_events)))
>      {
>        Vquit_flag = Vthrow_on_input;
>        /* If we're inside a function that wants immediate quits,
> @@ -11164,6 +11172,10 @@ syms_of_keyboard (void)
>    DEFSYM (Qiconify_frame, "iconify-frame");
>    DEFSYM (Qmake_frame_visible, "make-frame-visible");
>    DEFSYM (Qselect_window, "select-window");
> +  DEFSYM (Qhelp, "help");
> +  DEFSYM (Qiconify, "iconify");
> +  DEFSYM (Qdeiconify, "deiconify");
> +  DEFSYM (Qselection_request, "selection-request");

HELP_EVENT, ICONIFY_EVENT and DEICONIFY_EVENT already have the Lispy
names `help-echo', `iconify-frame' and `make-frame-visible',
respectively, so I think we should use those instead of inventing new
symbols.

bug#19547: Patch for this bug

[Thierry Volpiatto]

Johan Bockgård <bojohan@gnu.org> writes:

> HELP_EVENT, ICONIFY_EVENT and DEICONIFY_EVENT already have the Lispy
> names `help-echo', `iconify-frame' and `make-frame-visible',
> respectively, so I think we should use those instead of inventing new
> symbols.

Done, thanks.

-- 
Thierry