MichaelChirico / r-bugs

A ⚠️read-only⚠️mirror of https://bugs.r-project.org/
20 stars 0 forks source link

[BUGZILLA #17811] Graphical window redraw issues cause re-entrancy, incorrect title label, and small performance hit #6988

Open MichaelChirico opened 4 years ago

MichaelChirico commented 4 years ago

Created attachment 2614 [details] Demonstration plots to resize and change active device on

Two issues and a minor doc bug with graphics operations on Windows.

A demonstration file is attached which can show the problems.

A patch file is attached containing both patches discussed below.

  1. Minor documentation bug in documentation for windows():
When a screen device is double-buffered (the default) the screen is updated
100ms after last plotting call or every 500ms during continuous plotting.
These times can be altered by setting options("windowsTimeout") to a vector
of two integers before opening the device. 

options("windowsTimeout") should be options("windowsTimeouts")

  1. When you resize the active window, the ACTIVE label in the active window temporarily changes to "inactive" and back again. This does not happen for the inactive windows when resized.

While the active window is being resized it is blanked out and only repaints when you finally release the mouse after a delay set by 'timeafter' in options('windowsTimesouts'). This does not happen for the inactive windows when resized.

During my investigation of the code I discovered that the routine HelpExpose is called re-entrantly because of the bug above.

A patch is suggested below which cures this problem.

  1. The window is redrawn at least twice for any resize operation invoked by dragging with the mouse. Resizing to maximum or from maximum does not do this. For plots which take a long time to redraw this makes trying to adjust a window size very difficult.

A possible patch is suggested below which can alleviate this problem partly.

Examples

See attached file demonstrations.R

Terminology (from variables in devWindows.c): timeafter - update screen if no drawing activity for this much time in ms (default 100ms) timesince - update screen every timesince ms during continuous plotting (default 500ms) (resolution about 16ms. Mostly useful when the plot is drawn for the first time.)

Issue 2: Cause and suggested patch:

When a window is resized, ultimately HelpExpose in devWindows.c is called which replays the display list by calling GEplayDisplayList.

Replaying the displaylist temporarily changes the current device to be the window being resized.

HelpExpose sets xd->replaying = TRUE to let GA_Activate and GA_Deactivate know that they can return without doing anything. However this only works for the window located by 'xd' (the one being resized. If the ACTIVE window is not the one being resized then the method fails for the ACTIVE window in GA_Activate:

As a consequence when the ACTIVE window is resized it does not get a drawbits at the end of the replay but when an inactive window is resized it does get a drawbits, and the active window is also flushed and the title will change from ACTIVE to inactive and back again on the active window.

The lack of flushing if the ACTIVE window is resized means that it must wait until the timer set by GA_Timer expires before it is fully updated which causes a noticeable lack for the user. So ironically the bug means that the inactive window is flushed when resized (by accident) but the ACTIVE window isn't.

Another side-effect of the bug is that GA_Activate calls 'settext' to change the window's title bar. But 'settext' initiates another redraw as a consequence and this then re-enters HelpExpose. This happens for both calls to selectDevice in GEplayDisplayList.

The suggested patch below does the following:

A further change which is not required but might be worthwhile is to set GA_xd to NULL before replaying the displaylist and restoring it afterwards. This will disable any updates due to 'timesince' during the window redraw but it is not clear whether that matters. (Note that many top-level plot routine call dev.hold/dev.flush anyway to inhibit window update while they contruct the plot. Calls to dev.hold/dev.flush do not go onto the display list so the effect is not permanent.) There will be no need for 'timeafter' redraws (and no chance to trigger them since the timer cannot fire until a 'doevents' is called.) So setting GA_xd to NULL will disable completely the calls to GA_Timer while redrawing the window for a tiny performance gain.

If this is not done, GA_xd may change to point at the new current window which does no harm but it is not clear if it is doing any good in terms of helping with performance or appearance.

This change is shown commented out in the patch for now.

--- a/src/library/grDevices/src/devWindows.c +++ b/src/library/grDevices/src/devWindows.c @@ -854,16 +856,22 @@ static void HelpExpose(window w, rect r) pDevDesc dd = (pDevDesc) getdata(w); pGEDevDesc gdd = desc2GEDesc(dd); gadesc xd = (gadesc ) dd->deviceSpecific; + gadesc curxd = (gadesc ) GEcurrentDevice()->dev->deviceSpecific; + /gadesc save_GA_xd = GA_xd;*/

if (xd->resize) {
    GA_Resize(dd);
    /* avoid trying to replay list if there has been no drawing */
    if(gdd->dirty) {
    xd->replaying = TRUE;

+ if(curxd) curxd->replaying = TRUE; + /GA_xd = NULL;/ GEplayDisplayList(gdd); + drawbits(xd); + /GA_xd = save_GA_xd;/ xd->replaying = FALSE; + if(curxd) curxd->replaying = FALSE; }

Issue 3: Cause and suggested patch:

The window is redrawn at least twice for any resize operation invoked by dragging with the mouse. This happens because as soon as the mouse moves a redraw event occurs. While the window is redrawing the mouse will move further and may arrive where the user wants it to be. The first redraw has to complete before the second can start.

Usually that first redraw involves only a very small delta on the X or Y change because it is triggered as soon as Windows notices that the mouse is dragging the window. By ignoring resizing for movements smaller than some delta fro the current window size the window does not embark too soon on a full-blown redraw. This does mean that a small mouse movement will not resize the plot at all though. So there is a question about whether this is acceptable behaviour. Personally I find it quite "friendly" to use, especially with slow plots. A suggested patch which allows up to 5 pixels of "slack" is shown below.

--- a/src/library/grDevices/src/devWindows.c +++ b/src/library/grDevices/src/devWindows.c @@ -830,9 +830,11 @@ static void HelpResize(window w, rect r) if (r.width) { if ((xd->windowWidth != r.width) || ((xd->windowHeight != r.height))) {

A more sophisticated approach would be to abort a redraw if it is discovered that another redraw is on the way. I think this would require fairly major surgery though.


METADATA

MichaelChirico commented 4 years ago

Created attachment 2615 [details] Patch for issues 2 & 3

Contains both patches. Part of suggested patch for issue2 is commented out as unnecessary for the specific problem so more of an "enhancement" or "change of behaviour".


METADATA

INCLUDED PATCH

MichaelChirico commented 4 years ago

Created attachment 2616 [details] Replacement for previous 'demonstrations.R'

There was a stray closing bracket in the previous version of this file. This upload corrects this error.

This file contains a few demonstration plots of differing CPU load to use to experiment with buffered drawing for various timeout values and show different behaviours of active/inactive windows.


METADATA

INCLUDED PATCH