Opened 3 years ago

Last modified 3 years ago

#12618 assigned bug

[app_server] redraw issues after changing Panel background color

Reported by: diver Owned by: looncraz
Priority: normal Milestone: R1
Component: Servers/app_server Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: yes Platform: All

Description

hrev50052.

Originally filled at https://github.com/HaikuArchives/PecoRename/issues/5

PecoRename starts using a lot of cpu after changing Panel background color in Appearances preflet.

It also reproduces a bug in app_server when it fails to update foreground window when moving one above PecoRename.

Attachments (1)

BMessage-WasHandled-12618.patch (2.2 KB) - added by looncraz 3 years ago.
Patch Candidate

Download all attachments as: .zip

Change History (9)

comment:1 Changed 3 years ago by looncraz

Okay, I took a quick look at this and the problem *appears* to be that PecoRename's Fenster (BWindow) posts unhandled messages in MessageReceived(...) to be_app.

In the case of B_COLORS_UPDATED, this results in BApplication::DispatchMessage(...) handling the message and once again posting it to all windows in a vicious cycle.

I'm not sure what would be the most appropriate remedy. Repairing the application would be simple, but it seems that this behavior should probably be considered safe.

We could append a boolean (e.g. "be:message_sent" true) to the message in BApplication::DispatchMessage(...) to prevent the cycle from repeating..

Or augmenting BMessage with a "handled" flag - which would be faster and more universally applied to prevent unwanted echoes (this would be private API: MESSAGE_FLAG_WAS_HANDLED = 0x0200 in MessagePrivate.h).

I like the latter more than the former, as it doesn't pollute the message contents and should be much faster.

It is also possible to have the app_server send the B_COLORS_UPDATED message to each window directly, but the updates are not applied in as predictable as a manner when this happens and performance is lower. And, soonish, B_FONTS_UPDATED will need to be handled by BApplication and every BWindow, so this problem would return unless the app_server sends the message to every application and every window.

I will await suggestions.

comment:2 Changed 3 years ago by diver

Any ideas about foreground window corruption?

comment:3 in reply to:  2 ; Changed 3 years ago by looncraz

Replying to diver:

Any ideas about foreground window corruption?

AFAICT, it's a matter of timing between the window moving and the drawing. Probably a failure to disable CopyToFront() at some point, which results in part of the stalled drawing being moved during the window move.

I'll look to see if that theory pans out to anything.

comment:4 in reply to:  3 ; Changed 3 years ago by looncraz

Replying to looncraz:

Replying to diver:

Any ideas about foreground window corruption?

AFAICT, it's a matter of timing between the window moving and the drawing. Probably a failure to disable CopyToFront() at some point, which results in part of the stalled drawing being moved during the window move.

I'll look to see if that theory pans out to anything.

Okay, it's the use of LockParallelAccess() in Desktop::MoveWindowBy(...) that's the most likely culprit. Locking exclusive access for CopyRegion() would probably fix that issue, though it will probably have some undesirable performance ramifications as this would halt all drawing for that instant - while also giving moving windows higher effective priority (not necessarily a bad thing, though).

I will give that a try and see what happens.

Changed 3 years ago by looncraz

Patch Candidate

comment:5 Changed 3 years ago by looncraz

Has a Patch: set

comment:6 in reply to:  4 Changed 3 years ago by looncraz

Replying to looncraz:

I will give that a try and see what happens.

Tried it, didn't fix it. That will require more investigation.

comment:7 Changed 3 years ago by axeld

You could check the reply-to address, to check if it has been a local delivery.

Other than that, while we don't yet have such a flag (and therefore the need hasn't been strong), I'm okay with that solution, too. Would like to hear another opinion on the matter, though :-)

comment:8 Changed 3 years ago by diver

Cc: looncraz removed
Owner: changed from axeld to looncraz
Status: newassigned
Note: See TracTickets for help on using tickets.