View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0005305 | JEDI VCL | 00 JVCL Components | public | 2010-08-19 17:02 | 2011-06-07 17:28 |
| Reporter | ralfiii | Assigned To | obones | ||
| Priority | normal | Severity | feature | Reproducibility | always |
| Status | resolved | Resolution | won't fix | ||
| Product Version | 3.39 | ||||
| Target Version | Fixed in Version | ||||
| Summary | 0005305: Allow use of TJvFormPlacement without AppStorage | ||||
| Description | I'm just porting an old app that used the RxLibrary to Jedi. Currently I have to replace all TFormStorages with TJvFormStorages which is OK. BUT: I also have to add a TJvAppRegistryStorage (or whatever I need as storage) on every single form. Stupid work. I suppose most times all forms in an application will use the same AppStorage, so I added (very simple) support for that. I also added code to allow defining a default AppStorage-type, so that for your apps you only need to drop a TJvFormStorages, the defined AppStorage is created automatically on demand (if no other is defined already). | ||||
| Tags | No tags attached. | ||||
|
2010-08-19 17:02
|
RxPatch.patch (1,518 bytes)
Index: JvAppStorage.pas
===================================================================
--- JvAppStorage.pas (revision 14367)
+++ JvAppStorage.pas (working copy)
@@ -959,6 +959,8 @@
);
{$ENDIF UNITVERSIONING}
+var DefaultAppStorage : TJvCustomAppStorage;
+
implementation
uses
@@ -1296,6 +1298,7 @@
FReadOnly := False;
FInternalTranslateStringEngine := TJvTranslateString.Create(Self);
FSynchronizeFlushReload := False;
+ DefaultAppStorage:=self;
end;
destructor TJvCustomAppStorage.Destroy;
Index: JvFormPlacement.pas
===================================================================
--- JvFormPlacement.pas (revision 14367)
+++ JvFormPlacement.pas (working copy)
@@ -291,6 +291,10 @@
uses
Consts,
JclStrings,
+{$DEFINE CreateRegAppStorageOnDemand} // If not AppStorage is defined create a TJvAppRegistryStorage as default storage
+{$IFDEF CreateRegAppStorageOnDemand}
+ JvAppRegistryStorage,
+{$ENDIF}
JvJCLUtils, JvPropertyStorage;
const
@@ -345,6 +349,16 @@
inherited Loaded;
if not (csDesigning in ComponentState) then
begin
+ if not assigned(FAppStorage) then
+ begin
+{$IFDEF CreateRegAppStorageOnDemand}
+ if not assigned(DefaultAppStorage) then
+ AppStorage:=TJvAppRegistryStorage.Create(Owner);
+{$ENDIF}
+ if assigned(DefaultAppStorage) then
+ AppStorage:=DefaultAppStorage;
+ end;
+
ResolveAppStoragePath;
if Loading then
SetEvents;
|
|
|
You don't need that. Put a single AppStorage in a datamodule (or create it in a unit) and link it to all the form storages. Even better, create an ancestor form that has the form storage on it and make all forms inherit from it. This way you only have to define the link once. The way it has been separated is for flexibility, we won't be adding back the rigidity from the original design. |
|
|
I am not suggesting to reduce the flexibilty, I am only suggesting a way to reduce the work for the developer. With the patch you can use the FormStorage in a simpler way than currently required but you can still do anything as currently. So I don't see any downsides of this modifications. |
|
2010-08-20 13:01
|
RxPatch2.patch (3,131 bytes)
Index: JvAppStorage.pas
===================================================================
--- JvAppStorage.pas (revision 14367)
+++ JvAppStorage.pas (working copy)
@@ -959,6 +959,8 @@
);
{$ENDIF UNITVERSIONING}
+var DefaultAppStorage : TJvCustomAppStorage; //!
+
implementation
uses
@@ -1296,10 +1298,13 @@
FReadOnly := False;
FInternalTranslateStringEngine := TJvTranslateString.Create(Self);
FSynchronizeFlushReload := False;
+ DefaultAppStorage:=self; //!
end;
destructor TJvCustomAppStorage.Destroy;
begin
+ if DefaultAppStorage=self then //!
+ DefaultAppStorage:=nil; //!
if FlushOnDestroy then
Flush;
FreeAndNil(FInternalTranslateStringEngine);
Index: JvFormPlacement.pas
===================================================================
--- JvFormPlacement.pas (revision 14367)
+++ JvFormPlacement.pas (working copy)
@@ -111,6 +111,7 @@
procedure FormDestroy(Sender: TObject);
function GetForm: TForm;
procedure SetAppStorage(const Value: TJvCustomAppStorage);
+ function Store_AppStorePath : boolean; //!
protected
procedure ResolveAppStoragePath;
procedure Loaded; override;
@@ -141,7 +142,7 @@
published
property Active: Boolean read FActive write FActive default True;
property AppStorage: TJvCustomAppStorage read FAppStorage write SetAppStorage;
- property AppStoragePath: string read FAppStoragePath write SetAppStoragePath;
+ property AppStoragePath: string read FAppStoragePath write SetAppStoragePath stored Store_AppStorePath; //!
property MinMaxInfo: TJvWinMinMaxInfo read FWinMinMaxInfo write SetWinMinMaxInfo;
property Options: TPlacementOptions read FOptions write FOptions default [fpState, fpSize, fpLocation];
property PreventResize: Boolean read FPreventResize write SetPreventResize default False;
@@ -291,6 +292,10 @@
uses
Consts,
JclStrings,
+{$DEFINE CreateRegAppStorageOnDemand} //! If not AppStorage is defined create a TJvAppRegistryStorage as default storage
+{$IFDEF CreateRegAppStorageOnDemand}
+ JvAppRegistryStorage,
+{$ENDIF}
JvJCLUtils, JvPropertyStorage;
const
@@ -345,6 +350,17 @@
inherited Loaded;
if not (csDesigning in ComponentState) then
begin
+ //! Make use of FormStorage simpler
+ if not assigned(FAppStorage) then
+ begin
+{$IFDEF CreateRegAppStorageOnDemand}
+ if not assigned(DefaultAppStorage) then
+ AppStorage:=TJvAppRegistryStorage.Create(Owner);
+{$ENDIF}
+ if assigned(DefaultAppStorage) then
+ AppStorage:=DefaultAppStorage;
+ end;
+
ResolveAppStoragePath;
if Loading then
SetEvents;
@@ -1357,6 +1373,12 @@
ReplaceComponentReference(Self, Value, TComponent(FAppStorage));
end;
+//! reduce footprint in dfm: Don't store path if it was not modified
+function TJvFormPlacement.Store_AppStorePath : boolean;
+begin
+ result:=FAppStoragePath<>cFormNameMask+'\';
+end;
+
{ TJvFormStorageStringList }
procedure TJvFormStorageStringList.Assign(Source: TPersistent);
|
|
|
...and, just for the sake of completeness: The 2nd patch (includes the 1st one) does a bit cleanup when the default AppStorage is destroyed. It also reduces the footprint of the dfm file by not storing the AppPath if it still holds the default value. |
|
|
And, one more: In TJvAppRegistryStorage the default value for UseOldDefaultRoot should be set to True. Otherwise for lazy developers that forgot to set the Root-value the storage puts values directly under deposits by default under HKCU HKCU/Software/%CompanyName%/%ApplicationName% is clearly a better default location |
|
|
And: The Form storage utilty functions like "InternalSaveFormPlacement" are only used by TJvFormPlacement. So these functions could/should be moved out of unit JvJVCLUtils.pas (which btw. holds a lot of very clever and handy functions) to JvFormPlacement.pas |
|
|
...sorry, even more. CrtResString in JvJVCLUtils does not work properly on multi monitor systems. Use Result := Format( '(%d/%d)', [Screen.DesktopWidth, Screen.DesktopHeight]); instead. I guess there are a few more inherited flaws in the FormStorage-component. Should I post further notes here or does nobody look into posts with "feedback" status anyway?! |
|
|
...and the form state (e.g. Maximized) is not restored with Delphi Version < 2009... |
|
2010-08-25 16:52
|
Placement.patch (6,435 bytes)
Index: JvAppRegistryStorage.pas
===================================================================
--- JvAppRegistryStorage.pas (revision 14328)
+++ JvAppRegistryStorage.pas (working copy)
@@ -1,4 +1,4 @@
-{-----------------------------------------------------------------------------
+{-----------------------------------------------------------------------------
The contents of this file are subject to the Mozilla Public License
Version 1.1 (the "License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
@@ -85,6 +85,7 @@
FCompanyNameErrorHandled: Boolean;
function GetStorageOptions: TJvAppRegistryStorageOptions;
procedure SetStorageOptions(const Value: TJvAppRegistryStorageOptions);
+ function Store_Root : boolean; //!
protected
procedure Loaded; override;
@@ -125,7 +126,7 @@
constructor Create(AOwner: TComponent); override;
published
property RegRoot: TJvRegKey read GetRegRoot write SetRegRoot default hkCurrentUser;
- property Root read GetRoot write SetRoot;
+ property Root read GetRoot write SetRoot stored Store_Root; //!
property SubStorages;
property FlushOnDestroy;
property UseOldDefaultRoot: Boolean read FUseOldDefaultRoot write SetUseOldDefaultRoot stored True default False ;
@@ -182,7 +183,7 @@
begin
inherited Create(AOwner);
FRegHKEY := HKEY_CURRENT_USER;
- FUseOldDefaultRoot := False;
+ UseOldDefaultRoot := True; //! False;
FAppNameErrorHandled := False;
FCompanyNameErrorHandled := False;
end;
@@ -573,6 +574,12 @@
(Inherited StorageOptions).Assign(Value);
end;
+//! Don't store root if default path should be used anyway
+function TJvAppRegistryStorage.Store_Root : boolean;
+begin
+ result:=not FUseOldDefaultRoot;
+end;
+
{$IFDEF UNITVERSIONING}
initialization
RegisterUnitVersion(HInstance, UnitVersioning);
Index: JvAppStorage.pas
===================================================================
--- JvAppStorage.pas (revision 14328)
+++ JvAppStorage.pas (working copy)
@@ -959,6 +959,8 @@
);
{$ENDIF UNITVERSIONING}
+var DefaultAppStorage : TJvCustomAppStorage; //!
+
implementation
uses
@@ -1296,10 +1298,13 @@
FReadOnly := False;
FInternalTranslateStringEngine := TJvTranslateString.Create(Self);
FSynchronizeFlushReload := False;
+ DefaultAppStorage:=self; //!
end;
destructor TJvCustomAppStorage.Destroy;
begin
+ if DefaultAppStorage=self then //!
+ DefaultAppStorage:=nil; //!
if FlushOnDestroy then
Flush;
FreeAndNil(FInternalTranslateStringEngine);
Index: JvFormPlacement.pas
===================================================================
--- JvFormPlacement.pas (revision 14328)
+++ JvFormPlacement.pas (working copy)
@@ -111,6 +111,7 @@
procedure FormDestroy(Sender: TObject);
function GetForm: TForm;
procedure SetAppStorage(const Value: TJvCustomAppStorage);
+ function Store_AppStorePath : boolean; //!
protected
procedure ResolveAppStoragePath;
procedure Loaded; override;
@@ -141,7 +142,7 @@
published
property Active: Boolean read FActive write FActive default True;
property AppStorage: TJvCustomAppStorage read FAppStorage write SetAppStorage;
- property AppStoragePath: string read FAppStoragePath write SetAppStoragePath;
+ property AppStoragePath: string read FAppStoragePath write SetAppStoragePath stored Store_AppStorePath; //!
property MinMaxInfo: TJvWinMinMaxInfo read FWinMinMaxInfo write SetWinMinMaxInfo;
property Options: TPlacementOptions read FOptions write FOptions default [fpState, fpSize, fpLocation];
property PreventResize: Boolean read FPreventResize write SetPreventResize default False;
@@ -291,6 +292,10 @@
uses
Consts,
JclStrings,
+{$DEFINE CreateRegAppStorageOnDemand} //! If not AppStorage is defined create a TJvAppRegistryStorage as default storage
+{$IFDEF CreateRegAppStorageOnDemand}
+ JvAppRegistryStorage,
+{$ENDIF}
JvJCLUtils, JvPropertyStorage;
const
@@ -345,6 +350,17 @@
inherited Loaded;
if not (csDesigning in ComponentState) then
begin
+ //! Make use of FormStorage simpler
+ if not assigned(FAppStorage) then
+ begin
+{$IFDEF CreateRegAppStorageOnDemand}
+ if not assigned(DefaultAppStorage) then
+ AppStorage:=TJvAppRegistryStorage.Create(Owner);
+{$ENDIF}
+ if assigned(DefaultAppStorage) then
+ AppStorage:=DefaultAppStorage;
+ end;
+
ResolveAppStoragePath;
if Loading then
SetEvents;
@@ -1357,6 +1373,12 @@
ReplaceComponentReference(Self, Value, TComponent(FAppStorage));
end;
+//! reduce footprint in dfm: Don't store path if it was not modified
+function TJvFormPlacement.Store_AppStorePath : boolean;
+begin
+ result:=(FAppStoragePath<>cFormNameMask+'\') and (FAppStoragePath<>cFormNameMask);
+end;
+
{ TJvFormStorageStringList }
procedure TJvFormStorageStringList.Assign(Source: TPersistent);
Index: JvJVCLUtils.pas
===================================================================
--- JvJVCLUtils.pas (revision 14328)
+++ JvJVCLUtils.pas (working copy)
@@ -3908,8 +3908,13 @@
function CrtResString: string;
begin
- Result := Format('(%dx%d)', [GetSystemMetrics(SM_CXSCREEN),
- GetSystemMetrics(SM_CYSCREEN)]);
+//! Result := Format('(%dx%d)', [GetSystemMetrics(SM_CXSCREEN),
+//! GetSystemMetrics(SM_CYSCREEN)]);
+
+//! New Resolution Identifier, because old method did not work
+// properly for Multi-screen systems (returned only width/height of current screen)
+ Result := Format( '(%dx%d)',
+ [Screen.DesktopWidth, Screen.DesktopHeight]);
end;
function ReadPosStr(AppStorage: TJvCustomAppStorage; const Path: string): string;
@@ -4146,13 +4151,12 @@
PostMessage(Application.Handle, WM_SYSCOMMAND, SC_MINIMIZE, 0);
Exit;
end;
- {$IFNDEF DELPHI2010_UP} // Using this Hack didn't work with D2010
+ {$IFNDEF DELPHI2010_UP} //! Using this Hack didn't work with D2010
if Form.FormStyle in [fsMDIChild, fsMDIForm] then
TWindowState(Pointer(@Form.WindowState)^) := WinState
else
- {$ELSE}
+ {$ENDIF}
Form.WindowState := WinState;
- {$ENDIF}
end;
Form.Update;
end;
|
|
|
Attached is a patch that fixes all mentioned problems. *) feature: JvFormStorage uses application wide default AppStorage if one is defined *) feature: dfm footprint reduced for TJvFormPlacement (AppStoragePath) *) feature: dfm foorprint recuded for TJvAppRegistryStorage (Root) *) fix: JvJVCL.CrtResString returned wrong value for multi monitor systems (used by JFormPlacement) *) fix: Form state (e.g. Maximized) was not returned for Delphi Versions < 2009 |
|
|
Hi, I've added your patch for the CrtResString problem, but the problem of the storing the Form state was already fixed. I'm not sure that we should your DefaultStorage solution. I think we need a little bit more discussion about it. Kind regards Jens |
|
|
Fine. What's with the reduced footprint-stuff? Regarding DefaultStorage: I'd just like to remind that the adoptions we made have no effects whatsoever on an existing application, I hope you understand that. The whole thing only has an effect if you "forgot" to wire a Formstorage (you have to admit, this makes the use of the formstorage a lot easier) So I don't see any disadvantages. If you feel different please let me know. |
|
|
I don't like that DefaultStorage idea at all. The point is to have them split, and independent. Furthermore, there are issues when many components all write to the same file at once. Most obvious one is having parameters overwritten with wrong values. The way to overcome this is to centralize the storage component in a datamodule (for instance) and have all the others use it. So to me, sorry, allowing people not to think for a split second means that we have more and more requests about something they should have thought of when creating their program |
|
|
I agree with you. |
|
|
What we could add, though, is the raise of an exception if AppStorage is not set and the SavePlacement/LoadPlacement ReadXXX/WriteXXX methods are called. That exception would mention that one needs to set AppStorage to a global instance of a TJvAppStorage derived class And we could also add a "IgnoreNilAppStorage" boolean property that defaults to False but can be set to True to avoid the exception if a developer needs that. |
|
|
As I said in my mail: You don't like the idea of a default classtype. I understand that and I agree. What do think about the idea of the default instance? In other words: Instead of raising an exception when no AppStorage is defined simply use the same instance of a TJvAppStorage-descendant that was used the last time. (so not create a new object, but re-use the one that is already in use). This basically results in what you recommended - just instead of a central datamodule the component grabs the storage-object it needs itself. |
|
|
To complete that: Using that approach you as a developer can throw a formstorage and an appstorage on the main form of a project, configure the appstorage and wire them together. On all other forms dropping the formstorage would be enough. So there's no need for a central datamodule and there's no danger that you forget to wire a formstorage. |
|
|
I've seen this implementation in FastReport, and, to be "nice", I'll just say that I did not like it at all. It was buggy, error prone, and sometimes prevented anyone from using multiple instances of that "common" component. In the end, I'm not at all in favor of somehow magically wiring things together when it takes so little time to do it manually. |
|
|
Agreed! |
|
|
Then we won't change this, sorry. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2010-08-19 17:02 | ralfiii | New Issue | |
| 2010-08-19 17:02 | ralfiii | File Added: RxPatch.patch | |
| 2010-08-20 11:39 | obones | Note Added: 0017588 | |
| 2010-08-20 11:39 | obones | Status | new => feedback |
| 2010-08-20 12:00 | ralfiii | Note Added: 0017609 | |
| 2010-08-20 13:01 | ralfiii | File Added: RxPatch2.patch | |
| 2010-08-20 13:08 | ralfiii | Note Added: 0017611 | |
| 2010-08-20 16:20 | ralfiii | Note Added: 0017613 | |
| 2010-08-23 13:41 | ralfiii | Note Added: 0017618 | |
| 2010-08-23 14:07 | ralfiii | Note Added: 0017619 | |
| 2010-08-25 14:20 | ralfiii | Note Added: 0017622 | |
| 2010-08-25 16:52 | ralfiii | File Added: Placement.patch | |
| 2010-08-25 16:59 | ralfiii | Note Added: 0017626 | |
| 2010-09-06 00:32 | jfudickar | Note Added: 0017649 | |
| 2010-09-07 17:01 | ralfiii | Note Added: 0017659 | |
| 2010-10-11 10:27 | obones | Note Added: 0017865 | |
| 2010-10-11 23:36 | jfudickar | Note Added: 0017875 | |
| 2010-10-12 09:29 | obones | Note Added: 0017876 | |
| 2010-10-13 15:46 | ralfiii | Note Added: 0017881 | |
| 2010-10-13 15:51 | ralfiii | Note Added: 0017882 | |
| 2010-11-09 14:46 | obones | Note Added: 0018054 | |
| 2010-11-09 16:14 | jfudickar | Note Added: 0018064 | |
| 2011-06-07 17:28 | obones | Note Added: 0018573 | |
| 2011-06-07 17:28 | obones | Status | feedback => resolved |
| 2011-06-07 17:28 | obones | Resolution | open => won't fix |
| 2011-06-07 17:28 | obones | Assigned To | => obones |
