Re: [2/3] winex11: Returning more reliable value from X11DRV_MsgWaitForMultipleObjects.

[Ken Thomases]

Hi,

On Aug 23, 2016, at 8:13 AM, Akihiro Sagawa <sagawa.aki(a)gmail.com> wrote:
> This patch fixes ole32 test failure with my IME window patch.
>
> After processing several X events, X11DRV_MsgWaitForMultipleObjects always
> tells us that a new message is available. This is not true for some cases.
>
> For instance, when we call DestroyWindow, the X queues DestroyEvent. Then,
> X11DRV_MsgWaitForMultipleObjects handles the event only; none is posted or
> sent as hwnd for destroyed window is unavailable. However, the function
> states "new message is available" by returning count - 1 value.
>
> This is an issue for CoWaitForMultipleHandles because it expects a new
> message in the queue and consumes the message.

I'm looking at this with an eye toward whether the Mac driver needs a similar change.

First, there's no change in the ole32 tests with or without your IME window patch when using the Mac driver.  There's a clipboard failure but everything else succeeds.

Second, the Mac driver doesn't have a notion of the window being destroyed behind our backs.  I.e. there's nothing like a DestroyNotify event for a window whose HWND is still valid.

But, just because that particular case doesn't come up doesn't mean the change is not still relevant to the Mac driver for other cases.  However, I'm having a hard time seeing the logic of your change.

In process_events(), you use a variable named "queued" to hold the result of calls to event handlers.  That implies that event handlers should only return true if they have posted or sent a Windows message or hardware input.  But that doesn't seem to be what you've actually done in the event handlers.  For example, in the first "if" statement in X11DRV_MapNotify(), you return true without queuing anything.

Instead, you seem to be returning true for some notion of "success" in handling events, but I'm not sure how that plays into the rationale for this patch.  Am I misunderstanding something?

I also don't get why you changed the TRACE at the end of process_events().  The events really were processed.  They aren't "ignored" just because they didn't result in messages or input being queued.  Also, shouldn't the return type of process_events() be changed to BOOL?

Thanks,
Ken

[2/3] winex11: Returning more reliable value from X11DRV_MsgWaitForMultipleObjects.

[Akihiro Sagawa]

On Thu, 25 Aug 2016 13:35:43 -0500, Ken Thomases wrote:
> Hi,
> Hi,
> (snip)
> In process_events(), you use a variable named "queued" to hold the result
> of calls to event handlers.  That implies that event handlers should
> only return true if they have posted or sent a Windows message or
> hardware input.  But that doesn't seem to be what you've actually done
> in the event handlers.  For example, in the first "if" statement in
> X11DRV_MapNotify(), you return true without queuing anything.
>
> Instead, you seem to be returning true for some notion of "success"
> in handling events, but I'm not sure how that plays into the rationale
> for this patch.  Am I misunderstanding something?
> Ideally, event handlers should return FALSE if they succeed and nothing
> is posted or sent by them. However, at that time, I was afraid of
> introducing regressions, I limited return FALSE case if the handler
> completely fails. So, It's not perfect, yet.
> In other words, my patch allows the handler states it never posts/sends
> a message by returning FALSE. But returning TRUE doesn't mean there is a
> new message.
> I also don't get why you changed the TRACE at the end of
> process_events().  The events really were processed.  They aren't
> "ignored" just because they didn't result in messages or input being
> queued.  Also, shouldn't the return type of process_events() be changed
> to BOOL?
> Good point. I'll sent a patch in this weekend.

Thanks too,
Akihiro Sagawa