View Issue Details

IDProjectCategoryView StatusLast Update
0036166LazarusLCLpublic2019-10-30 11:46
ReporterSergio FernandezAssigned ToBart Broersma 
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version2.0.4Product Build 
Target Version2.2Fixed in Version 
Summary0036166: SaveToFile in TValueListEditor not saving data
Descriptionwhen I use SaveToFile in TValueListEditor, only the title captions save to the xml file, none of the data fields actually save.
Steps To ReproduceAdd a TValueListEditor to your form.
Add some data using InsertRow
then Call the SaveTofile procedure.
TagsNo tags attached.
Fixed in Revisionr62043, r62044, r62055
LazTarget-
Widgetset
Attached Files

Activities

Bart Broersma

2019-10-12 09:55

developer   ~0118505

Last edited: 2019-10-12 10:01

View 2 revisions

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.

wp

2019-10-12 10:32

developer   ~0118506

There is no SaveToFile method in Delphi's TValueListEditor.

Bart Broersma

2019-10-12 18:20

developer   ~0118520

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

Bart Broersma

2019-10-12 19:33

developer   ~0118523

Last edited: 2019-10-12 19:54

View 3 revisions

Hmm: LoadFromCSV will silently add extra columns (if it finds more than 1), which is definitely unwanted.

jamie philbrook

2019-10-12 20:41

reporter   ~0118524

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.

Bart Broersma

2019-10-12 23:06

developer   ~0118532

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.

Bart Broersma

2019-10-12 23:17

developer   ~0118533

> 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?
And ItemProps.

Bart Broersma

2019-10-13 13:59

developer   ~0118561

Please try with r62043.

Bart Broersma

2019-10-13 14:04

developer   ~0118562

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.

wp

2019-10-13 20:17

developer   ~0118567

Last edited: 2019-10-13 20:21

View 3 revisions

Looks good!

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.

wp

2019-10-13 20:35

developer   ~0118568

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)

Bart Broersma

2019-10-13 23:16

developer   ~0118571

> 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?

Bart Broersma

2019-10-13 23:17

developer   ~0118572

> 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, agreed.

wp

2019-10-13 23:29

developer   ~0118573

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

Bart Broersma

2019-10-14 17:52

developer   ~0118596

Last edited: 2019-10-14 19:23

View 2 revisions

Nice suggestion.

ETA: implemented in r62056.

jamie philbrook

2019-10-14 18:16

reporter   ~0118597

What about the loading part? Will it also be able to load the headers and content ?

Bart Broersma

2019-10-14 18:37

developer   ~0118598

Last edited: 2019-10-14 19:10

View 3 revisions

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.

Bart Broersma

2019-10-14 18:38

developer   ~0118599

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

Bart Broersma

2019-10-17 21:55

developer   ~0118650

@sergio and jamie: feedback please, otherwise I can't decide if this can be merged to fixes branch.

Bart Broersma

2019-10-30 11:46

developer   ~0118927

No feedback, so not merged to fixes branch.
Please close.

Issue History

Date Modified Username Field Change
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