[haiku-development] Re: Set*UIColor Version 3 Patch
- From: looncraz <looncraz@xxxxxxxxxxx>
- To: haiku-development@xxxxxxxxxxxxx
- Date: Fri, 6 Nov 2015 17:40:14 -0600
On 11/6/2015 16:38, John Scipione wrote:
I haven’t had time to look at your patch in detail but I wanted to try
to respond to your changes at a high level.
Thank you!
All these method signatures seems pretty reasonable to me.
HasDefaultColors() and HasSystemColors() don’t necessarily need to be
virtual but the Adopt… methods should be so that they can be
overridden by child classes.
I'd agree, but I'm under the impression that we don't have the luxury of
adding virtual methods freely and maintaining backwards compatibility.
If that's not the case, then these should absolutely be virtual, and I'd
have BControl and others override AdoptSystemColors() so calling them
won't mess with the intended drawing (LowColor should always match the
parent view color).
SetViewUIColor(color_which, float tint = B_NO_TINT)
SetHighUIColor(color_which, float tint = B_NO_TINT)
SetLowUIColor(color_which, float tint = B_NO_TINT)
I’m not altogether sure what the difference is between SetViewColor()
and SetViewUIColor() but I’m assuming that there is some difference
these names all seem pretty good to me. They come from Dan0
apparently?
The name SetViewUIColor comes from Dan0, but we need the separation so
ViewUIColor() et. al. make more sense ;-)
DelayedInvalidate(bigtime_t delay)
DelayedInvalidate(bigtime_t delay, BRect invalRect)
These come from BeOS R5.1 too right? Either way they seem pretty good
names except I would call the second BRect parameter name updateRect
instead of invalRect since that is what we use elsewhere and avoids
abbreviation. The idea of combining multiple messages together to send
at once seems like it can be a real performance win in certain
situations so I like this.
Yes, also from Dan0. I thought merging duplicates was the only way to
go ;-) I was annoyed more than once when Dan0 would repeat redraws
without need.
I will not go into describing how all of these work here, however most
people will just want to use AdoptSystemColors() and call it a day ;-)
I have been following your commits so I have a basic overview of what
you are going after. You seem to have figured out the problem in broad
strokes but I do have some concerns about some of your changes being
more practical than ideal. I remember specifically some blowback you
received setting the UI colors in BView::Layout() instead of in a more
appropriate spot.
Yes, but there is literally no other place it can be done more
appropriately, and it enables the top view to update with layout view
colors (which, usually, will be system colors, but may be custom colors
[previously not possible]).
2. All Haiku apps and preflets in the source tree have full live color
updating and some level of font awareness.
Wonderful! This is a big step forward and I didn’t think I would see
this day so soon. Great to hear. Font awareness of course is an
ongoing problem and there is no quick fix.
Definitely no quick fix, my first shot at full fontaware -ness, though,
showed that it may well be possible to create a generic solution. I am
looking at building a ratio-based view positioning and sizing snapshot
that can be used to resize even legacy, closed-source, programs more or
less properly. Not sure how well it will work, I have some experiments
to run ;-)
It works rather well with layouts, but not perfectly - and window sizing
still has some issues.
Due to how this will only properly work with certain apps, and possibly
poorly with others, I am building a blacklist system that will allow
blacklisting apps, or specific windows/views in an app.
3. All system system interface elements should intelligently adopt the
colors of their parents, so legacy apps should have no issues (there are a
few exceptions that can't be helped, such as buttons and BBoxes added
directly to a window).
I assume that you have to call AdoptParentColors() to get this
behavior in your own app.
Yes.
If you don't have a parent, though, you should use AdoptSystemColors()
if you want normal colors ;-)
4. Spamming set_ui_color will see the events merged and updates will only
occur at a set 60hz rate, IIRC. A future update will make this a system
configurable option.
This is a performance issue I’m guessing? Why make it configurable?
Yes, all about performance. A 60hz rate is fine for a 4.5GHz i7 2600k.
May not be so great for an AMD Athlon XP 2500+. I could either have it
adapt based on some type of feedback message, or simply set it based on
a cheap CPU performance guess ;-)
5. Spamming DelayedInvalidate() calls will merge redundant calls, contrary
to BeOS R5.1 behavior.
It is hard for me to make a judgement call on this one since I am not
sure what the motivation was on BeOS R5.1. It was never released
though and we aren’t trying to maintain compat with Dan0 anyway so
this seems like a place that you are free to change the behavior.
The end result will be the same - the view is redrawn. In R5.1 I think
DelayedInvalidate() used BMessenger::SendMessageAtTime(), so it lacked
the ability to merge repeats. In addition, it didn't use the cadence
the app_server does for drawing, so you could get a violent loop of
redraw calls (and flickering and tearing to go with it), whereas Haiku's
app_server creates a dirty region to redraw, than redraws at a proper
time (much better).
Merging the invalidations here, though, prevents extra locking and
BRegion calculations while still ensuring that the most updated content
is drawn.
Just a quick question about this. Did you make sure that the
read/write/execute permissions section of the Tracker’s info window
also is font-sensitive? I seem to recall that being an issue before.
The permissions view is still not font sensitive, but I plan to revisit
it when I address font sensitivity and awareness fully.
I saw that you changed BColorMap to be a wrapper around a Message so
this seems pretty straightforward. Once again, what is the motivation
behind 60Hz, just to limit the message queue backlog?
Yes, BColorMap is, in essence, quite simple. It uses copy-on-write
techniques, hides its data implementation, and, in the app_server, can
survive having its data stolen from it then broadcast to every
application. It's most important use is in storing the pending color
changes so you can spam set_ui_color with impunity, which means color
changes occur only once per cycle (if you set every color on the system
500 times every 60 seconds, you still won't slow down the system, since
your previous changes are simply overwritten, and we only pay attention
to the latest).
Yes, the 60hz is to limit how often applications have to respond to
color updates. The slider can send an incredible amount of updates, and
each one results in a complete redraw of every single visible view on
the system, so it can be heavy-hitting. I played with 30hz, and even
15hz, but I could see some tearing when that happened. Would be nice to
be able to trigger updates only after a system redraw... would require a
special flag for the messages... I kinda like that idea ;-)
I'm trying to restrain myself from making any more changes - I'm never
out of ideas.
Thanks for looking at it!
--The loon
Other related posts:
