View Issue Details

IDProjectCategoryView StatusLast Update
0014832LazarusLCLpublic2009-10-28 17:45
ReporterLuiz Americo Assigned ToBart Broersma  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Platformi386OSWindows 
Target Version1.0.0Fixed in Version0.9.29 (SVN) 
Summary0014832: Mask corruption in DBEdit
DescriptionIn a DBEdit with a mask set to 99/99 the mask is corrupted after the following steps (See the attached demo):

1) The dataset is closed.
  * The Masked DBEdit shows (__/__)
2) The dataset is open and an empty record is added
  * The Masked DBEdit shows (_____)
  * This occurs because TDBEdit.DataChange set EditText with an empty string ("")
3) Set the focus to the Masked DBEdit
  * The Masked DBEdit shows (__/__)
  * This occurs because Clear is called through WMSetFocus -> Reset -> SetText
3) Set the focus to the Normal DBEdit and add some random value. Set focus again to Masked DBEdit
  * The Masked DBEdit shows (_____)
  * Is not possible to add values in the third position (where should be /) but after loosing focus an exception will occur: "The current text does not match the mask"
  * This occurs because WMSetFocus calls Reset only when not Editing

The attached patch fixes the issue. Although i'm not sure if is correct or if there's another bug somewhere else.
TagsNo tags attached.
Fixed in Revision22323
LazTarget1.0
Widgetset
Attached Files

Activities

2009-10-17 21:19

 

DBEditMask.zip (3,100 bytes)

2009-10-17 21:20

 

dbeditmask.diff (631 bytes)   
Index: .
===================================================================
--- .	(revision 22198)
+++ .	(working copy)
@@ -1115,6 +1115,7 @@
 procedure TCustomMaskEdit.SetEditText(const AValue: string);
 var
   S: String;
+  i: Integer;
 begin
   if (not IsMasked) then
   begin
@@ -1124,7 +1125,8 @@
   begin
     //Make sure we don't copy more or less text into the control than FMask allows for
     S := Copy(AValue, 1, Length(FMask));
-    while Length(S) < Length(FMask) do S := S + FSpaceChar;
+    for i := Length(S) + 1 to Length(FMask) do
+       S := S + ClearChar(i);
     SetInheritedText(S);
   end;
 end;
dbeditmask.diff (631 bytes)   

Vincent Snijders

2009-10-20 21:22

manager   ~0031541

Bart, can you look at this issue?

Bart Broersma

2009-10-21 14:49

developer   ~0031552

@Vincent: I really do not have any experience with database programming.


However, I can speculate the following:

procedure TDBEdit.DataChange(Sender: TObject);
begin
  if FDataLink.Field <> nil then begin
    //use the right EditMask if any
    //EditMask := FDataLink.Field.EditMask; doesn't exist yet

    //if we are focused its possible to edit,
    //if the field is currently modifiable
    if Focused and FDataLink.CanModify then begin
      //display the real text since we can modify it
      //Text := FDataLink.Field.DisplayText//this is wrong, but Text seems Broken <----- +++
      Text := FDatalink.Field.Text;
      SelectAll;
    end else
      //otherwise display the pretified/formated text since we can't
      EditText := FDataLink.Field.DisplayText; <<---------- ***
    MaxLength := FDatalink.Field.Size;
  end
  else begin
    //todo: uncomment this when TField implements EditMask
    //EditMask := ''
    Text := '';
  end;
end;


*** It is, by nature, absolutely unsafe to use EditText, beacuse it will overwrite all MaskLiterals (like '/' in the example) in the control, and the user will not be able to correct this. This will result in an EDBEditError when the control looses focus. (This is Delphi compatible, so I decided not to change this in maskedit.pp).

I do not know why it was decided to set EditText instead of Text here.
If it is absolutely necessary to use EditText then this cannot really be prevented. There is no certain way to set EditText in such a way that Validating will always be succesfull.
It is however possible to protect against overwriting of MaskLiterals:
- First clear the control
- Then loop FDataLink.Field.DisplayText and
  set EditText[i] := FDataLink.Field.DisplayText[i] if
  EditText[i] = SpaceChar

I tested this workaround and it does prevent the exception in the example, however I do not know if it has unwanted side-effects.


+++ What is broken with Text and is that FDatalink.Field.Text or DBEdit.Text?

What is the difference between FDataLink.Field.DisplayText and FDataLink.Field.Text? (In other words what should be displayed in the control here in step 3 of the example?)

As far as I understand this section of code, EditText is set if the user cannot modify the data linked to the field.
Does this mean that the user can not alter data, but can alter text in the control?
In that case, we really need to have a FDataLink.Field.EditMask so we can temporarily disable the EditMask of the control and re-apply if necessary.


(B.t.w. The proposed patch in dbeditmask.diff is wrong, since it will in fact always clear the control, no matter what value you supply for EditText.)

Bart Broersma

2009-10-21 18:10

developer   ~0031554

Hm, I can slightly change maskedit.pp so that it will pad EditText with ClearChar() instead of FSpaceChar when the text is too short.
Nevertheless it is unwise to set EditText in the first place.

Luiz Americo

2009-10-21 20:24

developer   ~0031560

Last edited: 2009-10-21 20:25

>Hm, I can slightly change maskedit.pp so that it will pad EditText with >ClearChar() instead of FSpaceChar when the text is too short.

This is exactly what i did. See the patch. This fixes the bug for me.

Luiz Americo

2009-10-21 20:49

developer   ~0031561

> (In other words what should be displayed in the control here in step 3 of the example?)

__/__ . It's correct. Because Clear uses ClearChar and not FSpaceChar like EditText

> The proposed patch in dbeditmask.diff is wrong, since it will in fact always clear the control, no matter what value you supply for EditText

The patch only pads the difference between the text and the mask. Did i miss something?

Bart Broersma

2009-10-22 15:05

developer   ~0031583

Last edited: 2009-10-22 15:18

@Luiz: You're right about the working of your patch, sorry, my mistake.

However, in this particular case FDataLink.Field.DisplayText = '', but this probably is not the case in general?
Stepping through the code I guess it can contain the name of the field or a classname?

The real problem lies in the use of setting DBEdit.EditText, which is unsafe by nature. (IMHO the Borland guys should have made this a read-only property)

I patched dbedit.inc to leave MaksLiterals in place in the EditText in this case, which prevents leaving the control in a state where it can never be validated OK. Only patching maskedit.pp in the proposed way (padding with ClearChar) is not enough, any MaskLiteral before the padding starts will already be overwritten).

This will however not display FDataLink.Field.DisplayText in the control, but a somewhat mutilated version of it.
Hence my question: what is in general (or can be) the value of FDataLink.Field.DisplayText?
If that text needs to be shown in the control unaltered, this can only be achieved by disabling the editmask and this will require the implementation if TField.EditMask I think.

Luiz Americo

2009-10-22 19:00

developer   ~0031587

> However, in this particular case FDataLink.Field.DisplayText = '', but this probably is not the case in general?

It's pretty common have a null value. Moreover all fields are initialized with a null value.

> Stepping through the code I guess it can contain the name of the field or a classname?

No it contains the raw value of the field or a formatted (by TField) text

> I patched dbedit.inc to leave MaksLiterals in place in the EditText in this case, which prevents leaving the control in a state where it can never be validated OK. Only patching maskedit.pp in the proposed way (padding with ClearChar) is not enough, any MaskLiteral before the padding starts will already be overwritten).

OK

> This will however not display FDataLink.Field.DisplayText in the control, but a somewhat mutilated version of it.
Hence my question: what is in general (or can be) the value of FDataLink.Field.DisplayText?

It's up to the programmer set the correct Mask so it matches with the field value. An example is using with date fields:
ShortDateFormat := dd/mm/yyyy
Mask must be set to 99/99/9999;1;_
The fields values will be in the pattern 22/10/2009 so it's supposed to always matches the mask, so in this case the text will not be mutilated. (The correct field date handling through DBEdit is not so straight, but it's out of scope now)

Bart Broersma

2009-10-23 14:54

developer   ~0031621

>It's up to the programmer set the correct Mask so it matches with the field
> value. An example is using with date fields:
In that case, the FDataLink.Field.DisplayText shold be compatible with the editmask, and there should be no need to use
  EditText := FDataLink.Field.DisplayText
but a plain and simple
  Text := FDataLink.Field.DisplayText
should do also in that case (with the exception of substituting '/' with DateSeparator and ':' with HourSeparator.

I could easily rewrite TMaskEdit.SetEditText so that it is impossible to f* up the control, but that is in no way Delphi compatible. (The current implementation is not compatible also)

Personally I would prefer this option (for the time being), since it ensures that the control is never left in an unrecoverable state.
(Maybe I should leave a debugln statement in SetEditText stating that the programmer must be insane to even consider using it?)

I still think that ultimately we need to implement TField.EditMask to control the mask in a DBEdit.

2009-10-25 12:22

 

maskedit.pp.diff (2,466 bytes)   
Index: maskedit.pp
===================================================================
--- maskedit.pp	(revision 22249)
+++ maskedit.pp	(working copy)
@@ -74,6 +74,13 @@
    I decided to implement TCustomEdit.SetText in such a way that at least storing and retrieving the
    Text property results in the same text in the control.
    So SetText breaks Delphi compatibility (a little), but data integrity is preserved as the result of this.
+
+ - SetEditText is not Delphi compatible. Delphi allows setting any text in the control, leaving the control
+   in an unrecoverable state, where it is impossible to leave the control because the text can never be validated
+   (too short, too long, overwritten maskliterals). The app wil crash as a result of this.
+   I have decided to disallow this:
+   - EditText is truncated, or padded with ClearChar if necessary so that Length(EditText) = Length(FMask)
+   - Restore all MaskLiterals in the text
 }
 
 unit MaskEdit;
@@ -1252,8 +1259,11 @@
 
 
 procedure TCustomMaskEdit.SetEditText(const AValue: string);
+//Note: This is not Delphi compatible, but by design
+//Delphi lets you just set EditText of any length, which is extremely dangerous!
 var
   S: String;
+  i: Integer;
 begin
   if (not IsMasked) then
   begin
@@ -1263,7 +1273,12 @@
   begin
     //Make sure we don't copy more or less text into the control than FMask allows for
     S := Copy(UTF8ToAscii(AValue), 1, Length(FMask));
-    while Length(S) < Length(FMask) do S := S + FSpaceChar;
+    //Restore all MaskLiterals, or we will potentially leave the control
+    //in an unrecoverable state, eventually crashing the app
+    for i := 1 to Length(S) do
+      if IsLiteral(FMask[i]) then S[i] := ClearChar(i);
+    //Pad resulting string with ClearChar if text is too short
+    while Length(S) < Length(FMask) do S := S + ClearChar(Length(S)+1);
     SetInheritedText(S);
   end;
 end;
@@ -1724,10 +1739,12 @@
     FMaskSave := _MaskSave;
     if not TextIsValid(S) then
     begin
-      SetFocus;
+      //SetFocus does not always work, and if it works it will cause a double DoExit
+      //(because the default exception handler will steal the focus from us)
+      //and re-raise the exception, before it has been handled
+      //SetFocus;
       SetCursorPos;
       Raise EDBEditError.Create(SMaskEditNoMatch);
-      //DebugLn('TCustomMaskEdit.Validate: The current text does not match the the specified mask');
     end;
   end;
 end;
maskedit.pp.diff (2,466 bytes)   

Bart Broersma

2009-10-25 12:25

developer   ~0031654

maskedit.pp.diff is my proposed fix for this issue.
(It also fixes a problem with double DoExit if validation fails, see comments in sourcecode.)

Luiz, Vincent, please take a look and see if you can agree on this solution.

Luiz Americo

2009-10-26 04:14

developer   ~0031671

Last edited: 2009-10-26 04:44

I'm fine with this solution.

EDIT: It fixes this issue but does not allow to do what Delphi does.

In Delphi if EditMask of a Date field is !99/99/00;1;_ while the control is not focused/being edited it shows the date matching the ShortDateFormat. If ShortDateFormat if d/m/yyyy, 12/09/2003 will be show as 12/9/2003. Setting the focus to the control, makes the control shows text matching the EditMask, in the example will be 12/_9/03. Notice that the Text format differs according to the situation.

BTW: It seems that TField.EditMask does not do any special handling. It just to share the mask between more than one control, so using it won't help much. Anyway TDBEdit/TMaskEdit is acting almost closely on how Delphi does. I will report the missing features soon.

Bart Broersma

2009-10-26 12:32

developer   ~0031683

This cannot be done without temporarily disabling the current DBEdit.EditMask (or disabling the validation, or make the control not editable, since then FTextOnEnter will not change in that case and no validating will occur).

If we allow TMaskEdit.SetEditText to allow any text (as Delphi does) and we allow the user to type in the control, then the control will get into a state where it cannot be recovered, and the app will crash.

As I stated before, I have absolutely no experience in database programming, and I have no insight in the data-aware controls and how they function. So I do not really understand why we want to have a text in the control that does not match the current EditMask.

> while the control is not focused/being edited

If we can reliably detect this, it is possible to temporarily disable the editmask (or the validation), but I think this then should be implemented in DBEdit, not in TCustomMaskEdit.

> It seems that TField.EditMask does not do any special handling. It just to
> share the mask between more than one control, so using it won't help much.

If we have a TField.EditMask _AND_ we can detect wether or not the DBEdit (datasource?) should be editable, we can in fact on every datachange decide if the control must be masked or not. If so, use TField.EditMask, if not use no mask and any text can be displayed in the control.

> Anyway TDBEdit/TMaskEdit is acting almost closely on how Delphi does.
> I will report the missing features soon.

Please see the comments in maskedit.pp where I explained why some things are implemented different then in Delphi, not by accident, but by design.

2009-10-26 13:04

 

DBEditMaskDemo.zip (2,940 bytes)

Luiz Americo

2009-10-26 13:51

developer   ~0031684

Last edited: 2009-10-26 14:00

> If we allow TMaskEdit.SetEditText to allow any text (as Delphi does) and we allow the user to type in the control, then the control will get into a state where it cannot be recovered, and the app will crash.

Until doing further testing with DBEdit/Delphi i also did not understand why it allowed EditText to be set to any value. Now i see why they did so. It seems that this was introduced specifically to make the implementation of the DBEdit features possible (switch the visible text according what's the control state - editing or not).

This is how it works:

You have the actual data (12/9/2003) that is logically compatible with the mask (!99/99/00;1;_). But this data is not directly compatible with the mask, so when the user focus the controls, i.e., starts the editing, the actual data is converted to a format directly compatible with the mask (12/_9/03) (this conversion is missing in Lazarus - see bug 14895). When the user finishes the editing (looses focus), the actual data (12/9/2003) is again displayed in the control.
Notice that all data that is displayed be editing or not is compatible with the mask.

See the attached example and compare to the Delphi test program i sent to you privately. Both programs use exactly the same code and controls.

> As I stated before, I have absolutely no experience in database programming, and I have no insight in the data-aware controls and how they function. So I do not really understand why we want to have a text in the control that does not match the current EditMask.

See the Delphi demo and you will understand

> If we can reliably detect this, it is possible to temporarily disable the editmask (or the validation), but I think this then should be implemented in DBEdit, not in TCustomMaskEdit.

Correct. It should be implemented in DBEdit

> If we have a TField.EditMask _AND_ we can detect wether or not the DBEdit (datasource?) should be editable, we can in fact on every datachange decide if the control must be masked or not. If so, use TField.EditMask, if not use no mask and any text can be displayed in the control.

Delphi does not has TDBEdit.EditMask property, it uses TField.EditMask instead and they have exactly the same meaning. My point here is that the EditMask check you proposed could be done with the TDBEdit.EditMask property. It won't make any difference since it's just in a different location but the value and meaning are the same.

BTW: In the demo you can see the actual value of having EditMask as a property of TField. Notice that, in Delphi, the embedded editor of the DBGrid uses the same mask as the DBEdit. In Lazarus the DBGrid does not use any mask.

> Please see the comments in maskedit.pp where I explained why some things are implemented different then in Delphi, not by accident, but by design.

I understood that and also see that setting the EditMask directly is dangerous (this bug is an example). It seems that they allowed that mostly to implement TDBEdit in the lack of a better design.

Bart Broersma

2009-10-26 16:45

developer   ~0031688

> You have the actual data (12/9/2003) that is logically compatible with the
> mask (!99/99/00;1;_). But this data is not directly compatible with the mask,
> so when the user focus the controls, i.e., starts the editing, the actual
> data is converted to a format directly compatible with the mask (12/_9/03)
> (this conversion is missing in Lazarus - see bug 14895).

If we implement SetText this way (see my comments in 0014895), then there is no need for setting the EditText property here.

> When the user
> finishes the editing (looses focus), the actual data (12/9/2003) is again
> displayed in the control.
> Notice that all data that is displayed be editing or not is compatible with
> the mask.
?
"12/9/2003" as the actual text in the control is not compatible with the mask, so IMHO it should never be allowed to be there.
And "12/9/2009" is not the "actual data". If the user types "12/_9/2009", then this is in effect the actual data (as in the value of DBEdit.Text), the space is not removed by GetText.

> I understood that and also see that setting the EditMask
(I guess you mean EditText)
> directly is dangerous (this bug is an example). It seems that they
> allowed that mostly to implement TDBEdit in the lack of a better design.

Maybe we should come up with a better design in TDBEdit then?

I also do not really understand in the first place why there has to be a different text in the control, depending on wether the user is editing or not.
I still think that this can best be achieved by disabling/restoring the EditMask as needed and setting the Text property, not the EditText property, and that this should be done in DBEdit.

Luiz Americo

2009-10-26 18:01

developer   ~0031690

> "12/9/2003" as the actual text in the control is not compatible with the mask, so IMHO it should never be allowed to be there.

12/9/2003 is compatible with the Mask in the Delphi rules, otherwise the control would raise an exception when setting the Text property with it.

What Delphi does with "12/9/2003" in a !99/99/00;1;_ mask is the following:
12 matches directly 99 - It's OK.
9 matches 99 (9 means that you can have a numeric character or not), but to allow the user to add the allowed another numeric character it returns: _9
2003 matches 00 (00 means at least two numeric characters, but this does not means you can not pass more characters), but has more characters than the mask so it returns, the last two: 03.

To make it clearer in Delphi (use the program i sent) set the Mask to !99 and set text to 2003: you will get 03. If you use 99 as the mask you will get 20


> And "12/9/2009" is not the "actual data"

Yes it is. See in the example i sent to you. This is what Field.Text or DisplayText returns

> If the user types "12/_9/2009"

It's "12/_9/09".

>, then this is in effect the actual data (as in the value of DBEdit.Text), the space is not removed by GetText.

You are right the returned value is 12/ 9/09 that is a valid date to Delphi

> Maybe we should come up with a better design in TDBEdit then?
>I also do not really understand in the first place why there has to be a different text in the control, depending on wether the user is editing or not.
I still think that this can best be achieved by disabling/restoring the EditMask as needed and setting the Text property, not the EditText property, and that this should be done in DBEdit.

This could do the job (setting the Text with a not directly mask compatible).

Make IsMasked a protected property. DBEdit will store the old state and restore after setting the Text

Bart Broersma

2009-10-26 18:24

developer   ~0031692

Luiz: we are now discussing two different problems.
1. TCustomMaskEdit.SetText does not do what you expect.
2. Irrecoverable state of DBEdit control by means of setting EditText

I would propose to continu discussing issue 1 in 0014895.

Issue 2 should really be solved in the code for DBEdit and not in MaskEdit (provided of course that issue 1 is resolved).

This is what I would suggest for now:
- apply maskedit.pp.diff (resolves issue 2)
- leave this bugreport open and assigned to me
- fix issue 1 (0014895) (I'ld appreciate your help on this)
- after that come back to this issue and see what needs to be done

Is that OK with you?

Luiz Americo

2009-10-26 19:36

developer   ~0031694

> Is that OK with you?

Yes

Bart Broersma

2009-10-27 14:55

developer   ~0031734

@Vincet: can you apply my patch?

Vincent Snijders

2009-10-28 17:01

manager   ~0031767

Both thanks for the fix and the patch. I hope I got the commit message correct.

Issue History

Date Modified Username Field Change
2009-10-17 21:19 Luiz Americo New Issue
2009-10-17 21:19 Luiz Americo File Added: DBEditMask.zip
2009-10-17 21:20 Luiz Americo File Added: dbeditmask.diff
2009-10-20 21:22 Vincent Snijders LazTarget => 1.0
2009-10-20 21:22 Vincent Snijders Note Added: 0031541
2009-10-20 21:22 Vincent Snijders Assigned To => Bart Broersma
2009-10-20 21:22 Vincent Snijders Status new => assigned
2009-10-20 21:22 Vincent Snijders Target Version => 1.0.0
2009-10-21 14:49 Bart Broersma Note Added: 0031552
2009-10-21 18:10 Bart Broersma Note Added: 0031554
2009-10-21 20:24 Luiz Americo Note Added: 0031560
2009-10-21 20:25 Luiz Americo Note Edited: 0031560
2009-10-21 20:49 Luiz Americo Note Added: 0031561
2009-10-22 15:05 Bart Broersma Note Added: 0031583
2009-10-22 15:18 Bart Broersma Note Edited: 0031583
2009-10-22 19:00 Luiz Americo Note Added: 0031587
2009-10-23 14:54 Bart Broersma Note Added: 0031621
2009-10-25 12:22 Bart Broersma File Added: maskedit.pp.diff
2009-10-25 12:25 Bart Broersma Note Added: 0031654
2009-10-26 04:14 Luiz Americo Note Added: 0031671
2009-10-26 04:44 Luiz Americo Note Edited: 0031671
2009-10-26 12:32 Bart Broersma Note Added: 0031683
2009-10-26 13:04 Luiz Americo File Added: DBEditMaskDemo.zip
2009-10-26 13:51 Luiz Americo Note Added: 0031684
2009-10-26 14:00 Luiz Americo Note Edited: 0031684
2009-10-26 16:45 Bart Broersma Note Added: 0031688
2009-10-26 18:01 Luiz Americo Note Added: 0031690
2009-10-26 18:24 Bart Broersma Note Added: 0031692
2009-10-26 19:36 Luiz Americo Note Added: 0031694
2009-10-27 14:55 Bart Broersma Note Added: 0031734
2009-10-28 17:01 Vincent Snijders Fixed in Revision => 22323
2009-10-28 17:01 Vincent Snijders Status assigned => resolved
2009-10-28 17:01 Vincent Snijders Fixed in Version => 0.9.29 (SVN)
2009-10-28 17:01 Vincent Snijders Resolution open => fixed
2009-10-28 17:01 Vincent Snijders Note Added: 0031767
2009-10-28 17:45 Luiz Americo Status resolved => closed