View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002312 | JEDI VCL | 00 JVCL Components | public | 2004-11-12 07:11 | 2004-11-23 07:11 |
| Reporter | glchapman | Assigned To | user72 | ||
| Priority | normal | Severity | major | Reproducibility | always |
| Status | resolved | Resolution | won't fix | ||
| Product Version | 3.00 BETA 2 | ||||
| Target Version | Fixed in Version | ||||
| Summary | 0002312: Some Jvg properties of object-type directly write to fields | ||||
| Description | Some properties in the Jvg family of controls are declared like this: property Bevel: TJvgBevelOptions read FBevel write FBevel; where TJvgBevelOptions is a class inheriting from TPersistent, and FBevel is an instance of the class created and freed by the object declaring the property. I believe directly exposing object-type fields to writes like this is considered bad practice (since it can lead to leaks or access violations). Instead, the property should have a write method which uses an overridden Assign method to assign the passed-in value to the (field) object. In the attached zip are two diffs showing changes to fix this problem in JvgCommClasses and JvgSpeedButton (also copies of the changed units). I think similar changes will have to be made in the other Jvg units if you agree that this is a problem which should be fixed. | ||||
| Additional Information | The Assign methods do not assign the value of the OnChanged event, since this is generally used to inform the owning control owning that some part of it has changed. Also, the write methods do not call Changed. This is because it looks to me that, in the cases here, Changed will be called when the individual properties are changed (during Assign). | ||||
| Tags | No tags attached. | ||||
|
2004-11-12 07:11
|
JvgObjPropSet.zip (14,775 bytes) |
|
|
> I think similar changes will have to be made in > the other Jvg units if you agree that this is > a problem which should be fixed. Most definitely. Do you have a list of candidates? |
|
|
I don't have a list, but after a quick glance through some of the units, it appears that all properties whose type is one of the types in JvgCommClasses are broken. Also, I noted several component-type properties which do not register for FreeNotification with the components they are referencing (e.g., WallpaperImage in TJvgAskListBox). After looking at this, I realized my initial set of changes to JvgCommClasses was not thorough enough; all the classes need Assign methods in preparation for adding in the property write methods. So attached is a new zip file with a new version of JvgCommClasses (JvgSpeedButton.pas is still included, but it is unchanged). In general, the Assign methods now directly assign to the fields (except where the fields are objects), and then call Changed at the end -- it seemed like it was probably a good idea to limit the number of Changed events. (I also had broken TJvgTwainColors by assigning to the RGB properties rather than FromColor and ToColor; that's now fixed.) Do you want me to try to work on a more extensive patch? |
|
2004-11-14 11:37
|
JvgObjPropSet2.zip (17,846 bytes) |
|
|
> Do you want me to try to work on a more extensive patch? That would be greatly appreciated |
|
|
Well, I started working on a larger patch, but I'm stopping now: there are too many problems with the Jvg units. Aside from the problems mentioned earlier, several components make use of a pattern where they have a private TBitmap field which is sometimes used to refer to Image.Picture.Bitmap, where Image is the value of a TImage property (e.g., TJvgAskListBox, which sets WallpaperBmp to FWallpaperImage.Picture.Bitmap). This means that if anything changes the Image's Picture, the components are left with a stray pointer. Also, there is frequent use of: try DoSomething except end; Much of the time, it appears this is used when a string to numeric conversion might fail (e.g., the TJvgPrintCrossTable.CalcResults monstrosity), but of course another exception may have been anticipated. Who knows? Also, JvgHint seems to have misunderstood the VCL hint system (unless something major changed after D5). It sets up TJvgHint.NewShowHint as the Application.OnShowHint handler, then ends up creating and showing a new hint window every time this is called; as far as I can tell, these hint windows are not freed until application shutdown. Also, JvgButton uses a cumbersome method of iterating all components owned by a form every time an OnBlinkTimer event occurs. Unfotunately, this won't work if the button is on a frame; then it will be owned by the frame, not the parent form. Anyway, I've decided not to use the Jvg units anymore, so I won't be submitting a further patch. |
|
|
Fair enough. If anyone is keen on cleaning up the Globus units, you are welcome but I'll leave this issue now as well. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2004-11-12 07:11 | glchapman | New Issue | |
| 2004-11-12 07:11 | glchapman | File Added: JvgObjPropSet.zip | |
| 2004-11-13 14:17 |
|
Note Added: 0005622 | |
| 2004-11-13 14:17 |
|
Status | new => feedback |
| 2004-11-14 11:37 | glchapman | Note Added: 0005627 | |
| 2004-11-14 11:37 | glchapman | File Added: JvgObjPropSet2.zip | |
| 2004-11-15 11:42 |
|
Note Added: 0005632 | |
| 2004-11-22 11:37 | anonymous | Note Added: 0005688 | |
| 2004-11-23 07:11 |
|
Status | feedback => resolved |
| 2004-11-23 07:11 |
|
Resolution | open => won't fix |
| 2004-11-23 07:11 |
|
Assigned To | => user72 |
| 2004-11-23 07:11 |
|
Note Added: 0005692 |
