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

  • From: Axel Dörfler <axeld@xxxxxxxxxxxxxxxx>
  • To: haiku-development@xxxxxxxxxxxxx
  • Date: Mon, 23 Nov 2015 21:25:16 +0100

Am 22.11.2015 um 02:40 schrieb looncraz:

My final patch, located here: http://files.looncraz.net/svc/

Next round of review, still not through, but I think I got all the important things.
First of all, I think the API changes in BView are a nice addition, and also nicely done.

I would like to get some discussion going on why you did some stuff the way you did it.

There are two use cases of UI color changes:
1) one color is changed (via slider in Appearance)
2) all colors are changed as part of a theme change

You only get the update timing issues for case 1), and the usual solution that one would come up with would be to limit the update interval from the source.
For 2), the preferred solution would be to be able to set the UI colors atomically, ie. using a set_ui_colors() method.

The only use for BColorMap client side is that you know which view color you have to update (instead of just all of them).
However, you still have to invalidate the view, which limits the usefulness of BColorMap a lot.
I don't see any value in knowing which 23 UI colors the theme changed, but please enlighten me why this could be useful.

I could imagine that adding AddColor(), FindColor(), HasColor(), SetColor(), GetColor() to BMessage could be a somewhat nice addition,
although using B_INT32_TYPE would then be questionable. Once we want to support more than 8-bits per channel, such methods would be nice to have, anyway, though.

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).

BView: having virtual methods to deal with the color/font updates would be nice, but it's probably not worth it considering we only have 4 slots left.

Desktop::ColorUpdated() Why mark the window as dirty? Only the decorator should be marked dirty -- the window does the invalidate itself already (if needed). Calling it for each changed color looks very expensive, anyway (for really little gain).
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?

ServerWindow/invalidate: why not find the view by token (fast lookup) instead of walking over the complete view hierarchy?
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.

TabDecorator: there is no "virtually constant"; please use the 'f' prefix for member variables.

Bye,
Axel.

Other related posts:

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