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

  • From: looncraz <looncraz@xxxxxxxxxxx>
  • To: haiku-development@xxxxxxxxxxxxx
  • Date: Sun, 22 Nov 2015 15:01:33 -0600

On 11/22/2015 13:48, Axel Dörfler wrote:

As I said: please get the basics done first. I'm really only pissed at you since you claimed you had gone to lengths to fix all coding style issues when you in fact have not.

I ran every tool, and was pretty sure I had managed to get rid of the style issues. I have addressed every issue you presented, and scanned for other examples of them, and missed a couple. You do realize that this patch is one of the largest single contributions made to Haiku (size-wise) in at least a year, right? And is being made by someone who has only submitted two other, minor, patches previously?

Things will be missed, simple as that. No need to get upset over it. Your anger shows and is contagious. And is certainly misplaced.

Following the coding style is not optional, everyone else does it, and I really don't understand why you have such a hard time doing it.

I've repeatedly stated that I have no problems with it, and every time an issue is pointed out I repair it right away. It's the attitude that comes when those issues are missed in my own examinations that presents an issue.

When reviewing your own code, you tend to group things together, correct? This means you miss errors that stand out to other people. This will happen probably even in this one email. Compound that onto thousands of lines of code and realize you are getting angry about someone missing an 'h' in these, and a space before or after //, even though they may not even be aware that such a style rule exists, even though they've have read the style guidelines several times. Human fallibility is omnipresent.

But beyond style, you did *not* follow any suggestion I had besides the timeout and "dm" naming, and you didn't even get that one right at first shot.

As I've always said, I have different ideas, you have different ideas. If you want something in particular, don't just complain about how it is, offer an alternative. I tried to get you to do this many times, but still (even in this email) you do not.

You did *not* answer to the actual critic I had for your solutions so far in any meaningful way:
1) delayed message implementation is sub-optimal

Please, elaborate. What is sub-optimal. Because, computational complexity-wise, it is approximately the best that can be accomplished by anyone. There are no busy loops, little lock contention, considerations for every possible stall and the response that might have on the calling code, there a common-case optimizations, a simple API commonly already widely in use in the app_server, etc...

What more do you want? Please, be specific.

2) delayed invalidation implementation is poor

What is wrong with it? You've never mentioned this before.

3) Your API decisions are poor (ie. merge/comparison in delayed message)

The primary benefit of DelayedMessage is that it can merge messages according to a set of simple rules. Requiring a callback to make that decision, as you suggested, is not needed. There are, literally, only a few times and methods by which any message will ever be merged with other messages.

4) BColorMap should certainly not be a public API.

I have asked you repeatedly if you wanted me to do it this way and you never responded, even having me correct style issues, and how it sets existing values. NOW you say you don't want it (it is either public API, or it doesn't exist)?

This clearly demonstrates why the discussion should be functionality first. I have spent an incredible amount of time on BColorMap and its widespread deployment around the system. I greatly modified the Appearance preflet to use it, and optimized the app_server using it. You have had ample opportunity, even upon my repeated requests, to not have BColorMap.

In lieu of BColorMap, the ONLY working solution is to add numerous methods (FindColor, AddColor, SetColor, HasColor) to BMessage directly, and lose some of the efficiency in app_server. Unless, of course, you want all 3rd party apps to have to decode the B_COLORS_UPDATED BMessage manually, which is more work than it is worth, so few will bother, meaning future apps will be less likely to have proper color behavior.

5) Your development process is questionable (now, lets put a timeout of 25000 here, and a B_HOST_TO_BIG_ENDIAN here. I have no idea why, but it sure looks good). That is sloppy.

The timeout I genuinely don't know how that even got there. The endianness is a directly copy from BView's handling and proved itself to be required. I wouldn't think it would be, but I just made the assumption that it would be needed because BMessage uses a different internal storage format and rgb_color doesn't follow suit. It's not sloppy, it's following what others have done when doing the exact same thing yourself.


I haven't heard any good arguments from you concerning these.
You asked for a review, and you got one for most of your work, but you aren't open to discuss your code at all. You just like your solution too much.

I asked for specific suggestions, and received silence or derision. A proper conversation flows like this:

YOU: I don't like this, can you do it like this instead?
ME: Oh, I did it this way because X, Y, &Z.
YOU: I see, maybe you should think of A & B, and R, H, and T instead.
ME: Okay, I can do that, glad you caught this early!

Instead, this is what we see:

YOU: I don't like this.
ME: Why? I could do this way, or this way, what do you want?
YOU:
ME: Okay, I have rewritten this to be BColorMap...
YOU: There's a typo, a missing extra line, and a missing space, oh, and use SetInt32, you're wasting my time.
ME: Thanks, fixed all that now, do we want to keep using this?
YOU:
ME: (days later) Okay, so I am done with the requested changes and ready to have this reviewed:
YOU: You're wasting my time... I don't like this, I don't want BColorMap

This is what has been happening. Maybe you think you are being more constructive in your responses than you are?


All *you* are talking about are the numerous coding style violations.

I'm only talking about their prioritization over functionality. I don't mind repairing them, and have even stated that they will naturally become fewer as I acclimate to the coding style - assuming I continue to contribute at all.

Of course you are wasting my time when I have to tell you more than once about each violation. I can't imagine that you are too dumb to understand this.

There was only one time when a single violation was not repaired, and I think that was just because I reverted to an earlier version at one point and accidentally brought the style issue back in.

And I definitely don't hate you, but I have pretty much lost any respect for you throughout this whole thing.

I didn't think you did, that was an e-mail to me :p. I think, though, you may have been expecting more from me than you should have.

As I said, I'm not even against merging your code (once the style issues are solved), but you have to accept that its quality isn't that great. There is a lot more to development than having a look at how it works in the end.

The style issues, AFAICT, have been solved.

And, in reality, there is nothing more to developing software than its functionality. The computer doesn't care at all about styling issues. What we see here, though, is you having me repair styling issues without even discussing if the file in which I am correcting style issues should even exist.

Who cares about a missing space if I you want me to delete the implementation entirely?

I've tried to be as courteous as possible, even after you told me I was wasting your time. I made my changes, took a couple days to review things, then decided I really can't afford to keep going through this cycle. We should have completed this long ago, and would have if your feedback had been more constructive.

--The loon

Other related posts:

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