[BColorControl] Various selector bugs

[aldeck]

- The invalidation rect was offseted by a few pixels, it could be seen in prefs/Menu color picker.

- Mouse tracking was offseted (vertically) by a few pixels so you could move the red slider while clicking in the blue area.

- Horizontal (left and right) spacing of the selectors wasn't consistent.

The attached patch solves those three issues, although i need some advice concerning binary compatibility since i've modified the class definition.

[jackburton]

The patch breaks binary compatibility, since you changed the class size. Since there are 8 reserved uint32s, you could use one of them, replace it with a pointer, and allocate the 4 BRects (fRampRects[4]) dinamically. Let's remember, also, that currently our BColorControl doesn't support paletted mode (8 bit mode).

[aldeck]

Ok thanks for the info! I found back the tutorial on binary compatibility. I've got a new patch at home with binary compatibility and i also corrected the the height of the ramps that were subject to rounding errors. Now the ramps are exactly 2 * cellsize pixels height (conforming to the bebook). I started looking at implementing the palette mode. I'll post the patch later today.

[jackburton]

Reassigning to me. I'll apply the eventual patches done by aldeck.

[aldeck]

Ok, here comes the patch :)

- implemented palette (8 bit) mode - implemented partial redraw for the ramp and palette mode - reimplemented offscreen drawing (as discussed in #503) - The invalidation rect was offseted by a few pixels, it could be seen in prefs/Menu color picker. - Mouse tracking was offseted (vertically) by a few pixels so you could move the red slider while clicking in the blue area. - Horizontal (left and right) spacing of the selectors wasn't consistent. - maybe other minor fixes i can't remember right now :)

- adjusted cellsize in prefs/Screen since it was working with a wrong value

Well tested, under load, direct drawing, offscreen drawing, 8 bit mode, palette layouts. Left some todos for later.

Should be good hopefully :) Feel free to comment.

[jackburton]

I had a look at the patch. In theory it looks good, I think I'll apply in the next days. Only a few remarks: - You call Window() in _InitData(), where Window() is still NULL (it's valid only after AttachedToWindow() - I've moved setting fPaletteFrame to _LayoutView(), and called this last method inside SetCellSize(), so that it's resized dynamically. - I've disabled palette mode for now, since the colorspace should be recalculated if the workspace changes colorspace, or the window to which the view belongs is moved to a new workspace with different colorspace (overriding WorkspaceChanged() and ScreenChanged(), probably)

Apart from this, you did a damn good work!

[aldeck]

Ok i see my mistake for the first point, initing the bitmap should definitely be done elsewhere! In fact i uncommented this statement without checking ;) Concerning dynamic colorspace change, thanks for your info, i/we 'll do it later :)

Thanks for your detailed review, changes and comments! Alex

[jackburton]

Replying to aldeck:

Ok i see my mistake for the first point, initing the bitmap should definitely be done elsewhere! In fact i uncommented this statement without checking ;) Concerning dynamic colorspace change, thanks for your info, i/we 'll do it later :) Thanks for your detailed review, changes and comments! Alex

I just noticed that WorkspaceChanged() and ScreenChanged() are BWindow methods, not BView ones. Oh well, we'll have to do something else then. I've applied the patch in r23671. I decided to keep enabled the paletted mode, although I moved its check in AttachedToWindow(). I also changed a couple of other minor things (like the Ramps being calculated wrongly: fCellSize * 2 instead of Bounds().Height() / 4 (only the tracker preferences showed a problem). Thank you again for your work!

I'm closing this bug.

[aldeck]

Thanks for your time! I'm affraid, but there are a few minor issues with your modifications :)

You forgot to apply the changes to prefs/Backgrounds, the old calculation made it look the right size but it was wrong.

Concerning fPaletteFrame in palette mode, it needs one more pixel in height to account for the last horizontal line of the grid (touching the bottom bevel). Thats why i did put an if. But we'd have to call _LayoutView() after we know the colorspace, after AttachedToWindow() ... I'll think about that.

Concerning the rampHeight, Height()/4 is wrong, because, Height()==4*cellSize-1 so there's a rounding error. We can see there's one pixel line missing at the bottom. I think tracker preferences calls it with a different layout, wich i don't handle in "ramp" mode. I'll patch this.

BTW i've just been granted commit access by the dev team! I admit a bit scared to use it right now, but will soon :) I won't hesitate to ask questions on the dev mailing list and trac when needed, and hope we can have fun making haiku!

Thanks Stefano, Alex

[aldeck]

soon i'll commit myself :)

[jackburton]

Replying to aldeck:

Thanks for your time! I'm affraid, but there are a few minor issues with your modifications :)

Ops!

You forgot to apply the changes to prefs/Backgrounds, the old calculation made it look the right size but it was wrong.

You are right. I forgot to commit the file, although I have it changed on my local copy.

Concerning fPaletteFrame in palette mode, it needs one more pixel in height to account for the last horizontal line of the grid (touching the bottom bevel). Thats why i did put an if. But we'd have to call _LayoutView() after we know the colorspace, after AttachedToWindow() ... I'll think about that.

I removed that pixel, tested, and it seemed correct also without it, so I just went this route.

Concerning the rampHeight, Height()/4 is wrong, because, Height()==4*cellSize-1 so there's a rounding error. We can see there's one pixel line missing at the bottom. I think tracker preferences calls it with a different layout, wich i don't handle in "ramp" mode. I'll patch this.

Hmmm okay.

BTW i've just been granted commit access by the dev team! I admit a bit scared to use it right now, but will soon :) I won't hesitate to ask questions on the dev mailing list and trac when needed, and hope we can have fun making haiku!

Great! Sorry if I messed up, please apply the needed fixes yourself :)