View Issue Details

IDProjectCategoryView StatusLast Update
0005903JEDI VCL00 JVCL Componentspublic2012-09-10 14:15
ReporterAriochAssigned Toobones 
PrioritynormalSeveritytrivialReproducibilityhave not tried
Status resolvedResolutionfixed 
Product VersionDaily / GIT 
Target VersionFixed in Version3.46 
Summary0005903: remove garbage from JvBaseDLG ?
Descriptioni 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;
TagsNo tags attached.

Relationships

related to 0005899 resolvedobones TJvOpenWithDialog filename property 
related to 0003342 resolvedobones TJvBrowseForFolderAction action component incomplete and unworkable. Solution provided. 

Activities

obones

2012-06-11 17:36

administrator   ~0019872

I believe they are ancestors used to distinguish between families of dialogs

Arioch

2012-06-11 22:54

developer   ~0019881

Last edited: 2012-06-13 15:10

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. *)

Arioch

2012-06-11 22:55

developer   ~0019882

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)

Arioch

2012-06-11 22:58

developer   ~0019883

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)

Arioch

2012-06-11 23:00

developer   ~0019884

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)

obones

2012-06-13 14:06

administrator   ~0019950

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

Arioch

2012-06-13 15:08

developer   ~0019956

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.

Arioch

2012-06-13 15:14

developer   ~0019957

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);

obones

2012-06-14 11:21

administrator   ~0019986

I used the "modeling" tab of Delphi XE2 to make the diagram.
The Execute(ParentWnd: HWND) overload has been added and is in SVN

Issue History

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