wxDisplay::GetClientArea ignores changes to taskbar position

[kevinwatters]

wxDisplay::GetClientArea returns a rectangle equal to the display rectangle MINUS the rectangle occupied by the taskbar.

If you move the taskbar while a wx application is running, subsequent calls to wxDisplay::GetClientArea do not reflect the new client area.

[ericj]

I can confirm this. Reason is that wxDisplayFactory *gs_factory is only initialized once, so that changes that occur at runtime (moving taskbar, changing screen resolution, etc.) are not recognized by the application.

As a quick fix i intended to implement a simple static wxDisplay::Reset(), but then i noticed the class wxDisplayModule which seems to be for the same purpose. But it's not used anywhere.

Looking at the source, it seems that i could just create a wxDisplayModule instance and call OnExit() on it to clear the static list of displays, but somehow it doesn't feel right and i don't think it was intended to be used this way.

[vadz]

The simplest solution is probably to not use the cache in wxDisplayImplWin32Base::GetClientArea() implementation or maybe even at all (other fields of the struct don't change but as we can't get the client area without getting them as well it doesn't seem useful to continue caching them). I.e. just remove m_info and create a local wxDisplayInfo variable in every method using it.

Just invalidating the cache only when the taskbar is moved would be even better but I don't know of any Windows messages notifying about this. If anybody does, please let us know.

[ericj]

The problem does not only arise when the taskbar is moved, also changes of the display resolution are not recognized.

Just invalidating the cache only when the taskbar is moved would be even better but I don't know of any Windows messages notifying about this. If anybody does, please let us know.

Experimentally i found out that a WM_SETTINGCHANGE message with wParam=SPI_SETWORKAREA is send.
 http://msdn.microsoft.com/en-us/library/ms725497(VS.85).aspx
 http://msdn.microsoft.com/en-us/library/ms724947(VS.85).aspx

I don't know how reliable this method would be and i think it would require to create a hidden window to receive this message, doesn't it?

[vadz]

Thanks for testing this! I didn't think WM_SETTINGSCHANGE would be sent in this case, good to know I was wrong about it.

We do have a hidden window for this kind of use already, see wxCreateHiddenWindow() function in src/msw/utils.cpp. But maybe it's an overkill to use it just to be able to cache the results. I guess the native functions we call should be quite fast as they probably just return values from some user32.dll struct without going into kernel mode. So maybe it's still better to just get rid of the cache.

[Carsten]

The attached patch fixes this issue, and a few related, as discussed at

 http://thread.gmane.org/gmane.comp.lib.wxwidgets.devel/124070
 http://thread.gmane.org/gmane.comp.lib.wxwidgets.devel/106955

Caching of monitor details (geometry, client area, name, is primary) has been removed entirely.

The list of available monitors is still cached, or else the entire list had to be re-acquired over and over again for each of the above detail queries.
The list is renewed though (on initialization and) on each call to wxDisplay::GetCount() (wxMSW only).

From a users perspective, I think that this gives the best balance between excessive Win32 API calls and flexibility, because he loses no performance when he iterates the list of displays only once (e.g. during app init, and once we have to acquire the list in any case), and if he iterates the list later (e.g. to find gone or newly attached displays) it must be recreated anyways, too.

Only (small) problem: Don't do this:

for (unsigned int i=0; i<wxDisplay::GetCount(); i++) ...;

but better this:

const unsigned int NumDisplays=wxDisplay::GetCount();
for (unsigned int i=0; i<NumDisplays; i++) ...;

At the same time, the use case of keeping a wxDisplay instance for later use is ok, because when queried, it just looks up the current data via a single Win32 API call.

[vadz]

IMO reenumerating all the displays in GetCount() is really unexpected. I think we should provide an explicit Refresh() method to do this instead.

[Carsten]

Well, doing it in GetCount() was the nearby approach without changing the wxDisplay API.

I can update the patch to have wxDisplay::Refresh() instead, implying appropriate implementations for the other ports as well.

[vadz]

I don't think the other ports cache anything in fact so it could be left empty for them.

OTOH maybe we shouldn't add it even for MSW. I still think we should just catch WM_SETTINGSCHANGED and update automatically ourselves when needed. So maybe let's just do nothing in GetCount() for now?

[Carsten]

When the settings are changed (i.e. the taskbar is repositioned and WM_SETTINGSCHANGED is triggered) there is not actually a need to re-enumerate the monitors, because their number has not changed and the new client rectangle can now be obtained without problems with my patch.

That is, changing the taskbar requires no additional action at all (as far as wxDisplay is concerned) when this patch is applied, neither do changes to screen resolution.

The application code might be interested in learning when a WM_SETTINGSCHANGED occurs (and then query the wxDisplay about the new client area etc.), but that is probably of no concern to wxDisplay itself.

The only event that requires additional action is adding or removing monitors during the lifetime of the application.

I opted to address this via putting the re-enumerate code in GetCount(), but I agree that - for now - just doing this once during the first-time initialization and then never again is feasible as well. If we later find that we want wxDisplay to update when a new monitor was connected or disconnected during app lifetime, something like wxDisplay::Refresh() could still be introduced.

Please let me know what you prefer more:

• Doing the init only once (especially not in GetCount()). This would resemble the existing code the most.
• Introduce Refresh()...

I'll change the patch either way, where the second option involves changes to other ports that I alone can not sufficiently test (unless Refresh() is simply implemented as an empty method in wxDisplayFactory, unlike the other methods which are declared as abstract (...=0)).

[vadz]

I prefer doing it only once for now and redoing it in response to WM_SETTINGSCHANGED (which I believe is sent when a monitor is added/removed too) later.

TIA!

[Carsten]

Revised patch that enumerates the monitors only once during app init (not anew on each call to GetCount())

[Carsten]

Ok, thanks, revised patch is attached. :-)

[VZ]

(In [65555]) Remove display information caching from MSW wxDisplay implementation.

Don't cache display rectangle and client rectangle as they can both change
during the program lifetime (especially the latter which changes whenever
taskbar is moved or shown/hidden) and retrieving them every time they're
needed doesn't seem to be a problem performance-wise anyhow.

We still cache the list of all the monitors, ideally we'd refresh it when we
receive a notification about a display being [dis]connected.

Closes #4582.