Bugzilla – Full Text Bug Listing
| Summary: | Add Support for ToolItems | ||
|---|---|---|---|
| Product: | z_Archived | Reporter: | Thomas Schindl <tom.schindl> |
| Component: | Nebula | Assignee: | Thomas Schindl <tom.schindl> |
| Status: | RESOLVED FIXED | QA Contact: | |
| Severity: | normal | ||
| Priority: | P3 | CC: | cgross |
| Version: | unspecified | ||
| Target Milestone: | --- | ||
| Hardware: | PC | ||
| OS: | Mac OS X - Carbon (unsup.) | ||
| Whiteboard: | |||
|
Description
Thomas Schindl
Chris, if you could spend a minute. I'd appreciate if you can take a look at my implementation which is found in toolitem-branch of PGroup. Hi Tom, Here's some quick comments. I'm not quite sure what some of the code is doing so I may be missing some things. Comments: -AbstractToolItemRenderer needs comments -The method API for AbstractToolItemRenderer#paint seems strange. I'd rather have a 'min' property on the renderer rather than pass in an array in the value argument and require an extra doPaint method. -The method calculateSizes is confusing to me. Generally doesn't feel right even though I'm not exactly sure what its returning. -I'd rather see something where the group strategy wasn't responsible for drawing the toolitems. Instead something where the group strategy simply marked off an area for the toolbar and PGroup would draw the items. That would make it easy to create new group strategies. This is a small point though. It's not too important. -PGroup#getToolItems returns a List (SWT generally sticks to arrays) -I'd make getToolItems public. -I'd make getToolItemRenderer public too. regards, -Chris (In reply to comment #2) > Hi Tom, > > Here's some quick comments. I'm not quite sure what some of the code is doing > so I may be missing some things. > > Comments: > -AbstractToolItemRenderer needs comments ok. > -The method API for AbstractToolItemRenderer#paint seems strange. I'd rather > have a 'min' property on the renderer rather than pass in an array in the value > argument and require an extra doPaint method. ok > -The method calculateSizes is confusing to me. Generally doesn't feel right > even though I'm not exactly sure what its returning. It calculates the 2 different sizes possible for a ToolItem (if not enough space is given items who hold text and image are hide the text) > -I'd rather see something where the group strategy wasn't responsible for > drawing the toolitems. Instead something where the group strategy simply > marked off an area for the toolbar and PGroup would draw the items. That would > make it easy to create new group strategies. This is a small point though. > It's not too important. Ok will try to implement it. > -PGroup#getToolItems returns a List (SWT generally sticks to arrays) ok > -I'd make getToolItems public. ok > -I'd make getToolItemRenderer public too. ok > > regards, > -Chris Thanks for the fast response will implement those changes ASAP and ping on this bug. Ok. I implemented all your suggestions. (In reply to comment #4) > Ok. I implemented all your suggestions. Looks good. More comments on methods of the AbstractToolItemRenderer would be nice. An implementer is going to have a hard time knowing what to return from calculateSizes or what the min property is. ok docu is on my TODO list and then'll merge the stuff back to HEAD. Thanks for your help. added JavaDoc and released changes to HEAD |