On 05/10/2013 09:11 AM, Alexandre Julliard wrote:
Sam Edwards <cfswo...@gmail.com> writes:
Yeah, I don't call it until patch 2 in the series. This patch just
introduces it without calling it, which does cause a warning. (I hope
this doesn't violate the "atomic patches" rule.)
It does, you can't add dead code (or warnings for that matter).
So, if I were to give this patch a try 2, I'd have to call the event
handler somewhere (probably right after selecting for the resize event)?
I'll keep that in mind for the future. :)
But no, you can't select for events on the gdi display; actually I don't
think you want to do it this way at all.
So gdi_display should not receive events? I'm guessing this has
something to do with the fact that Wine opens one Xlib Display per
thread, so the XRandR resize events need to be pulled down on one of
the thread-local X connections, rather than the master
gdi_display. (I'm still not sure why Wine does it this way. Are
certain versions of Xlib not thread-safe?)
Most window events need to be handled in the thread that owns the
window. Pulling them from a global connection in a different thread
would only cause extra context switches and potential deadlocks.
This is kind of a special case because RRScreenChangeNotify is a root
window event, so there's no "thread that owns the window" there (unless
Wine has some internal representation for the X11 root window, and a
thread that owns that representation, that I'm not aware of).
Handling external resizes
should be done in the desktop process, and most likely involves the
wineserver too.
The event handler does call X11DRV_resize_desktop, which messages the
desktop process with the new size. But, I can see how it would be
cleaner to put the event checker in the same process as well, since
that way the XRandR resize doesn't have to get processed once for
every process on the system.
How would this involve the wineserver, though? It looks like we
already have the infrastructure for synchronizing screen_width and
screen_height changes from one process to another.
Not really, only the process that triggered the change gets properly
updated at the moment. The Mac driver tries to do it by broadcasting a
message, but that doesn't update processes that currently don't own a
window. There are also race conditions, and various issues with window
surfaces and DCE regions. Resizing the desktop is a tricky problem.
Aha; this is why the wineserver is necessary: So that applications that
haven't created any windows can receive the update.
As far as XRandr is concerned, at this point you could probably simply
cache the mode and don't bother to update except when the app triggers a
change, since that's what we do for the rest of the desktop parameters
anyway.
Thanks for the suggestion. That sounds like it has a much smaller
impact, so I think I'll use that approach instead.
Until then, I'll withdraw this patch.
Thanks,
Sam
