Opened 12 years ago

Closed 12 years ago

#2355 closed bug (fixed)

Incorrect BView::Draw() Update Rect after CopyBits()

Reported by: bonefish Owned by: stippi
Priority: normal Milestone: R1
Component: Servers/app_server Version: R1/pre-alpha1
Keywords: Cc: tqh
Blocked By: Blocking:
Platform: All



After a sequence of these method invocations:

Invalidate((35.000000, 345.000000) - (41.000000, 359.000000))
Invalidate((0.000000, 345.000000) - (6.000000, 359.000000))
Invalidate((0.000000, 345.000000) - (6.000000, 359.000000))
CopyBits(((0.000000, 15.000000) - (560.000000, 388.000000)) -> (0.000000, 0.000000) - (560.000000, 373.000000))
Invalidate((0.000000, 345.000000) - (559.000000, 359.000000))

the view receives the following Draw() method call:

TermView::Draw((0.000000, 0.000000) - (560.000000, 373.000000))
Bounds(): (0.000000, 0.000000) - (560.000000 - 373.000000)
clipping region:
  (0.000000, 330.000000) - (6.000000, 344.000000)
  (35.000000, 330.000000) - (41.000000, 344.000000)
  (0.000000, 345.000000) - (559.000000, 358.000000)
  (0.000000, 359.000000) - (560.000000, 373.000000)

The update rect covers the whole view, although the clipping region is constrained to 44 pixel rows at the bottom.

Change History (8)

comment:1 by mmlr, 12 years ago

This might be intended behaviour. In hrev25854 the behaviour for calling Draw() has been adjusted to work the same as under R5. Under R5 you will get Draw() calls also for regions that do not intersect with the current clipping, which allows caching and asynchronously repainting of exposed areas as done by Firefox for example. Could you please check if this also happens under BeOS? Note that the clipping region will still limit the drawing to the same area, even though the update rect might be bigger.

comment:2 by bonefish, 12 years ago

The behavior can be observed in hrev25853 as well. Unfortunately I haven't been able to reproduce the behavior with a minimal test app yet. Might be timing related. It is relatively reliably reproducible with the Terminal in the state I have it ATM.

At any rate I really can't see how supplying an unnecessarily large update rect to Draw() can have any positive effects. Even if the rect is cached and used as a clipping region for later drawing outside Draw() the minimal update rect should suffice. The only effect I can see is that more work than necessary is caused.

comment:3 by tqh, 12 years ago

Cc: tqh added

comment:4 by stippi, 12 years ago

The fact that you cannot reproduce this in a test app is interesting. Knowing how the update mechanism works, I would be tempted to say that other views in the view hierarchy are being invalidated as well. But since this is Terminal, it is hard to see which ones. The update rect is unfortunately one and the same rect for all the views that are invalidated in one "session" (the frame of the dirty region). It is clipped to the respective view bounds on the client side. In another words, if there is a view being redrawn to the top of the view you are interested in, you would get an invalidation rect that extends to the top of that view no matter what parts you invalidated. I could change this, so that each view gets it's own update rect. I am looking at the clipping region of each potentially affected view anyways to know whether or not it should be put into the update session. However, the circumstances with the Terminal might hint at another problem with CopyBits(). Have you already taken a look at the update message that arrives at the window? Is the TermView the only one being redrawn? BTW, I can see what you are trying to do and why this behavior is definitely a problem.

comment:5 by bonefish, 12 years ago

You are right, another view is invalidated in this case. When hitting enter in the last line of the terminal screen the history size changes and thus the scroll bar range is updated. Simulating this situation in the test app, I can reproduce the same update rect behavior there, too.

ATM I'm working around in TermView::Draw() by getting the frame of the clipping region, but this is of course more expensive than doing it in the app server in the first place.

Under BeOS the test app gets the correct (i.e. minimal) update rect, BTW.

comment:6 by stippi, 12 years ago

Owner: changed from axeld to stippi
Status: newassigned

Ah, great, so it isn't something mysterious. Yeah, I guess I can fix this for you.

comment:7 by stippi, 12 years ago

Could you please check if hrev25879 fixed this issue for you?

comment:8 by bonefish, 12 years ago

Resolution: fixed
Status: assignedclosed

Confirmed fixed. Thanks!

Note: See TracTickets for help on using tickets.