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

  • From: Stephan Aßmus <superstippi@xxxxxx>
  • To: haiku-development@xxxxxxxxxxxxx
  • Date: Sun, 22 Nov 2015 22:44:00 +0100

Hi,

Am 22.11.2015 um 02:40 schrieb looncraz:

My final patch, located here: http://files.looncraz.net/svc/ , includes
style corrections even for code that I did not create. I have tried to
be as patient as I can be, and have addressed every concern presented to
me in a timely and, I hope, courteous manner. I have done this for
almost exactly two months now, and have effectively rewritten this patch
twice (much of the fault of that is certainly my own).

Scrollbar in the browser window tells me I am still below 25% of the complete patch.

So far, I think the new API which got introduced is fine. Some remarks below. Because of that, and I think this is what Axel said as well, I tend to think it would be good to commit the patch, and change some of the implementation later, when time permits and if necessary. But the implementation should not impact the new API very much, since most application code does not deal with the new BColorMap.

Ok, two points so far:

* I would suggest "B_FOLLOW_LEFT_TOP" is better (more meaningful) than "B_FOLLOW_DEFAULT".

* I don't like the changes to the textview stuff in HaikuDepot (the CharacterStyle[Data]) so much. My feeling is that the text view API should not be concerned with Interface Kit stuff (beyond graphics primitive types like rgb_color). And API dealing with "color_which" would be such a misplaced (in my view) "dependency". The solution as I see it, would be to implemented the inheritence support I planned for CharacterStyle. If HaikuDepot integrating with live color changes is an important concern, I could live with these parts being commited, and removed again later, once inheritence is implemented.

Other than that, I only spotted one coding style violation in the first 20 or so percent of the patch, which involved a semicolon misplaced on its own line instead of on the end of the line. Clearly an innocent typo. I wouldn't worry about fixing that. Maybe when submitting the patch by whomever applies it.

I hope to find more time to review the rest of the patch soon.

Best regards,
-Stephan

Other related posts:

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