View Issue Details

IDProjectCategoryView StatusLast Update
0032463LazarusLCLpublic2018-02-12 17:58
ReporterMichal Gawrycki Assigned Towp  
PrioritynormalSeverityminorReproducibilityhave not tried
Status closedResolutionfixed 
Product Version1.9 (SVN) 
Summary0032463: Regression - TDBLookupComboBox raise exception
DescriptionRevision r55894 causes regression in TDBLookupComboBox. Value of combo text is assigned to data field in "TCustomComboBox.Change" function which is incorrect in case of DBLookupComboBox.
I suggest moving field data assignment to "TDBComboBox.Change".
TagsNo tags attached.
Fixed in Revisionr56884, r56948
LazTarget1.8.2
Widgetset
Attached Files

Relationships

related to 0032383 resolvedJesus Reyes TDbComboBox has problems to set data when item changes 
related to 0030587 closedLuiz Americo TDBComboBox – field data is not updated if combo is not focused but element is changed by mouse wheel 
related to 0032943 closedJesus Reyes TDBLookupComboBox and csDropDown - restore correct ItemIndex 
related to 0033160 closedwp Selected item in TDBComboBox when TDataSet are browse 

Activities

Michal Gawrycki

2017-09-24 13:05

reporter  

dbcombobox-change.patch (1,612 bytes)   
Index: lcl/dbctrls.pp
===================================================================
--- lcl/dbctrls.pp	(revision 55911)
+++ lcl/dbctrls.pp	(working copy)
@@ -707,6 +707,7 @@
 
   TDBComboBox = class(TCustomDBComboBox)
   protected
+    procedure Change; override;
     procedure DataChange(Sender: TObject); override;
     procedure KeyDown(var Key: Word; Shift: TShiftState); override;
     procedure KeyPress(var Key: char); override;
Index: lcl/include/customdbcombobox.inc
===================================================================
--- lcl/include/customdbcombobox.inc	(revision 55911)
+++ lcl/include/customdbcombobox.inc	(working copy)
@@ -34,15 +34,9 @@
 
 procedure TCustomDBComboBox.Change;
 begin
-  FDataLink.Modified;
-  try
-    if FDataLink.CanModify then begin
-      FDataLink.Field.AsString := Text;
-      FDatalink.Modified;
-    end;
-  finally
-    inherited Change;
-  end;
+  if FDataLink.CanModify then
+    FDataLink.Modified;
+  inherited Change;
 end;
 
 function TCustomDBComboBox.GetReadOnly: Boolean;
Index: lcl/include/dbcombobox.inc
===================================================================
--- lcl/include/dbcombobox.inc	(revision 55911)
+++ lcl/include/dbcombobox.inc	(working copy)
@@ -17,6 +17,18 @@
   FDataLink.Field.Text := Text;
 end;
 
+procedure TDBComboBox.Change;
+begin
+  try
+    if FDataLink.CanModify then begin
+      FDataLink.Field.AsString := Text;
+      FDatalink.Modified;
+    end;
+  finally
+    inherited Change;
+  end;
+end;
+
 procedure TDBComboBox.DataChange(Sender: TObject);
 var
   DataLinkField: TField;
dbcombobox-change.patch (1,612 bytes)   

Juha Manninen

2017-09-24 14:26

developer   ~0103014

Ok right. Thanks for the patch. Applied.

wp

2017-12-29 23:51

developer   ~0105124

Last edited: 2017-12-30 10:21

View 4 revisions

This patch should be reverted because it crashes the DBCombobox:

Load the demo attached to issue 0030587, click into the DBCombobox, and turn the mouse wheel --> crash.

The crash happens in line

  FDataLink.Field.AsString := Text;

of the newly inserted Change method, because the dataset is not yet in edit mode.

Correct behavior could be achieved by checking FDatalink.Editing:

procedure TDBComboBox.Change;
begin
  try
    if FDataLink.CanModify and FDataLink.Editing then begin
      FDataLink.Field.AsString := Text;
      FDatalink.Modified;
    end;
  finally
    inherited Change;
  end;
end;

But this gives back the erroneous behavior which was fixed in issue 0030587.

I found good behavior if both r55912 (this issue) and r55894 (issue 0032383) are reverted and TDBCombobox.Select (modified for issue 0030587) is changed to

procedure TDBComboBox.Select;
begin
  //avoid reseting text when calling select
  FDataLink.OnDataChange := nil;
  try
    if FDataLink.Edit then
    begin
       FDataLink.Modified; // <-- added
       FDataLink.UpdateData;
       inherited Select;
    end
    else
    begin
       // if cannot modify, let it reset
       FDatalink.Reset;
       DataChange(Self);
    end;
  finally
    FDataLink.OnDataChange := @DataChange;
  end;
end;

However, I am not sure if this is the correct solution because the IsModified flag should already have been set when the routine is entered. Unfortunately, no demo project has been added to issue 0032383.

Juha Manninen

2017-12-30 10:23

developer   ~0105134

wp, please use your judgement to fix the issue. You know it better.

wp

2017-12-30 21:59

developer   ~0105166

Last edited: 2017-12-30 23:59

View 2 revisions

Michael, I had to undo the patch partly because it crashes the combobox as described above (r56883, r56884).

Please check whether the issue that you reported still exists.

Michal Gawrycki

2018-01-04 00:07

reporter   ~0105303

I can confirm that the reported problem does not occur after your changes.
I found another one about DBLookupComboBox, but I'm not sure if it's caused by your changes so I'll check it and open a new issue if needed.

wp

2018-01-04 09:03

developer   ~0105312

Thank you.

Before resolving this issue I'd ask you to check DBLookupCombobox. In r56948 I use the same code in TDBLookupCombobox.Select as in TDBCombobox.Select. Please check if you still see issues in DBLookupCombobox.

For the future I'd ask you to always add sample projects to your reports which show the error. This little projects are a big help for the reviewer to test the patch and the effect of other related patches.

Michal Gawrycki

2018-01-04 17:40

reporter   ~0105326

My issue concerned that TDBLookupComboBox set value of "Text" property to the data field, which is incorrect in case of lookup contol (because there should be a key value from lookup table nor display text). So in this case it works ok (r56954).
I usually attach an example project. But in this case the matter seemed obvious to me, because if I entered anything that was not on the lookup list in TDBLookupComboBox, I was getting an exception.

wp

2018-01-04 18:09

developer   ~0105330

Thank you for testing DBLookupCombobox.

Please don't assume that anything is obvious. If it is obvious to you it may not be obvious to the reviewer because he maybe is used to work with the control in a different way. And even if it really were "obvious" then the demo would help another reviewer who comes back to this issue because of another issue (like me). With a sample project I know without going into details of the current report: this demo should not give an error any more if that other issue is fixed correctly.

I am resolving this now as being fixed. Your turn to close it. Thank you again.

Michal Gawrycki

2018-01-04 19:33

reporter   ~0105336

Thank you for help. I close this report.

Issue History

Date Modified Username Field Change
2017-09-24 13:05 Michal Gawrycki New Issue
2017-09-24 13:05 Michal Gawrycki File Added: dbcombobox-change.patch
2017-09-24 14:16 Juha Manninen Assigned To => Juha Manninen
2017-09-24 14:16 Juha Manninen Status new => assigned
2017-09-24 14:17 Juha Manninen Relationship added related to 0032383
2017-09-24 14:26 Juha Manninen Fixed in Revision => r55912
2017-09-24 14:26 Juha Manninen LazTarget => -
2017-09-24 14:26 Juha Manninen Note Added: 0103014
2017-09-24 14:26 Juha Manninen Status assigned => resolved
2017-09-24 14:26 Juha Manninen Resolution open => fixed
2017-12-29 23:51 wp Note Added: 0105124
2017-12-29 23:51 wp Status resolved => assigned
2017-12-29 23:51 wp Resolution fixed => reopened
2017-12-29 23:51 wp Relationship added related to 0030587
2017-12-29 23:52 wp Note Edited: 0105124 View Revisions
2017-12-30 00:01 wp Note Edited: 0105124 View Revisions
2017-12-30 10:21 wp Note Edited: 0105124 View Revisions
2017-12-30 10:22 Juha Manninen Assigned To Juha Manninen => wp
2017-12-30 10:23 Juha Manninen Note Added: 0105134
2017-12-30 21:59 wp Note Added: 0105166
2017-12-30 21:59 wp Status assigned => feedback
2017-12-30 23:59 wp Note Edited: 0105166 View Revisions
2018-01-04 00:07 Michal Gawrycki Note Added: 0105303
2018-01-04 00:07 Michal Gawrycki Status feedback => assigned
2018-01-04 09:03 wp Note Added: 0105312
2018-01-04 09:03 wp Status assigned => feedback
2018-01-04 17:40 Michal Gawrycki Note Added: 0105326
2018-01-04 17:40 Michal Gawrycki Status feedback => assigned
2018-01-04 18:09 wp Note Added: 0105330
2018-01-04 18:11 wp Fixed in Revision r55912 => r56884, r56948
2018-01-04 18:11 wp LazTarget - => 1.8.2
2018-01-04 18:11 wp Status assigned => resolved
2018-01-04 18:11 wp Resolution reopened => fixed
2018-01-04 19:33 Michal Gawrycki Note Added: 0105336
2018-01-04 19:33 Michal Gawrycki Status resolved => closed
2018-01-10 18:46 Juha Manninen Relationship added related to 0032943
2018-02-12 17:58 wp Relationship added related to 0033160