View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0036166||Lazarus||LCL||public||2019-10-12 00:42||2019-10-30 11:46|
|Reporter||Sergio Fernandez||Assigned To||Bart Broersma|
|Summary||0036166: SaveToFile in TValueListEditor not saving data|
|Description||when I use SaveToFile in TValueListEditor, only the title captions save to the xml file, none of the data fields actually save.|
|Steps To Reproduce||Add a TValueListEditor to your form.|
Add some data using InsertRow
then Call the SaveTofile procedure.
|Tags||No tags attached.|
|Fixed in Revision||r62043, r62044, r62055|
Do you happen to know if SaveToFile exists in Delphi's TValueListEditor?
And if so, in what format does it save the data. Does it also save KeyOptions along with the data?
Also, if Delphi has LoadFromFile: can you load if KeyAdd, KeyDelete and/or KeyEdit are NOT in KeyOptions?
If Delphi does not support this, we have the freedom to decide how we want to implement these.
IMO save should then only save key/value pairs, load should only be possible if KeyAdd and KeyDelete are in KeyOptions.
||There is no SaveToFile method in Delphi's TValueListEditor.|
So, we are free to implement it as we see fit.
Mutatis mutandis for LoadFromFile.
Personally I would save/load only the data (maybe also ItemProps???)
Hmm: LoadFromCSV will silently add extra columns (if it finds more than 1), which is definitely unwanted.
It already can save just the data using the Strings.SaveTofile.. or Stream which I use now.
The issue is that the base grid is not including this when it saves the headers.
But I can't have the Strings.Saveto… include the headers with the way I use it, it will break at least 3 of my apps but, I can
settle with modifying the base grid SaveTo….. to include the this info etc.. At least it would be a good away to reload the complete control if needed.
The whole TValueListEditor suffers from being derived from TCustomStringGrid IMO.
In essence it is a simple tool to edit key-value pairs, that happens to use a TStringGrid.
It should not expose the tons of grid methods/properties it does now.
In the past this already has caused serious problems.
Anyhow, it is what it is.
IMO, given the functionality of TValueListEditor, I would just save the data.
In hindsight it might have been a better idea to split saving content from saving layout in grids.
For saving the layout we have TXMLPropStorage.
(We also don't save layout/propertie of a TSynEdit when we do SaveTo... do we?)
It might not be a big problem to re-implement SaveContent (currently it does not save the cells because fGrid.Celda[i,j]^.Text is empty (except for the headers), because of the way SetCells is implemented (it updates Strings property).
Loading it will be the big problem though.
> include the headers with the way I use it,
Having headers and what they are is controlled by DisplayOptions and TitleCaptions, so now we have to save/restore those as well?
||Please try with r62043.|
||Since the new behaviour differs from default grid behaviour (it resets rowcount to the stored value), I'm not sure this can be merged to fixes branch.|
As for the file format: I would like to draw your attention to the efforts to replace in the IDE files the enumerated nodes by simple nodes, ie. "<cell1>" would be replaced simply by "<cell>". Maybe it's a good idea to follow this strategy. One advantage would be that it is easier to manually add/delete cells in the written file (even although this maybe should be discouraged at all...). On the other hand, having the row number in the node name simplifies debugging the written file considerably - having debugged the non-numbered rows in the ODS spreadsheet file format a lot I know what I am talking about. ... just thoughts....
I manually added a third column to the written file. The valuelisteditor silently reads only two. That's fine. But having a file with more than two columns is an indication that the user maybe did not select the correct filename. Therefore, this is probably a good place to raise an exception.
I also tried the LoadFromCSVFile that you already mentioned. I also added a third column to a file written by SaveToCSVFile. This file is also accepted silently but now it is displayed with 3 columns where the third column is empty. Probably another case for an exception.
Ah, and once a file with three columns has been loaded the VLE always displays three columns, even after clear has been called. I guess that Clear should also reset the number of columns, just for safety.
Adding a simple demo for the load/save methods. It also contains the modified files with three columns. Click the Load buttons first to see the effect of three columns. For further teste delete the test.dat and test.csv files and recreate them correctly by the Save buttons.
valuelisteditor_loadsave.zip (2,395 bytes)
> As for the file format: I would like to draw your attention to the efforts to replace in the IDE files the enumerated nodes by simple nodes...
Well, that should be fixed in Grids.pas then, it's not a TValueListEditor issue.
What annoys me is that SaveContent calls inherited (->TCustomstringGrid) and then does all the work that was done there again.
The only usefull part of inherited SaveContent is that one also calls inherited (->TCustomGrid) which saves everything else but the content.
Ideally TValueListEditor should only call TCustomGrid.SaveContent (something like inherited inherited SaveContent).
What would be the proper solution for that?
> I manually added a third column to the written file. The valuelisteditor silently reads only two. That's fine.
> But having a file with more than two columns is an indication that the user maybe did not select the correct filename.
> Therefore, this is probably a good place to raise an exception.
Yes, I've been missing an "inherited inherited" rather often myself.
I'd do it this way:
- Save wheter soContent is in SaveOptions
- Temporarily remove soContent from SaveOptions
- Call inherited (--> saves only the TCustomGrid stuff because soContent is removed)
- Restore soContent option if needed
- Call the new code if soContent is set
ETA: implemented in r62056.
||What about the loading part? Will it also be able to load the headers and content ?|
Raising an exception inside LoadContent leaves the grid in an unrecoverable visual mess.
Simply exiting LoadContent in that case does not create this mess.
This seems to happen wether or not the grid has focus when doing the LoadFromFile.
It also does not matter if I remove the call to Clean.
Note: up to that point (if Clean is commented out) nothing has happened to the grid at that time.
ETA: Well, this happens to TStringGrid as well. You don't see it immediately but the grid becomes unusable.
Reported as 0036182.
> What about the loading part? Will it also be able to load the headers and content ?
Why don't you try it?
For me it saves headers (titles) and content and reloads them too (if the grid did not have titles, then loading will remove the titles from curent grid and vice versa).
||@sergio and jamie: feedback please, otherwise I can't decide if this can be merged to fixes branch.|
No feedback, so not merged to fixes branch.
|2019-10-12 00:42||Sergio Fernandez||New Issue|
|2019-10-12 09:53||Bart Broersma||Assigned To||=> Bart Broersma|
|2019-10-12 09:53||Bart Broersma||Status||new => confirmed|
|2019-10-12 09:53||Bart Broersma||LazTarget||=> -|
|2019-10-12 09:53||Bart Broersma||Status||confirmed => assigned|
|2019-10-12 09:55||Bart Broersma||Status||assigned => feedback|
|2019-10-12 09:55||Bart Broersma||Note Added: 0118505|
|2019-10-12 10:01||Bart Broersma||Note Edited: 0118505||View Revisions|
|2019-10-12 10:32||wp||Note Added: 0118506|
|2019-10-12 18:20||Bart Broersma||Note Added: 0118520|
|2019-10-12 18:32||Bart Broersma||Status||feedback => assigned|
|2019-10-12 19:33||Bart Broersma||Note Added: 0118523|
|2019-10-12 19:54||Bart Broersma||Note Edited: 0118523||View Revisions|
|2019-10-12 19:54||Bart Broersma||Note Edited: 0118523||View Revisions|
|2019-10-12 20:41||jamie philbrook||Note Added: 0118524|
|2019-10-12 23:06||Bart Broersma||Note Added: 0118532|
|2019-10-12 23:17||Bart Broersma||Note Added: 0118533|
|2019-10-13 13:59||Bart Broersma||Status||assigned => feedback|
|2019-10-13 13:59||Bart Broersma||Note Added: 0118561|
|2019-10-13 14:04||Bart Broersma||Note Added: 0118562|
|2019-10-13 20:17||wp||Note Added: 0118567|
|2019-10-13 20:18||wp||Note Edited: 0118567||View Revisions|
|2019-10-13 20:21||wp||Note Edited: 0118567||View Revisions|
|2019-10-13 20:35||wp||File Added: valuelisteditor_loadsave.zip|
|2019-10-13 20:35||wp||Note Added: 0118568|
|2019-10-13 23:16||Bart Broersma||Note Added: 0118571|
|2019-10-13 23:17||Bart Broersma||Note Added: 0118572|
|2019-10-13 23:29||wp||Note Added: 0118573|
|2019-10-14 17:52||Bart Broersma||Note Added: 0118596|
|2019-10-14 17:54||Bart Broersma||Target Version||=> 2.2|
|2019-10-14 17:54||Bart Broersma||Fixed in Revision||=> r62043, r62044|
|2019-10-14 18:16||jamie philbrook||Note Added: 0118597|
|2019-10-14 18:37||Bart Broersma||Note Added: 0118598|
|2019-10-14 18:38||Bart Broersma||Note Added: 0118599|
|2019-10-14 18:44||Bart Broersma||Note Edited: 0118598||View Revisions|
|2019-10-14 18:59||Bart Broersma||Fixed in Revision||r62043, r62044 => r62043, r62044, r62055|
|2019-10-14 19:10||Bart Broersma||Note Edited: 0118598||View Revisions|
|2019-10-14 19:23||Bart Broersma||Note Edited: 0118596||View Revisions|
|2019-10-17 21:55||Bart Broersma||Note Added: 0118650|
|2019-10-30 11:46||Bart Broersma||Status||feedback => resolved|
|2019-10-30 11:46||Bart Broersma||Resolution||open => fixed|
|2019-10-30 11:46||Bart Broersma||Note Added: 0118927|