View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0005903 | JEDI VCL | 00 JVCL Components | public | 2012-06-10 12:32 | 2012-09-10 14:15 |
| Reporter | Arioch | Assigned To | obones | ||
| Priority | normal | Severity | trivial | Reproducibility | have not tried |
| Status | resolved | Resolution | fixed | ||
| Product Version | Daily / GIT | ||||
| Target Version | Fixed in Version | 3.46 | |||
| Summary | 0005903: remove garbage from JvBaseDLG ? | ||||
| Description | i think those classes would better be removed at all TJvCommonDialogP = class(TJvCommonDialog) public // procedure Execute; virtual; abstract; end; // (rom) alternative to TJvCommonDialogP TJvCommonDialogF = class(TJvCommonDialog) public // function Execute: Boolean; virtual; abstract; end; | ||||
| Tags | No tags attached. | ||||
|
|
I believe they are ancestors used to distinguish between families of dialogs |
|
|
And in those families it was important to make Execute fucntion or procedure ? And now, as in 2005 or 2006 Delphi introduced overloaded Execute members - aren't they anyway obsolete now ? http://docwiki.embarcadero.com/Libraries/en/Vcl.Dialogs.TCommonDialog.Execute https://forums.embarcadero.com/thread.jspa?threadID=72901&tstart=0 (* BTW, in another ticket here there was answer - it was D2005 aka D9 making that change, not D2006. *) |
|
|
C:\Delphi\Libs\JediVCL\jvcl\run>grep -n "TJvCommonDialogP" *.pas JvBaseDlg.pas 48 TJvCommonDialogP = class(TJvCommonDialog) 53 // (rom) alternative to TJvCommonDialogP JvCommonExecDlg.pas 40 TJvCommonExecDialog = class(TJvCommonDialogP) JvDesktopAlert.pas 150 TJvCustomDesktopAlert = class(TJvCommonDialogP) JvDialogActns.pas 42 TJvCommonDialogPClass = class of TJvCommonDialogP; 73 TJvCommonDialogPAction = class(TJvCommonDialogAction) 78 //FDialog: TJvCommonDialogP; 79 function GetDialogClass: TJvCommonDialogPClass; reintroduce; virtual ; 97 // TJvCommonDialogPAction? 283 //=== { TJvCommonDialogPAction } ======================================= ====== 285 constructor TJvCommonDialogPAction.Create(AOwner: TComponent); 287 DialogClass: TJvCommonDialogPClass; 301 function TJvCommonDialogPAction.GetDialogClass: TJvCommonDialogPClass; 306 function TJvCommonDialogPAction.HandlesTarget(Target: TObject): Boolean; JvImageDlg.pas 44 TJvImageDialog = class(TJvCommonDialogP) JvJVCLAboutForm.pas 96 TJvJVCLAboutComponent = class(TJvCommonDialogP) JvTipOfDay.pas 54 TJvTipOfDay = class(TJvCommonDialogP) JvWinDialogs.pas 330 TJvRunDialog = class(TJvCommonDialogP) 365 TJvNewLinkDialog = class(TJvCommonDialogP) 377 TJvAddHardwareDialog = class(TJvCommonDialogP) 385 TJvOpenWithDialog = class(TJvCommonDialogP) 418 TJvExitWindowsDialog = class(TJvCommonDialogP) |
|
|
C:\Delphi\Libs\JediVCL\jvcl\run>grep -ni "TJvCommonDialogF" *.pas JvAddPrinter.pas 45 TJvAddPrinterDialog = class(TJvCommonDialogF) JvBaseDlg.pas 54 TJvCommonDialogF = class(TJvCommonDialog) JvBrowseFolder.pas 178 TJvBrowseForFolderDialog = class(TJvCommonDialogF, IFolderFilter) JvCalc.pas 51 TJvCalculator = class(TJvCommonDialogF) JvDialogActns.pas 43 TJvCommonDialogFClass = class of TJvCommonDialogF; 98 TJvCommonDialogFAction = class(TJvCommonDialogAction) 103 //FDialog: TJvCommonDialogF; 104 function GetDialogClass: TJvCommonDialogFClass; reintroduce; virtua ; 122 TJvBrowseForFolderAction = class(TJvCommonDialogFAction) 126 function GetDialogClass: TJvCommonDialogFClass; override; 149 TJvFloppyFormatAction = class(TJvCommonDialogFAction) 153 function GetDialogClass: TJvCommonDialogFClass; override; 167 TJvControlPanelAction = class(TJvCommonDialogFAction) 171 function GetDialogClass: TJvCommonDialogFClass; override; 311 //=== { TJvCommonDialogFAction } ====================================== ====== 313 constructor TJvCommonDialogFAction.Create(AOwner: TComponent); 315 DialogClass: TJvCommonDialogFClass; 329 function TJvCommonDialogFAction.GetDialogClass: TJvCommonDialogFClass; 334 function TJvCommonDialogFAction.HandlesTarget(Target: TObject): Boolean 346 function TJvBrowseForFolderAction.GetDialogClass: TJvCommonDialogFClass 382 function TJvFloppyFormatAction.GetDialogClass: TJvCommonDialogFClass; 406 function TJvControlPanelAction.GetDialogClass: TJvCommonDialogFClass; JvFindFiles.pas 53 TJvFindFilesDialog = class(TJvCommonDialogF) JvLoginForm.pas 67 TJvCustomLogin = class(TJvCommonDialogF) JvProgressDialog.pas 103 TJvProgressDialog = class(TJvCommonDialogF) JvWinDialogs.pas 178 TJvFormatDriveDialog = class(TJvCommonDialogF) 220 TJvAppletDialog = class(TJvCommonDialogF) 289 // (rom) changed to new TJvCommonDialogF to get better Execute 293 TJvChangeIconDialog = class(TJvCommonDialogF) 349 TJvObjectPropertiesDialog = class(TJvCommonDialogF) 397 TJvDiskFullDialog = class(TJvCommonDialogF) 443 TJvURLAssociationDialog = class(TJvCommonDialogF) 483 TJvMIMEAssociationDialog = class(TJvCommonDialogF) 582 TJvSoftwareUpdateDialog = class(TJvCommonDialogF) |
|
|
So overall i can see no "is" or "as" operators there. Probably JvDialogActns.pas is to be reviewed and updated. After we can be sure if Delphi 2005 or 2006 introduced overloaded Execute function. Proably during update the ...F and ...P versions can be happily merged. |
|
2012-06-13 14:03
|
dialogs_class_diagram.png (972,427 bytes) |
|
|
Attached is the class diagram showing (almost) all dialog classes in the JVCL. Basically, to me there is no clear logic about using TJvCommonDialog, TJvCommonDialogP and TJvCommonDialogF. The TJvCommondDialogD class makes sense as it groups dialogs that are not in CommDlg.dll (see http://docwiki.embarcadero.com/Libraries/en/Vcl.Dialogs.TCommonDialog) So I propose we remove the P and F version in favor of the TJvCommonDialog class, however your remark about overloaded Execute makes me wonder what problem you see here. Do you have more details? Because I must insist on keeping compatibility with Delphi 6 and 7 |
|
|
AS. which tool was used for diagram making ? TJvCommondDialogD class ... groups dialogs that are not in CommDlg.dll I don;t know why it is missed (okay, its children are missed) in your diagram, but grepping sources suggests more specific definition: TJvDiskRes = (dsSuccess, dsCancel, dsSkipfile, dsError); JvCommonDialogD.pas JvCopyError.pas JvDeleteError.pas JvDiskPrompt.pas JvRenameError.pas So one can say, it groups dialog related to filesystem access errors and asking user what to do. That old DOS Abort/Retry/Ignore/Fail prompt in GUI. So i guess it maybe better be renamed accordingly, TJvCommondDiskErrorDialog or like that. -------------- > So I propose we remove the P and F version So we aagree here. ------- > your remark about overloaded Execute makes me wonder what problem you see here. https://forums.embarcadero.com/thread.jspa?threadID=72901&tstart=0 I see potential EAbstract raised if VCL or some 3rd-party or even developers would call new full version of Execute method. I can't look into D7 sources, but i suppose it had only single no-parameter Execute method. Since D2005 the abstract one, designed to be overriden, is the one with parameter, and old no-parameter one just chain-calling the latter. Basically i think Borland should had made old Execute(void) function non-virutal after such changes. > Because I must insist on keeping compatibility with Delphi 6 and 7 So basically either IfDef's to be used in each inheriting class, or that new Execute variant be introduced into D6/D7 and only it be overriden in JVCL dialogs. |
|
|
Since it was not done on that ticket, want to restate: C:\Delphi\Libs\JediVCL\jvcl\design\JvBaseDlgEditor.pas Since Delphi 5 is no more supported, this to be removed: type // (p3) TCommonDialog.Execute is protected in D5... TAccessProtectedCommonDialog = class(TCommonDialog); |
|
|
I used the "modeling" tab of Delphi XE2 to make the diagram. The Execute(ParentWnd: HWND) overload has been added and is in SVN |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2012-06-10 12:32 | Arioch | New Issue | |
| 2012-06-10 14:49 | Arioch | Relationship added | related to 0005899 |
| 2012-06-11 17:36 | obones | Note Added: 0019872 | |
| 2012-06-11 17:36 | obones | Status | new => feedback |
| 2012-06-11 22:54 | Arioch | Note Added: 0019881 | |
| 2012-06-11 22:55 | Arioch | Note Added: 0019882 | |
| 2012-06-11 22:58 | Arioch | Note Added: 0019883 | |
| 2012-06-11 23:00 | Arioch | Note Added: 0019884 | |
| 2012-06-13 11:14 | obones | Relationship added | related to 0003342 |
| 2012-06-13 14:03 | obones | File Added: dialogs_class_diagram.png | |
| 2012-06-13 14:06 | obones | Note Added: 0019950 | |
| 2012-06-13 15:08 | Arioch | Note Added: 0019956 | |
| 2012-06-13 15:08 | Arioch | Note Edited: 0019881 | |
| 2012-06-13 15:10 | Arioch | Note Edited: 0019881 | |
| 2012-06-13 15:14 | Arioch | Note Added: 0019957 | |
| 2012-06-14 11:21 | obones | Note Added: 0019986 | |
| 2012-06-14 11:21 | obones | Status | feedback => resolved |
| 2012-06-14 11:21 | obones | Fixed in Version | => Daily / SVN |
| 2012-06-14 11:21 | obones | Resolution | open => fixed |
| 2012-06-14 11:21 | obones | Assigned To | => obones |
| 2012-09-10 14:15 | obones | Fixed in Version | Daily / SVN => 3.46 |
