Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#1701 closed bug (fixed)

[BColorControl] Various selector bugs

Reported by: aldeck Owned by: jackburton
Priority: normal Milestone: R1
Component: Kits/Interface Kit Version: R1/pre-alpha1
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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

Attachments (3)

colorControl.diff (6.7 KB) - added by aldeck 11 years ago.
colorControl8.diff (22.3 KB) - added by aldeck 11 years ago.
colorControl9.diff (2.2 KB) - added by aldeck 11 years ago.
soon i'll commit myself :)

Download all attachments as: .zip

Change History (12)

Changed 11 years ago by aldeck

Attachment: colorControl.diff added

comment:1 Changed 11 years ago by 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).

comment:2 Changed 11 years ago by 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.

comment:3 Changed 11 years ago by jackburton

Owner: changed from axeld to jackburton

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

comment:4 Changed 11 years ago by 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.

Changed 11 years ago by aldeck

Attachment: colorControl8.diff added

comment:5 Changed 11 years ago by jackburton

Status: newassigned

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!

comment:6 Changed 11 years ago by 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

comment:7 in reply to:  6 Changed 11 years ago by jackburton

Resolution: fixed
Status: assignedclosed

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

comment:8 Changed 11 years ago by 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

Changed 11 years ago by aldeck

Attachment: colorControl9.diff added

soon i'll commit myself :)

comment:9 in reply to:  8 Changed 11 years ago by 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 :)

Note: See TracTickets for help on using tickets.