[haiku-development] Re: Set*UIColor Version 3 Patch

  • From: John Scipione <jscipione@xxxxxxxxx>
  • To: "haiku-development@xxxxxxxxxxxxx" <haiku-development@xxxxxxxxxxxxx>
  • Date: Fri, 6 Nov 2015 14:38:51 -0800

On Wed, Nov 4, 2015 at 7:35 AM, looncraz <looncraz@xxxxxxxxxxx> wrote:

On 11/2/2015 14:34, looncraz wrote:
It is the fifth day since I posted the patch and still no response.

I'm a big boy, if there's criticism waiting, I can handle it ;-)

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.

On Fri, Oct 30, 2015 at 10:47 AM, looncraz <looncraz@xxxxxxxxxxx> wrote:
1. BView gains the following methods:

HasDefaultColors() const
HasSystemColors() const
AdoptParentColors()
AdoptSystemColors()
AdoptViewColors(BView* view);

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.

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?

color_which ViewUIColor(float* tint = NULL)
color_which HighUIColor(float* tint = NULL)
color_which LowUIColor(float* tint = NULL)

Same here, looks good to me.

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.

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.

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.

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.

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?

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.

6. Tracker InfoView, AboutSystem, and others are now font-sensitive, so you
no longer need to use a magnifying glass to read the details on a high def+
monitor - just set the font size and be happy.

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.

7. Every window and view receives a B_COLORS_UPDATED message when any color
is updated, this message includes a BColorMap data structure that can be
accessed with static methods or you can instantiate a BColorMap from the
message directly. I will be documenting BColorMap once I am sure its API is
solidified (pending Axel et.al. feedback). In order to cut down on the
number of calls, these messages are merged together and sent only about 60
times a second at a maximum, so the message may include many colors which
have been updated (possibly every color on the system).

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?

A word on performance:

I have taken great strides to ensure the optimal performance for this system
and to ensure things remain fluid, however I do not have much slow hardware
available to me so testers on the slowest hardware available would be
welcome. I can provide an install image if anyone is interested and doesn't
feel like building the source themselves.

A word on BeOS R5.1 dan0 API compatibility:

While dan0 had a method called SetViewUIColor it used const char* constants,
whereas this version uses the existing enumerated color_which constants.
Removing the _UI_ part of the dan0 constants should result in a compiling
program with the expected behavior.

DelayedInvalidate(), however, should be source compatible, with the added
benefit that calls are merged on the server side and the given rects are
potentially merged, resulting in less unnecessary redraws. As with dan0,
short delays make very little sense, this is meant to DELAY a redraw,
after-all.

Other related posts:

  1. Home
  2. haiku-development
  3. Archive
  4. 11-2015