View Issue Details

IDProjectCategoryView StatusLast Update
0037313LazarusLCLpublic2020-08-04 06:44
ReporterJoeny Ang Assigned ToBart Broersma  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
Product Version2.1 (SVN) 
Fixed in Version2.1 (SVN) 
Summary0037313: [Patch] TCustomEdit and CharCase
DescriptionWhen CharCase is either ecUpperCase or ecLowerCase:
- Assigning a value to Text will trigger OnChange() twice
- Inside OnChange(), assigning a value to Text will infinitely trigger OnChange()

Patch:
- changed FTextChangedByRealSetText and FTextChangedLock to integer, incrementing/decrementing them to preserve value, because OnChange() can be recursively triggered
- discards all OnChange() calls inside TCustomEdit.RealSetText() and trigger CM_TEXTCHANGED ourself
- added upper/lower case check in TCustomEdit.RealSetText() to prevent infinite OnChange() calls

Tested OK with the test project of accorp from issue 0032630.

This might be related to issue 0032602.
TagsNo tags attached.
Fixed in Revisionr63542, r63575
LazTarget2.2
Widgetset
Attached Files

Relationships

related to 0032602 confirmed TEdit.OnChange event handler is calling twice for same text 

Activities

Joeny Ang

2020-07-08 09:15

reporter  

tcustomedit-charcase-v1.patch (2,689 bytes)   
--- lcl/stdctrls.pp
+++ lcl/stdctrls.pp
@@ -756,8 +756,8 @@
     FOnChange: TNotifyEvent;
     FSelLength: integer;
     FSelStart: integer;
-    FTextChangedByRealSetText: Boolean;
-    FTextChangedLock: Boolean;
+    FTextChangedByRealSetText: Integer;
+    FTextChangedLock: Integer;
     FTextHint: TTranslateString;
     procedure ShowEmulatedTextHint(const ForceShow: Boolean = False);
     procedure HideEmulatedTextHint;
--- lcl/include/customedit.inc
+++ lcl/include/customedit.inc
@@ -81,8 +81,8 @@
   BorderStyle := bsSingle;
   FAutoSelect := True;
   FAutoSelected := False;
-  FTextChangedByRealSetText := False;
-  FTextChangedLock := False;
+  FTextChangedByRealSetText := 0;
+  FTextChangedLock := 0;
   AutoSize := True;
   // Accessibility
   AccessibleRole := larTextEditorSingleline;
@@ -560,14 +560,35 @@
 end;
 
 procedure TCustomEdit.RealSetText(const AValue: TCaption);
+var
+  FTemp: TCaption;
 begin
   if (FEmulatedTextHintStatus=thsShowing) and (AValue<>'') then
     HideEmulatedTextHint;
 
-  FTextChangedByRealSetText := True;
+  // need to do this to prevent infinite recursive OnChange() calls
+  FTemp := AValue;
+  case FCharCase of
+    ecUpperCase: FTemp := UTF8UpperCase(AValue);
+    ecLowerCase: FTemp := UTF8LowerCase(AValue);
+  end;
+  if RealGetText = FTemp then
+    Exit;
+
+  Inc(FTextChangedByRealSetText);
   Modified := False;
-  inherited RealSetText(AValue);
-  FTextChangedByRealSetText := False;
+
+  // discard in-between TextChange() calls
+  Inc(FTextChangedLock);
+  try
+    inherited RealSetText(FTemp);
+  finally
+    Dec(FTextChangedLock);
+    if FTextChangedLock = 0 then
+      // trigger TextChange() here
+      Perform(CM_TEXTCHANGED, 0, 0);
+    Dec(FTextChangedByRealSetText);
+  end;
 
   if (FEmulatedTextHintStatus=thsHidden) and CanShowEmulatedTextHint then
     ShowEmulatedTextHint;
@@ -608,7 +629,7 @@
   SStart, SLen: Integer;
 begin
   //debugln('TCustomEdit.TextChanged ',DbgSName(Self));
-  if FTextChangedLock then
+  if FTextChangedLock > 0 then
     Exit;
   if FCharCase in [ecUppercase, ecLowercase] then
   begin
@@ -624,11 +645,11 @@
       CPos := CaretPos;
       SStart := SelStart;
       SLen := SelLength;
-      FTextChangedLock := True;
+      Inc(FTextChangedLock);
       try
         Text := Temp;
       finally
-        FTextChangedLock := False;
+        Dec(FTextChangedLock);
       end;
       SelStart  := SStart;
       SelLength := SLen;
@@ -639,7 +660,7 @@
   begin
     if ([csLoading,csDestroying]*ComponentState=[]) then
     begin
-      if not FTextChangedByRealSetText then
+      if FTextChangedByRealSetText = 0 then
         Modified := True;
       Change;
     end;

tcustomedit-charcase-v1.patch (2,689 bytes)   

Bart Broersma

2020-07-08 10:58

developer   ~0123814

Last edited: 2020-07-08 11:26

View 3 revisions

Questions:

+ // need to do this to prevent infinite recursive OnChange() calls
+ FTemp := AValue;
+ case FCharCase of
+ ecUpperCase: FTemp := UTF8UpperCase(AValue);
+ ecLowerCase: FTemp := UTF8LowerCase(AValue);
+ end;
+ if RealGetText = FTemp then
+ Exit;

You are aware that in a masked TCustomMaskEdit RealGetText is not the same as the Text you see in the control?
If your comment is correct, this will trigger an infinite loop in TCustomMaskEdit.
Did you test this?

Why Perform(CM_TEXTCHANGED, 0, 0) instead of simply Change?

I'm not so sure that calling this directly from within RealSetText is the correct way to handle this.

Joeny Ang

2020-07-08 11:53

reporter   ~0123817

Hi, I tested this on TCustomEdit and TCustomMemo; and accorp's test project checks out.

Regarding the CM_TEXTCHANGED... just needed to find a common place where text is actually changed, thus RealSetText() :)

Just tested TCustomMaskEdit... it did not trigger an infinite loop but a lot of things were amiss. I'll see what I can do.

Bart Broersma

2020-07-08 15:26

developer   ~0123822

Well, any solution to this (IMO minor) problem should definitely not screw upt TCustomMaskEdit.
Notice that this control already issues 2 OnChanges for every change (see related issue for an explanation).
If the past has shown one thing, it is that messing with this can potentially open a whole new can of worms ;-)

Bart Broersma

2020-07-08 15:37

developer   ~0123823

Notice that in Delphi 7, the following scenario will trigger an OnChange:
 CharCase = ecUpperCase
 Text = 'CAMELCASE'
Now do Text := 'CamelCase': Text in the control does not change (of course), but OnChange is fired.

If you simply UpperCase/LowerCase the text and then call RealSetText the behaviour in Lazarus is the same.

Joeny Ang

2020-07-08 15:55

reporter   ~0123825

Hi, thanks for pointing out regarding your camelcase test. I tried removing the snippet you mentioned above (the one to prevent infinite OnChange calls) and the behavior is now similar to Delphi 7.

That code was added for cases when CharCase is either ecUpperCase or ecLowerCase, and you assign Text inside OnChange(). Tried doing this on Delphi 3 and OnChange() is called more that 20 times, but it stops. Unlike in Lazarus, it continues until heap is exhausted.

Could you test the patch without the above snippet? I will try to find another way to prevent the infinite OnChange calls and test/patch it to work with TCustomMaskEdit.

Thanks :)

Joeny Ang

2020-07-08 16:15

reporter   ~0123827

Last edited: 2020-07-08 16:18

View 2 revisions

Initial test on TCustomMaskEdit with this patch (minus the infinite calls code)...
- if EditMask is not set, it behaves like a TCustomEdit
- with an EditMask:
     - OnChange() is now triggered only once (via Text assignment or keyboard input)
     - select all, then paste will trigger OnChange() twice, but if empty, paste will only trigger it once (will check on this)
     - Modified is always False when Text is assigned, True when text is either pasted from clipboard or input via keyboard

Bart Broersma

2020-07-08 16:30

developer   ~0123830

@Joeny
> - with an EditMask:
 > - OnChange() is now triggered only once (via Text assignment or keyboard input)

Changes are then that this will break Ondrej's case (which I broke with my attempt to fix the related issue).

Also it feels a bit "off" that we would need 2 new counters to fix this issue.

Bart Broersma

2020-07-09 15:06

developer   ~0123840

Patch in edit.charcase.diff seems to resolve the double OnChange, but I only tested this on Windows.
edit.charcase.diff (1,325 bytes)   
Index: lcl/include/customedit.inc
===================================================================
--- lcl/include/customedit.inc	(revision 63518)
+++ lcl/include/customedit.inc	(working copy)
@@ -560,6 +560,8 @@
 end;
 
 procedure TCustomEdit.RealSetText(const AValue: TCaption);
+var
+  Temp: TCaption;
 begin
   if (FEmulatedTextHintStatus=thsShowing) and (AValue<>'') then
     HideEmulatedTextHint;
@@ -566,7 +568,17 @@
 
   FTextChangedByRealSetText := True;
   Modified := False;
-  inherited RealSetText(AValue);
+  if (CharCase in [ecUpperCase, ecLowerCase]) then
+  begin
+    if (CharCase = ecUpperCase) then
+      Temp := Utf8UpperCase(AValue)
+    else
+      Temp := Utf8LowerCase(AValue);
+    if (Temp <> RealGetText) then
+      inherited RealSetText(Temp);
+  end
+  else
+    inherited RealSetText(AValue);
   FTextChangedByRealSetText := False;
 
   if (FEmulatedTextHintStatus=thsHidden) and CanShowEmulatedTextHint then
@@ -607,7 +619,7 @@
   CPos: TPoint;
   SStart, SLen: Integer;
 begin
-  //debugln('TCustomEdit.TextChanged ',DbgSName(Self));
+  //debugln(['TCustomEdit.TextChanged ',DbgSName(Self),', FTextChangedLock=',FTextChangedLock,', FTextChangedByRealSetText=',FTextChangedByRealSetText]);
   if FTextChangedLock then
     Exit;
   if FCharCase in [ecUppercase, ecLowercase] then
edit.charcase.diff (1,325 bytes)   

Bart Broersma

2020-07-09 18:35

developer   ~0123849

Well, both our patches potentially didn't restore the EemulatedTextHint, mine set Modified To False always for the case where Temp=RealGetText (which it should not do if Modified was True before).
New patch exits RealSetText as soon as possible (before doing anyhting else).
edit.charcase.2.diff (1,490 bytes)   
Index: lcl/include/customedit.inc
===================================================================
--- lcl/include/customedit.inc	(revision 63518)
+++ lcl/include/customedit.inc	(working copy)
@@ -560,13 +560,27 @@
 end;
 
 procedure TCustomEdit.RealSetText(const AValue: TCaption);
+var
+  Temp: TCaption;
 begin
+  case
+    CharCase of
+      ecNormal: Temp := AValue; //if Text=AValue SetText won't call RealSetText, so no need to check
+      ecUpperCase: begin
+                     Temp := Utf8UpperCase(AValue);
+                     if (Temp = RealGetText) then Exit;
+                   end;
+      ecLowerCase: begin
+                     Temp := Utf8LowerCase(AValue);
+                     if (Temp = RealGetText) then Exit;
+                   end;
+    end;
   if (FEmulatedTextHintStatus=thsShowing) and (AValue<>'') then
     HideEmulatedTextHint;
 
   FTextChangedByRealSetText := True;
   Modified := False;
-  inherited RealSetText(AValue);
+  inherited RealSetText(Temp);
   FTextChangedByRealSetText := False;
 
   if (FEmulatedTextHintStatus=thsHidden) and CanShowEmulatedTextHint then
@@ -607,7 +621,7 @@
   CPos: TPoint;
   SStart, SLen: Integer;
 begin
-  //debugln('TCustomEdit.TextChanged ',DbgSName(Self));
+  //debugln(['TCustomEdit.TextChanged ',DbgSName(Self),', FTextChangedLock=',FTextChangedLock,', FTextChangedByRealSetText=',FTextChangedByRealSetText]);
   if FTextChangedLock then
     Exit;
   if FCharCase in [ecUppercase, ecLowercase] then
edit.charcase.2.diff (1,490 bytes)   

Bart Broersma

2020-07-09 21:54

developer   ~0123852

@Joeny Ang: can tyou test with edit.charcase.2.diff please?
Changing Text inside OnChange still produces 1 OnChange too many, but it does this also when CharCase=ecNormal.

Joeny Ang

2020-07-10 03:36

reporter   ~0123860

Hi Bart,

Just tested edit.charcase.2.diff...

For TCustomEdit...
+ input via keyboard/clipboard paste/setting Text/SelText does not trigger 2 OnChange()s
- setting Text inside OnChange() triggers an additional OnChange(), except when SelText was changed instead of Text.

For TCustomMaskEdit (with EditMask set)...
+ setting Text is OK
- setting SelText will always trigger OnChange() even if value was not changed
- input via keyboard triggers additional OnChange()
- setting Text inside OnChange() triggers an additional OnChange(), except when SelText was changed instead of Text.

Bart Broersma

2020-07-10 21:26

developer   ~0123871

The issues with TMaskEdit are already known (they are caused by 0032602).
I just verified: setting CharCase in a TMaskEdit (when it has an EditMask set) does exactly nothing (it wil always be ecNormal).
Instead controlling upper/lowercase is done in the EditMask.

So, patch from edit.charcase.2.diff improves the current situation?
If you agree, I will commit it, but not close this issue, since there are still remaining problems.

Bart Broersma

2020-07-10 22:15

developer   ~0123872

Last edited: 2020-07-10 22:17

View 3 revisions

Uploaded sample application.

Results with sample application (numbers are the times OnChange is fired):
Before
                                                           ecNormal ecUpperCase ecLowerCase
Clear Text, Press Set Text                                     1         2           2
Press Set Text again                                           0         2           2
Check "Change Text in OnChange" Clear Text, Press Set Text     3         3           3
Press Set Text again                                           3     infinite     infinite


After applying edit.charcase.2.diff
                                                           ecNormal ecUpperCase ecLowerCase
Clear Text, Press Set Text                                     1         1           1
Press Set Text again                                           0         0           0
Check "Change Text in OnChange" Clear Text, Press Set Text     3         3           3
Press Set Text again                                           3         3           3 


Of course all the 3's should become 2, once the related issue is fixed.
e.zip (3,434 bytes)

Joeny Ang

2020-07-11 10:57

reporter   ~0123878

Last edited: 2020-07-11 10:58

View 2 revisions

Yes, I think the patch is good.

Now for the 3rd OnChange(), I got this:

1BEGIN: TCustomEdit.RealSetText(): ->CamelCase
  2BEGIN: TWinControl.RealSetText(): ->CamelCase
    3BEGIN: TCustomEdit.TextChange(): CamelCase
Edit1.OnChange
Edit1.OnChange: setting Text to 'FooBar'
      4BEGIN: TCustomEdit.RealSetText(): CamelCase->FooBar
        5BEGIN: TWinControl.RealSetText(): CamelCase->FooBar
          6BEGIN: TCustomEdit.TextChange(): FooBar
Edit1.OnChange
Edit1.OnChange: setting Text to 'FooBar'
          6END
          6BEGIN: TControl.RealSetText(): FooBar->FooBar
          6END
        5END
      4END
    3END
    3BEGIN: TControl.RealSetText(): FooBar->CamelCase
      3SIGNAL: TControl.RealSetText: CM_TEXTCHANGED
      4BEGIN: TCustomEdit.TextChange(): FooBar
Edit1.OnChange
Edit1.OnChange: setting Text to 'FooBar'
      4END
    3END
  2END
1END


The 3rd trigger is from TControl.RealSetText(), which is called after the Text assignment inside OnChange() is processed. Note that the values used in the check (if RealGetText = Value then Exit;) is already wrong (comparing FooBar to CamelCase), CamelCase is the initial value assigned before OnChange(), and Foobar is the current value of Text set inside OnChange(). This happens because of re-entry.

We need to prevent the 3rd call to OnChange() from executing. How to solve this? Can we just remove the call to TControl.RealSetText()? Or have another boolean var in TControl that is by default True, and set to false in TCustomEdit to prevent the CM_TEXTCHANGED from being triggered?

Bart Broersma

2020-07-11 12:13

developer   ~0123881

IIRC then at some point TWinControl.WSSetText is called, this wil make the WS issue a message that the control has changed (which LCL picks up ansd issues a Change), and right after that TControl.SetText is called, which in turn again causes a change.
I fiddled with TWinControl implementation (removing the call to TControle.RealSetText), but that caused issues, which I didn't really understand.
So, this patch was reverted.

Bart Broersma

2020-07-11 12:17

developer   ~0123882

Committed in r63542.
I did not request it to be merged yet (given the havoc I created with my patch for 0032602)

Bart Broersma

2020-07-11 12:20

developer   ~0123883

I decided to mark this issue as resolved.
The remaining part is 0032602.

Please close this one.

Joeny Ang

2020-07-14 04:14

reporter   ~0123993

Hi, just checked out trunk, your patch is commented?

Bart Broersma

2020-07-16 23:20

developer   ~0124106

Yep, I commited after I did a final test to create the table above...

Bart Broersma

2020-07-16 23:28

developer   ~0124107

Committed it correctly now (I hope).
Next time please re-open the issue.
Merely adding a note won't grba my attention. It was just by change I saw it.
If you re-open it, it will show up prominently in my "Assigned to Me" stack.

Please close if OK now.

Joeny Ang

2020-07-17 02:58

reporter   ~0124111

Ok, thanks :)

Issue History

Date Modified Username Field Change
2020-07-08 09:15 Joeny Ang New Issue
2020-07-08 09:15 Joeny Ang File Added: tcustomedit-charcase-v1.patch
2020-07-08 10:58 Bart Broersma Note Added: 0123814
2020-07-08 11:19 Bart Broersma Relationship added related to 0032602
2020-07-08 11:26 Bart Broersma Note Edited: 0123814 View Revisions
2020-07-08 11:26 Bart Broersma Note Edited: 0123814 View Revisions
2020-07-08 11:53 Joeny Ang Note Added: 0123817
2020-07-08 15:26 Bart Broersma Note Added: 0123822
2020-07-08 15:37 Bart Broersma Note Added: 0123823
2020-07-08 15:55 Joeny Ang Note Added: 0123825
2020-07-08 16:15 Joeny Ang Note Added: 0123827
2020-07-08 16:18 Joeny Ang Note Edited: 0123827 View Revisions
2020-07-08 16:30 Bart Broersma Note Added: 0123830
2020-07-09 15:05 Bart Broersma File Deleted: edit.charcase.diff
2020-07-09 15:06 Bart Broersma Note Added: 0123840
2020-07-09 15:06 Bart Broersma File Added: edit.charcase.diff
2020-07-09 18:35 Bart Broersma Note Added: 0123849
2020-07-09 18:35 Bart Broersma File Added: edit.charcase.2.diff
2020-07-09 21:54 Bart Broersma Assigned To => Bart Broersma
2020-07-09 21:54 Bart Broersma Status new => feedback
2020-07-09 21:54 Bart Broersma LazTarget => -
2020-07-09 21:54 Bart Broersma Note Added: 0123852
2020-07-10 03:36 Joeny Ang Note Added: 0123860
2020-07-10 03:36 Joeny Ang Status feedback => assigned
2020-07-10 21:26 Bart Broersma Note Added: 0123871
2020-07-10 22:15 Bart Broersma Note Added: 0123872
2020-07-10 22:15 Bart Broersma File Added: e.zip
2020-07-10 22:16 Bart Broersma Note Edited: 0123872 View Revisions
2020-07-10 22:17 Bart Broersma Note Edited: 0123872 View Revisions
2020-07-11 10:57 Joeny Ang Note Added: 0123878
2020-07-11 10:58 Joeny Ang Note Edited: 0123878 View Revisions
2020-07-11 12:13 Bart Broersma Note Added: 0123881
2020-07-11 12:17 Bart Broersma Note Added: 0123882
2020-07-11 12:20 Bart Broersma Status assigned => resolved
2020-07-11 12:20 Bart Broersma Resolution open => fixed
2020-07-11 12:20 Bart Broersma Fixed in Version => 2.1 (SVN)
2020-07-11 12:20 Bart Broersma Fixed in Revision => r63542
2020-07-11 12:20 Bart Broersma LazTarget - => 2.2
2020-07-11 12:20 Bart Broersma Note Added: 0123883
2020-07-14 04:14 Joeny Ang Note Added: 0123993
2020-07-16 23:20 Bart Broersma Status resolved => assigned
2020-07-16 23:20 Bart Broersma Resolution fixed => open
2020-07-16 23:20 Bart Broersma Note Added: 0124106
2020-07-16 23:28 Bart Broersma Status assigned => resolved
2020-07-16 23:28 Bart Broersma Resolution open => fixed
2020-07-16 23:28 Bart Broersma Fixed in Revision r63542 => r63542, r63575
2020-07-16 23:28 Bart Broersma Note Added: 0124107
2020-07-17 02:58 Joeny Ang Note Added: 0124111
2020-08-04 06:44 Joeny Ang Status resolved => closed