Possible bug(s) in PaintManager.repaint?

[zhaizhai]

Hi, whilst trying to implement a custom component, I noticed a few situations where my component would refuse to repaint properly (it would call repaint, but the repainting would be short-circuited somehow). Looking at the PaintManager.repaint function, there are two things that seem fishy to me. First, we have

    var da = canvas.$da;
    if (da.width > 0) {
        if (x >= da.x                &&
            y >= da.y                &&
            x + w <= da.x + da.width &&
            y + h <= da.y + da.height  )
         {
            return;
         }
         MB.unite(da.x, da.y, da.width, da.height, x, y, w, h, da);
    }
   else {
       MB.intersection(0, 0, canvas.width, canvas.height, x, y, w, h, da);
   }

It seems like there should not be an early return there, as we don't want to skip painting entirely; we just want to avoid calling MB.unite when unnecessary.

Second issue is that at the end of the function passed to setTimeout, we have

    $this.paint(context, canvas);

    context.restore();
    canvas.$da.width = -1; //!!!    

It seems to me that the last line is unnecessary, and in fact I tried commenting it out and things seemed to work OK. Actually, fixing the two issues described here seemed to solve my problem, although of course it could have broken something else.

Are these bugs? Can you explain what scenario the canvas.$da.width = -1 is supposed to guard against? If needed, I can try to create some problematic examples to help you reproduce my issue, but this will take some effort, so I figured I would ask first. Thanks!

[barmalei]

From one hand paint manager is very carefully overviewed and tested code, since it has big impact to entire zebra functionality. From another hand the first snippet with "return" also look not clear to me. Have to check why it is necessary (or unnecessary).

canvas.$da.width = -1 is indication the paint cycle has been completed and the canvas has no any dirty area. It allows paint manager to avoid unnecessary overhead.

I would appreciate very much any snippet you can provide to illustrate the problem.

PS: nice to see you can follow in source code so deep :)

[zhaizhai]

Thanks for the quick response! I see now that I might not have been implementing my custom component in the intended way. I was trying to do some animation, and the bug I had was that through a chain of events, a call to paint actually triggered synchronously something else which modified the state and asked to repaint again. The second repaint was effectively ignored because of the canvas.$da.width = -1 thing, which short-circuited the next repaint.

In this case I can fix the problem in my code. At first I thought setting canvas.$da.width = -1 was just an unnecessary optimization (since you were already throttling at 20 fps), but now I see that calling repaint during painting is somewhat dangerous practice, since you can get into an infinite loop of repaints (albeit throttled). Setting the width to -1 essentially invalidates any repaints called synchronously within a paint method. So I can see why you might have wanted to do that.

Perhaps instead of ignoring these silently, though, there should be a warning? That would have helped me in this case. I'm thinking that valid uses (if there are any) of calling repaint within a paint method can be implemented by calling repaint asynchronously (or just ignoring the warning).

I will also see if I can construct a snippet to illustrate the first issue with the early return.

[barmalei]

In general calling repaint method from paint manager "paint callback" (set with setTimeout()) is not forbidden. Paint manager "paint callback" does two important things:

• validates canvas and components hierarchy it holds
• paint the canvas UI components hierarchy

At the validation stage validated UI components can call repaint method as many time as they want and need. Actually it is not avoidable, since validation often means applying layout managers what causes UI components resizing and relocating. Called at the validation stage "repaint(...)" method will update dirty area of the rendered canvas. So at the second stage (paint) paint manager will have valid and up-to-date dirty area.

At the second (paint) stage calling repaint method is too late. It also will cause canvas dirty area updating but painting process has already been initiated and the dirty area updates will not take effect.

The rule is the following: don't call repaint method from a component paint(...), update(...), paintOnTop(...) methods, but feel free to call it from any other places.

[zhaizhai]

By "paint method" I did mean one of paint, update, paintOnTop. So my proposal is to do something like adding a warning here:

   // prevent double painting, sometimes 
   // width can be -1 what cause clearRect 
   // clean incorrectly  
   if (canvas.$da.width <= 0) {
       console.warn("repaint ignored because called synchronously within one of paint, update, or paintOnTop!");
       return ;
   }

As for the early return in calculating dirty area, I realized that there are only two scenarios where the canvas will have a dirty area:

1. there is already another paint scheduled, or
2. you are doing something like calling repaint within paint

In the first case, it's fine to return early because a paint will happen soon anyway. In the second case, the repaint is already going to ignored (by the current logic), so it doesn't matter. Still, I feel like the code is a bit confusing as it stands. I'm planning to send you a pull request as a suggestion for clarifying the logic (no pressure to accept it, I understand if you don't want to make changes to this part of the code).