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

  • From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
  • To: haiku-development@xxxxxxxxxxxxx
  • Date: Sun, 6 Dec 2015 17:21:48 +0100

Am 23/11/2015 um 23:19 schrieb looncraz:

set_ui_colors would then require some list of colors. It could be a
c-array, but that's unsafe, or it could be a BList, but then it'd have
to be ordered or contain key-data pairs. BColorMap::SetUIColors()
handles this with just repeated calls to set_ui_color which, as you
rightly point out, is far less than ideal (though it is astonishingly
fast nonetheless).

Sorry for getting back to you so late, but I'm quite busy these days.
It's not only about speed, but also about atomicity of the changes.
It just looks better when all colors change at once. And then, why not animate the change from one theme to the next? ;-)
set_ui_colors() could still accept a message with the changes; at least I agree that an array wouldn't be the optimal solution.

There are numerous places already were I pull the new color value right
from the message. There are numerous apps which have bitmaps that don't
need to be redrawn unless a specific color is set, likewise some objects
have less than trivial color handling as it is (such as BColumnListView,
which still needs to be optimized to use the data already in the message).

This could be eliminated entirely, but we will be doing more work than
is needed when a single color changes rapidly.

Very little extra work, actually, as, AFAICT all views are still invalidated anyway (and rightly so). So you have all this extra API for pretty little gain.

The one downside I see is that two versions of each of those methods
will need to exist, or we would need to adopt Be's B_*_UI_COLOR tactic,
which puts a stopper in iterations and reduces performance. It would be
a major reworking.

Not sure I understand what you're aiming at there.

At that point, I think it would make more sense to send an empty
B_COLORS_UPDATED message, or keep & modify BColorMap. It can certainly
live on as a private addition, but I think it is very beneficial for 3rd
party apps which want to more precisely handle color updates.

I think a message would just be good enough for this case. Also, you could still provide the one changed color (if any) -- just ignore this if there are more than a single change.
But as I said before: even if it sounds nice to know which color changed, it actually has very little use or impact in real code.

Is there any other use case you anticipate for the delayed message
implementation? If all you really do with it is reply throttling, it
could be solved a lot simpler, especially since you already keep
pending data extra (as I said above, I wouldn't do any housekeeping
there, though).
It is extremely good for scheduling future tasks without leaving
anything else running, and is probably the easiest possible way to
prevent duplication of effort. Julian Harnath, a week or two ago, was
talking about needing to run a cleanup routine every minute or so. He
was talking about creating a cleanup thread just for that. I offered
DelayedMessage as an alternative, and he can then use existing threads
to do the job.

That, too, could be solved a lot simpler, though. Just add a timeout to the wait for the next message, and carry out any extra work then.
I see no need for anything fancier than this.

Throttling is, of course, also a very useful facet. And how much it
simplifies the remainder of the code since it knows exactly what you
want to happen if there other messages with the same code pending, which
is something that cannot be done with a BMessageRunner (which is also
more resource heavy).

Without any more use cases, this is just a theoretical advantage, though.

Why add the UI color stuff to the server's View at all? It looks like
it could be handled client side completely. Have you looked into this?
I did it this way originally, but PushState() and PopState() could crush
low and high colors, so they'd stop updating.

I either don't understand this, or it sounds like a problem of the implementation: when the colors are updated, and views update their view/low/high colors, they will automatically affect the state, too, no?

In addition, the
app_server can tint its colors locally and I don't have to have all of
the message traffic for each view as it was before, now the server and
client stay in sync without repeated Set*Color() calls in [...]

It only does that when it draws itself, right, in the decorator, and the workspace view. I don't see that as much of a problem, though, anyway, as that's pretty much the same as client drawing.

ServerWindow/invalidate: why not find the view by token (fast lookup)
instead of walking over the complete view hierarchy?
Didn't know that was possible. How is that achieved? All I found in
Window is fTopView.

Have a look at AS_SET_CURRENT_VIEW.

I know you're fond of it, but I really have a hard time to see any
real use case for all this code, though. Why not just use a
BMessageRunner with B_INVALIDATE to your view? I could understand the
desire to do this entirely client-side to safe resources, but not this
way.
I think Be did it that way, and it limited the utility of
DelayedInvalidate() quite a bit. If you have a DelayedInvalidate() call
in Draw() and you had a non-scheduled call to Draw() (which isn't
uncommon), then you'd have twice as many redraws happening all of a
sudden. Rinse, repeat, and soon you have a ton of BMessageRunners
firing off invalidation after invalidation.
>
The current implementation is smart enough to not allow that, while
smart enough to know when to merge and when not to merge or cancel
DelayedInvalidate() calls. Both methods have the same drawback of using
a server to send the invalidation message, and both send the same amount
of messages overall, but in different places.

The app_server is already smart enough to combine several invalidates, so that doesn't apply here.

The current way has a BView send AS_VIEW_DELAYED_INVALIDATE_RECT with a
BRect attached. The ServerWindow creates a DelayedMessage to be sent at
the time requested, with a merge mode of DM_MERGE_DUPLICATES. The
message is then added to the sender's list when Flush() is called and
the same ServerWindow will, at the right time, receive
AS_VIEW_INVALIDATE_RECT, which will then perform the invalidation.

With BMessageRunner, the codepath is actually quite a bit more complex,
with messages being added to each other, and falling to BRoster, then to
BMessenger, then BMessage::Private::SendMessage(), then finally to the
port, but the overall complexity is not meaningfully different... except
you now no longer have the ability to merge unwanted duplication.

As I said above, the app_server is smart enough to merge invalidations all on its own.
If you really want to do cheaper animations, the best would be a totally different approach, namely to register a periodic update directly with the app_server. That would be something I'd understand, but I still see no reason to have a DelayedInvalidate().

I may have some time tonight to work on it, things are going to be
getting hectic in about fifteen minutes and aren't going to let off
until next year. My bank account doesn't care, though ;-)

BTW, there is no Haiku B1 -- not sure if you came up with that, or if it's already mentioned this way in the documentation. It would be R1B1, but since R1 is all that matters, only that should be noted there, IMO.

Bye,
Axel.


Other related posts:

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