Re: master 326fff4: Improve w32notify notifications

Re: master 326fff4: Improve w32notify notifications

[Michael Albinus]

Hi Fabrice,

> diff --git a/test/lisp/filenotify-tests.el b/test/lisp/filenotify-tests.el
> index d3610f0..3908894 100644
> --- a/test/lisp/filenotify-tests.el
> +++ b/test/lisp/filenotify-tests.el
> @@ -754,7 +757,9 @@ longer than timeout seconds for the events to be delivered."
>          (should (file-notify-valid-p file-notify--test-desc))
>          ;; After removing the watch, the descriptor must not be valid
>          ;; anymore.
> +        (read-event nil nil file-notify--test-read-event-timeout)
>          (file-notify-rm-watch file-notify--test-desc)
> +        (read-event nil nil file-notify--test-read-event-timeout)
>          (file-notify--wait-for-events
>           (file-notify--test-timeout)
>  	 (not (file-notify-valid-p file-notify--test-desc)))
> @@ -776,7 +781,9 @@ longer than timeout seconds for the events to be delivered."
>          (should (file-notify-valid-p file-notify--test-desc))
>          ;; After deleting the directory, the descriptor must not be
>          ;; valid anymore.
> +        (read-event nil nil file-notify--test-read-event-timeout)
>          (delete-directory file-notify--test-tmpfile t)
> +        (read-event nil nil file-notify--test-read-event-timeout)
>          (file-notify--wait-for-events
>  	 (file-notify--test-timeout)
>  	 (not (file-notify-valid-p file-notify--test-desc)))

I'm curious why you need the additional `read-event' calls. The
following `file-notify--wait-for-events' loops until the condition is
satisfied, calling `read-event' every iteration.

Best regards, Michael.

Re: master 326fff4: Improve w32notify notifications

[Fabrice Popineau]

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

2016-03-19 16:11 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:

> Hi Fabrice,
>
> > diff --git a/test/lisp/filenotify-tests.el
> b/test/lisp/filenotify-tests.el
> > index d3610f0..3908894 100644
> > --- a/test/lisp/filenotify-tests.el
> > +++ b/test/lisp/filenotify-tests.el
> > @@ -754,7 +757,9 @@ longer than timeout seconds for the events to be
> delivered."
> >          (should (file-notify-valid-p file-notify--test-desc))
> >          ;; After removing the watch, the descriptor must not be valid
> >          ;; anymore.
> > +        (read-event nil nil file-notify--test-read-event-timeout)
> >          (file-notify-rm-watch file-notify--test-desc)
> > +        (read-event nil nil file-notify--test-read-event-timeout)
> >          (file-notify--wait-for-events
> >           (file-notify--test-timeout)
> >        (not (file-notify-valid-p file-notify--test-desc)))
> > @@ -776,7 +781,9 @@ longer than timeout seconds for the events to be
> delivered."
> >          (should (file-notify-valid-p file-notify--test-desc))
> >          ;; After deleting the directory, the descriptor must not be
> >          ;; valid anymore.
> > +        (read-event nil nil file-notify--test-read-event-timeout)
> >          (delete-directory file-notify--test-tmpfile t)
> > +        (read-event nil nil file-notify--test-read-event-timeout)
> >          (file-notify--wait-for-events
> >        (file-notify--test-timeout)
> >        (not (file-notify-valid-p file-notify--test-desc)))
>
> I'm curious why you need the additional `read-event' calls. The
> following `file-notify--wait-for-events' loops until the condition is
> satisfied, calling `read-event' every iteration.
>
>
Oh ... sorry, yes, we should revert those read-event calls then.
I added them mechanically, overlooking the macro.

Fabrice

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

Re: master 326fff4: Improve w32notify notifications

[Michael Albinus]

Fabrice Popineau <fabrice.popineau@gmail.com> writes:

Hi Fabrice,

> Oh ... sorry, yes, we should revert those read-event calls then.
> I added them mechanically, overlooking the macro.

I could revert them, running also full tests on w32. If you could
provide me a compiled Emacs from master, again ...

> Fabrice

Best regards, Michael.

Re: master 326fff4: Improve w32notify notifications

[Michael Albinus]

Fabrice Popineau <fabrice.popineau@gmail.com> writes:

> This is the case for which I said I had a disagreement.
>
> Are you saying that you get 4 notifications where I get only 2 ?
> In this case, that must be a difference between Windows 7 and Windows
> 10,
> but that is weird.

Yes, I get 4 `changed' events. Running on 64-bit "Windows 7 Enterprise SP1".

Hmm, what shall we do? Allow both variants, and document it in the test file?

> Fabrice

Best regards, Michael.

Re: master 326fff4: Improve w32notify notifications

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Thu, 24 Mar 2016 14:16:13 +0100
> Cc: emacs-devel@gnu.org
> 
> Fabrice Popineau <fabrice.popineau@gmail.com> writes:
> 
> > This is the case for which I said I had a disagreement.
> >
> > Are you saying that you get 4 notifications where I get only 2 ?
> > In this case, that must be a difference between Windows 7 and Windows
> > 10,
> > but that is weird.
> 
> Yes, I get 4 `changed' events. Running on 64-bit "Windows 7 Enterprise SP1".
> 
> Hmm, what shall we do? Allow both variants, and document it in the test file?

Yes, I think so.

Thanks.

Re: master 326fff4: Improve w32notify notifications

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Hmm, what shall we do? Allow both variants, and document it in the test file?
>
> Yes, I think so.

Done.

> Thanks.

Best regards, Michael.

Re: master 326fff4: Improve w32notify notifications

[Andy Moreton]

On Mon 21 Mar 2016, Fabrice Popineau wrote:

> 2016-03-19 16:11 GMT+01:00 Michael Albinus <michael.albinus@gmx.de>:
>>
>> I'm curious why you need the additional `read-event' calls. The
>> following `file-notify--wait-for-events' loops until the condition is
>> satisfied, calling `read-event' every iteration.
>>
> Oh ... sorry, yes, we should revert those read-event calls then.
> I added them mechanically, overlooking the macro.
>
> Fabrice

Hi Fabrice,

The master 326fff4 commit also broke the cygwin w32 (i.e. non-X11)
build. The following trivial patch fixes it.

    AndyM

diff --git a/src/w32xfns.c b/src/w32xfns.c
index 9b633c4..9a10bf3 100644
--- a/src/w32xfns.c
+++ b/src/w32xfns.c
@@ -48,6 +48,7 @@ init_crit (void)
      when the input queue is empty, so make it a manual reset event. */
   input_available = CreateEvent (NULL, TRUE, FALSE, NULL);
 
+#if HAVE_W32NOTIFY
   /* Initialize the linked list of notifications sets that will be
      used to communicate between the watching worker threads and the
      main thread.  */
@@ -60,6 +61,7 @@ init_crit (void)
     }
   else
     DebPrint(("Out of memory: can't initialize notifications sets."));
+#endif /* HAVE_W32NOTIFY */
 
 #ifdef WINDOWSNT
   keyboard_handle = input_available;
@@ -90,6 +92,7 @@ delete_crit (void)
       interrupt_handle = NULL;
     }
 
+#if HAVE_W32NOTIFY
   if (notifications_set_head)
     {
       /* Free any remaining notifications set that could be left over.  */
@@ -104,6 +107,7 @@ delete_crit (void)
 	}
     }
   free (notifications_set_head);
+#endif /* HAVE_W32NOTIFY */
 }
 
 void

Re: master 326fff4: Improve w32notify notifications

[Eli Zaretskii]

> From: Andy Moreton <andrewjmoreton@gmail.com>
> Date: Tue, 29 Mar 2016 15:31:38 +0100
> 
> Hi Fabrice,
> 
> The master 326fff4 commit also broke the cygwin w32 (i.e. non-X11)
> build. The following trivial patch fixes it.

Thanks, pushed.  (I wish someone would volunteer to install such
trivial changes, though.)

Re: master 326fff4: Improve w32notify notifications

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

> Thanks, pushed.  (I wish someone would volunteer to install such
> trivial changes, though.)

I would do, and I did in other cases, if I understand the
patch. Unfortunately, src/w32xfns.c is out of my scope. And I fear I'm
not the only one.

Anyway, if I could help in whatever is trivial, just ping me.

Best regards, Michael.