View Issue Details

IDProjectCategoryView StatusLast Update
0032602LazarusLCLpublic2020-07-18 08:30
Reporteraccorp Assigned To 
PrioritynormalSeverityminorReproducibilityalways
Status confirmedResolutionreopened 
OSXubuntu 16.04 
Product Version1.9 (SVN) 
Summary0032602: TEdit.OnChange event handler is calling twice for same text
DescriptionIf edit text is modified inside OnChange handler:

procedure TForm1.Edit1Change(Sender: TObject);
begin
  Edit1.Text:='from-on-change';
end;


then executing

  Edit1.Text:='test';


trigger OnChange event one time for 'test' and twice for 'from-on-change'.
Additional InformationMethod TWinControl.RealSetText is calling TControl.RealSetText with old value, while text was already updated.

procedure TWinControl.RealSetText(const AValue: TCaption); // AValue = 'test'
begin
  ...
  WSSetText(AValue); // call OnChange, set text to 'from-on-change'
  ...
  inherited RealSetText(AValue); // text was modified, but AValue = 'test'
  ...
end;

procedure TControl.RealSetText(const Value: TCaption);
begin
  if RealGetText = Value then Exit; // 'from-on-change' <> 'test' 
  FCaption := Value;
  Perform(CM_TEXTCHANGED, 0, 0); // extra OnChange
end;
TagsNo tags attached.
Fixed in Revision
LazTarget-
WidgetsetGTK 2, Win32/Win64, QT
Attached Files

Relationships

related to 0032630 closedJuha Manninen [patch] Gtk2 TEdit/TMemo: fix text selection, make OnChange event compatible with LCL-Win32 
related to 0031618 closedJuha Manninen TSpinEdit.OnChange events in GTk2 
related to 0037313 closedBart Broersma [Patch] TCustomEdit and CharCase 

Activities

accorp

2017-10-23 14:57

reporter  

bug-onchange.zip (2,546 bytes)

Juha Manninen

2017-10-23 17:09

developer   ~0103718

Looks like you know how to fix it. Please provide a patch.

accorp

2017-10-23 17:55

reporter   ~0103720

Obvious and simplest solution would be to remove "inherited RealSetText(AValue);" but I'm not familiar with lcl internals enough to say for sure, this line has been there for a while.

Index: lcl/include/wincontrol.inc
===================================================================
--- lcl/include/wincontrol.inc	(revision 56166)
+++ lcl/include/wincontrol.inc	(working copy)
@@ -8297,7 +8297,6 @@
   begin
     WSSetText(AValue);
     InvalidatePreferredSize;
-    inherited RealSetText(AValue);
     AdjustSize;
   end
   else inherited RealSetText(AValue);

jamie philbrook

2017-10-23 18:38

reporter   ~0103721

Last edited: 2017-10-23 18:41

View 2 revisions

I am not exactly following here but, I just wanted to say before things
get off on the wrong start.

 My Old Delphi docs states that this event when called requires you
to test for the MODIFIED property. This Property is to be set by code
if the TEXT property has been changed by code. If the User makes changes
then this MODIFIED property is not set and there for any code that takes
place in the OnChange event should be checking for this Property and also
any code that effects the TEXT property should be setting the MODIFIED property
to TRUE.
 
 So when you do this.
 EDIT1.TEXT := '.....';
 EDIT1.MODIFIED := True;
---
 ONChangeEvent..
    If EDIT1.MODIFIED then exit;

 Now it would make perfect sence to actually have that PROPERTY to auto set
to true if you change the TEXT via code and cleared automatically after the
OnChange event exists ..

EDIT:
 I wanted to add that it appears the TEDIT control is doing exactly as
it should be.

jamie philbrook

2017-10-23 19:05

reporter   ~0103723

Sorry for the confusion but I just ran a Test on my old D3 and the DOCs there
isn't exactly what happens.. They miss lead you into wrong thinking.

 The MODIFIED property does get set automatically when USER makes changes
to the Edit control Text...

 When code makes changes to text there is no setting of the MODIFIED property
 Also, I noticed that Setting the MODIFIED to true after you change the
text gets cleared before the Onchange Event is called. I don't know if that
is a bug or not.

 But the results here shows the MODIFIED property does get set only when
USER makes changes only, Which gives you the option to check that.

 changing the TEXT inside the Onchange event does cause a recall to the event
if the TEXT differs.
 So basically the calling twice is by design via code if the Text differs.

accorp

2017-10-23 19:31

reporter   ~0103724

Changing text in code:
Edit1.Text:='test'

Lazarus:
Edit1: text=test, modified=False
Edit1: text=from-on-change, modified=False
Edit1: text=from-on-change, modified=True

Delphi 7:
Edit1: text=test, modified=False
Edit1: text=from-on-change, modified=False


Actually, OnChange is calling three times, in title I meant two times for same text. Also, on third call "modified=True" wich is wrong anyway.

jamie philbrook

2017-10-24 02:13

reporter   ~0103728

Last edited: 2017-10-24 02:16

View 2 revisions

I think this could be part of the issue..

CustomEdit.Inc

procedure TCustomEdit.RealSetText(const AValue: TCaption);
begin
  FTextChangedByRealSetText := True;
  if FTextHintShowing and (not FSettingTextHint) then HideTextHint;
  Modified := False;
  inherited RealSetText(AValue);
  FTextChangedByRealSetText := False;
end;
Setting the Text inside the OnChange Event will Call the RealSetText above while the Inherited RealSetText has not yet returned.

 FTextChangedByRealSetText is getting out of sync. A re-entry call is going to set this to false and cause issues with the prior call.
 So it ends up being FALSE when it should remain true. This also goes
for the MODIFY property.

 maybe a local variable to record the current state and restore it prior to exiting for both ?

TCustomEdit.TextChanged will set MODIFIED to True if fTextChangedByRealSetText is false and then assume its user INPUT and not code input that is changing text.

Bart Broersma

2017-10-24 22:30

developer   ~0103744

Using a counter instead of a boolean wil solve the modified problem, but I still get an extra OnChange.

Bart Broersma

2017-10-24 23:01

developer   ~0103745

Last edited: 2017-10-24 23:11

View 2 revisions

Not calling inherited in TWinControl.RealSetText may cause FCaption not being updated.
So we may need to set it in TWinControl.RealSetText if we omit the inherited call.

On Windows this seems to work.

jamie philbrook

2017-10-24 23:07

reporter   ~0103746

Last edited: 2017-10-24 23:50

View 2 revisions

ok, how about checking for equal text as the first line?

procedure TCustomEdit.RealSetText(const AValue: TCaption);
 begin
 ++ If Text = AValue Then exit;

   FTextChangedByRealSetText := True;
   if FTextHintShowing and (not FSettingTextHint) then HideTextHint;
   Modified := False;
   inherited RealSetText(AValue);
   FTextChangedByRealSetText := False;
 end;

I know this is done in the ancestor code but the issue seems to be starting here. so maybe a check here is better which will also not warrant any
changes elsewhere.

I give up, this problem is deep..

accorp

2017-10-24 23:13

reporter   ~0103747

Last edited: 2017-10-24 23:20

View 2 revisions

@Bart Broersma
FCaption is never updated inside TControl.RealSetText (when is called from TWinControl.RealSetText) due to this line:

  if RealGetText = Value then Exit;


Bart Broersma

2017-10-24 23:16

developer   ~0103748

Last edited: 2017-10-25 11:12

View 2 revisions

The issue of the double OnChange may not be an issue with only TCustomEdit but with any TWinControl.
The issue with Modified af course must be fixed in TCustomEdit.

Bart Broersma

2017-10-25 11:09

developer   ~0103754

> FCaption is never updated inside TControl.RealSetText (when is called from
> TWinControl.RealSetText)

You're right.

Bart Broersma

2017-10-25 11:27

developer   ~0103755

Please test and close if OK.

Ondrej Pokorny

2017-11-09 22:54

developer   ~0103979

r56181 caused a regression: OnChange is not called in my custom control after your modification. I have to check why but it's way too late now.

+ IMO we shouldn't merge such bug-prone modifications that fix only minor things into a stable release at the last moment.

Juha Manninen

2017-11-10 10:00

developer   ~0103989

> + IMO we shouldn't merge such bug-prone modifications that fix only minor things into a stable release at the last moment.

I fully agree. Such changes must be tested thoroughly. Trunk is the right place for that.

Bart Broersma

2017-11-10 21:49

developer   ~0103994

Last edited: 2017-11-10 22:07

View 3 revisions

I have no idea how to solve this then.
@Ondrej/Juha: if I knew it would cause a regression I would not have committed, nor asked for merging, wouldn't I.

Looking at this with a retrospectroscope isn't helpfull IMO.

Reverted r56181.
Unassigning.

Bart Broersma

2018-02-04 18:22

developer   ~0106235

Last edited: 2018-02-18 17:39

View 3 revisions

Calling inherited RealSetText after WSSetText also causes 2 TextChanged to be executed after every change you make to a TMaskEdit (if EditMask is set).

This is caused by the fact that TMaskEdit.RealGetText removes the mask (as by design), and therefor in TControl.RealSetText the test for RealGetText = Value evaluates to False, and a Perform(CM_TEXTCHANGED, 0, 0) will be issued, when in fact already we have handled a CM_TEXTCHANGED message at that point.

Why call TControl.RealSetText in the first place if we already call WSSetText, which in turn handles everything we need?

So, contrary to previous notes: this is not a corner case issue.

Bart Broersma

2018-02-04 18:27

developer   ~0106236

Last edited: 2018-02-04 18:40

View 2 revisions

PS. I distinctly remember that changing (by user) a TMaskEdit dit NOT cause 2 OnChanges in the past (I know this, because in an old version InsertChar caused 2 OnChanges, see the comments in the InsertChar method, and I fixed that).

EDIT: this was probably a side effect of removing the "overriden" Text property from TMaskEdit and overriding RealGetText/RealSetText instead.

Bart Broersma

2018-02-13 17:32

developer   ~0106371

Last edited: 2018-02-13 17:32

View 2 revisions

> OnChange is not called in my custom control after your modification
@Ondrej, can you please try to find out why?
What's the scenario in which TControl.RealSetText needs to be called _after_ WSSetText has been performed?
Did WSSetText not trigger an OnChange in your case?

Bart Broersma

2018-02-18 17:42

developer   ~0106437

Reminder sent to: Ondrej Pokorny

See note 0106371.

Ondrej Pokorny

2018-02-18 21:01

developer   ~0106451

Yes, I'll take a look. Unfortunately fiddling with TWinControl is not simple, so I need to find an untroubled time.

Bart Broersma

2018-02-18 22:38

developer   ~0106456

OK, thanks.

Issue History

Date Modified Username Field Change
2017-10-23 14:57 accorp New Issue
2017-10-23 14:57 accorp File Added: bug-onchange.zip
2017-10-23 17:09 Juha Manninen Note Added: 0103718
2017-10-23 17:55 accorp Note Added: 0103720
2017-10-23 18:38 jamie philbrook Note Added: 0103721
2017-10-23 18:41 jamie philbrook Note Edited: 0103721 View Revisions
2017-10-23 19:05 jamie philbrook Note Added: 0103723
2017-10-23 19:31 accorp Note Added: 0103724
2017-10-24 02:13 jamie philbrook Note Added: 0103728
2017-10-24 02:16 jamie philbrook Note Edited: 0103728 View Revisions
2017-10-24 22:30 Bart Broersma Note Added: 0103744
2017-10-24 23:01 Bart Broersma Note Added: 0103745
2017-10-24 23:07 jamie philbrook Note Added: 0103746
2017-10-24 23:11 Bart Broersma Note Edited: 0103745 View Revisions
2017-10-24 23:13 accorp Note Added: 0103747
2017-10-24 23:16 Bart Broersma Note Added: 0103748
2017-10-24 23:20 accorp Note Edited: 0103747 View Revisions
2017-10-24 23:50 jamie philbrook Note Edited: 0103746 View Revisions
2017-10-25 11:09 Bart Broersma Note Added: 0103754
2017-10-25 11:12 Bart Broersma Note Edited: 0103748 View Revisions
2017-10-25 11:27 Bart Broersma Fixed in Revision => r56181
2017-10-25 11:27 Bart Broersma LazTarget => 1.8
2017-10-25 11:27 Bart Broersma Note Added: 0103755
2017-10-25 11:27 Bart Broersma Status new => resolved
2017-10-25 11:27 Bart Broersma Fixed in Version => 1.8
2017-10-25 11:27 Bart Broersma Resolution open => fixed
2017-10-25 11:27 Bart Broersma Assigned To => Bart Broersma
2017-10-25 11:27 Bart Broersma Target Version => 1.8
2017-10-25 11:47 accorp Status resolved => closed
2017-11-03 19:10 Juha Manninen Relationship added related to 0032630
2017-11-09 22:54 Ondrej Pokorny Note Added: 0103979
2017-11-09 22:54 Ondrej Pokorny Status closed => assigned
2017-11-09 22:54 Ondrej Pokorny Resolution fixed => reopened
2017-11-10 10:00 Juha Manninen Note Added: 0103989
2017-11-10 21:46 Bart Broersma Assigned To Bart Broersma =>
2017-11-10 21:49 Bart Broersma Note Added: 0103994
2017-11-10 21:49 Bart Broersma Status assigned => acknowledged
2017-11-10 21:50 Bart Broersma Note Edited: 0103994 View Revisions
2017-11-10 22:07 Bart Broersma Note Edited: 0103994 View Revisions
2018-02-04 18:22 Bart Broersma Note Added: 0106235
2018-02-04 18:24 Bart Broersma Note Edited: 0106235 View Revisions
2018-02-04 18:27 Bart Broersma Note Added: 0106236
2018-02-04 18:40 Bart Broersma Note Edited: 0106236 View Revisions
2018-02-13 17:32 Bart Broersma Note Added: 0106371
2018-02-13 17:32 Bart Broersma Note Edited: 0106371 View Revisions
2018-02-18 17:39 Bart Broersma Note Edited: 0106235 View Revisions
2018-02-18 17:42 Bart Broersma Note Added: 0106437
2018-02-18 21:01 Ondrej Pokorny Note Added: 0106451
2018-02-18 22:38 Bart Broersma Note Added: 0106456
2018-09-05 12:16 Juha Manninen LazTarget 1.8 => -
2018-09-05 12:16 Juha Manninen Fixed in Version 1.8 =>
2018-09-05 12:16 Juha Manninen Target Version 1.8 =>
2018-09-05 17:41 Juha Manninen Relationship added related to 0031618
2020-07-08 11:19 Bart Broersma Relationship added related to 0037313
2020-07-08 15:44 Bart Broersma Fixed in Revision r56181 =>
2020-07-08 15:44 Bart Broersma Widgetset GTK 2, Win32/Win64, QT => GTK 2, Win32/Win64, QT
2020-07-11 12:20 Bart Broersma Status acknowledged => confirmed