View Issue Details

IDProjectCategoryView StatusLast Update
0031679LazarusLCLpublic2018-06-06 09:59
ReporterGabor Boros Assigned ToZoran Vučenović  
PrioritynormalSeverityminorReproducibilityalways
Status closedResolutionfixed 
PlatformLinux 64bitOSKubuntu 
Product Version1.7 (SVN) 
Summary0031679: TDateTimePicker - OnChange event fired more once than needed
DescriptionThe OnChange event fired when ESC pressed, fired twice when up/down arrow key pressed,...
Steps To ReproduceStart the attached project, press the ESC key, press up/down arrow keys, ...
TagsNo tags attached.
Fixed in Revisionr54645, r54651, r54652, r54653, r55133
LazTarget-
Widgetset
Attached Files

Activities

Gabor Boros

2017-04-18 20:31

reporter  

Gabor Boros

2017-04-19 10:13

reporter   ~0099677

1. The ESC problem. TCustomDateTimePicker.UndoChanges call UpdateDate without check equality of FDateTime and FConfirmedDateTime.
2. The double OnChange firing can solved by revert revision 51967 "components: datetimepicker: invoke Change/OnChange also when DateTime was changed programatically (like edit)."

Juha Manninen

2017-04-19 10:21

developer   ~0099679

Last edited: 2017-04-19 10:22

View 2 revisions

Gabor, if you know how to fix case 1. then please provide a patch.
Case 2. is Ondrej's commit from a year and a month ago. I believe it is also fixable in a clean way.

Gabor Boros

2017-04-19 12:56

reporter  

datetimepicker.pas.patch (550 bytes)   
Index: components/datetimectrls/datetimepicker.pas
===================================================================
--- components/datetimectrls/datetimepicker.pas	(revision 54635)
+++ components/datetimectrls/datetimepicker.pas	(working copy)
@@ -1923,8 +1923,11 @@
 
 procedure TCustomDateTimePicker.UndoChanges;
 begin
-  FDateTime := FConfirmedDateTime;
-  UpdateDate;
+  if (FDateTime <> FConfirmedDateTime) then
+   begin
+     FDateTime := FConfirmedDateTime;
+     UpdateDate;
+   end;
 end;
 
 { GetDateTimePartFromTextPart function
datetimepicker.pas.patch (550 bytes)   

Gabor Boros

2017-04-19 12:57

reporter   ~0099684

1. The ESC problem. Attached a patch (created against trunk 54635) which solve the problem.

Juha Manninen

2017-04-19 14:12

developer   ~0099686

I applied the patch in r54645. Thanks.

Zoran Vučenović

2017-04-20 14:01

developer   ~0099697

About problem 2:
Gabor correctly noticed that problem 2 appeared in revision 51967.

Actually, my decision was (the reason is Delphi compatibility) that OnChange fires only on user changing, not on setting date/time in code. Then I introduced some variable and firing OnChange was moved from SetDateTime to UpdateDate procedure, which first performs some checks and only then, when assured that the date/time changed on user interaction, calls Change.

However, Ondrey just added call to Change in SetDateTime (so now it fires twice when change happens on user interaction).

I'm not against switching to other way -- that OnChange fires whenever date/time gets changed (on user input or in code), but I think that it should be discussed first.
We can decide to go for Delphi compatibility or for compatibility with other widgets (TEdit).

Anyway, this change wasn't done properly, so for now I'm going to switch back the behaviour to way it was before rev. 51967 -- OnChange will be called only on user input, not when it is changed in code.

Zoran Vučenović

2017-04-20 14:25

developer   ~0099698

Applied in rev. 54651, now OnChange fires only on user input, not on date/time changes in code.

May I ask for backporting to 1.8, as this behaviour (OnChange fires twice) is regression (it doesn't happen in 1.6).

Zoran Vučenović

2017-04-20 15:09

developer   ~0099699

And about problem 1 -- solved differently, because UpdateDate should execute anyway, as text on screen might have been changed by user, even if FDateTime is still equal to FConfirmedDate.

But change is no more called from UndoChanges.
See revisions 54652 and 54653.

Juha Manninen

2017-04-20 16:04

developer   ~0099701

Last edited: 2017-04-20 16:05

View 2 revisions

Zoran, you should assign this issue to yourself. Then add the commits to be backported into this page:
 http://wiki.freepascal.org/Lazarus_1.8_fixes_branch
They will be merged later.

Zoran Vučenović

2017-04-21 07:41

developer   ~0099725

OK, thanks, Juha.

Gabor Boros

2017-04-21 12:23

reporter   ~0099728

Tried with the attached example and trunk 54656. Undo(ESC) (after changed the date ) fire the OnChange event. Is this the expected behaviour?

Zoran Vučenović

2017-04-21 16:41

developer   ~0099732

Last edited: 2017-04-21 16:55

View 2 revisions

@Gabor:
It now happens only when the underlying DateTime field actually got changed. And it is the case with undo(ESC) only when selection jumps from one date part to another (for example from day to month). Then OnChange fires twice -- when the day got changed (when selection jumped to month) and when the change got undone (when ESC key got pressed).
Yes, I'd say it is expected behaviour in this case.

Previously, it would fire even it this case:
1. Start the attached example, position on day part (it is 18 in your example).
2. press 4 on keyboard
3. press ESC
Now OnChange does not fire, previously it would.

Now, another thing -- if you want to see what is wrong with your original patch -- change UndoChanges procedure to what you proposed and do the same three steps -- the Date gets updated to 04, ignoring ESC.

I hope you are satisfied with this answer. If not, please don't hesitate to ask further. I would like to help as much as I can.

Gabor Boros

2017-04-21 17:01

reporter   ~0099733

Start the attached example, click on the dropdown button, select a different day, OnChange fired (works as expected), press ESC, OnChange fired. The OnChange after ESC is correct from your POV?

Zoran Vučenović

2017-04-21 17:42

developer   ~0099735

Yes. From my point of view, it is correct. If OnChange fires when date gets changed, it should fire again when the date is changed again (and it is realy changed twice in the case you describe).

We can discuss if the event should be prevented to fire in the first place (to postpone firing OnChange as long as it is possible to undo it with ESC), but if it got fired once, it certainly should fire again when you change it again (with ESC).

Only it does not fire any more in the three-steps scenario I described in my previous post.

Gabor Boros

2017-04-21 18:09

reporter   ~0099738

If it correct I can live with it. :-)

Before your modifications I experienced an other problem. Undo change back to the first value not to the latest correct value. But this problem not exists any more.

Ondrej Pokorny

2017-05-31 18:34

developer   ~0100754

I added the dtpoDoChangeOnSetDateTime Option to call Change on SetDateTime in r55133. It's not used by default. OnChange shouldn't be called twice if it is set.

Issue History

Date Modified Username Field Change
2017-04-18 20:30 Gabor Boros New Issue
2017-04-18 20:31 Gabor Boros File Added: DateTimePicker_OnChange.tar.gz
2017-04-19 10:13 Gabor Boros Note Added: 0099677
2017-04-19 10:21 Juha Manninen Note Added: 0099679
2017-04-19 10:22 Juha Manninen Note Edited: 0099679 View Revisions
2017-04-19 12:56 Gabor Boros File Added: datetimepicker.pas.patch
2017-04-19 12:57 Gabor Boros Note Added: 0099684
2017-04-19 14:12 Juha Manninen Note Added: 0099686
2017-04-20 14:01 Zoran Vučenović Note Added: 0099697
2017-04-20 14:25 Zoran Vučenović Note Added: 0099698
2017-04-20 15:09 Zoran Vučenović Note Added: 0099699
2017-04-20 16:04 Juha Manninen Note Added: 0099701
2017-04-20 16:05 Juha Manninen Note Edited: 0099701 View Revisions
2017-04-20 17:58 Bart Broersma Assigned To => Zoran Vučenović
2017-04-20 17:58 Bart Broersma Status new => assigned
2017-04-21 07:41 Zoran Vučenović Note Added: 0099725
2017-04-21 07:43 Zoran Vučenović LazTarget => -
2017-04-21 07:43 Zoran Vučenović Status assigned => resolved
2017-04-21 07:43 Zoran Vučenović Resolution open => fixed
2017-04-21 12:23 Gabor Boros Note Added: 0099728
2017-04-21 16:41 Zoran Vučenović Note Added: 0099732
2017-04-21 16:55 Zoran Vučenović Note Edited: 0099732 View Revisions
2017-04-21 17:01 Gabor Boros Note Added: 0099733
2017-04-21 17:42 Zoran Vučenović Note Added: 0099735
2017-04-21 18:09 Gabor Boros Note Added: 0099738
2017-04-21 22:13 Juha Manninen Fixed in Revision => r54645, r54651, r54652, r54653
2017-05-31 18:34 Ondrej Pokorny Fixed in Revision r54645, r54651, r54652, r54653 => r54645, r54651, r54652, r54653, r55133
2017-05-31 18:34 Ondrej Pokorny Note Added: 0100754
2018-06-06 09:59 Gabor Boros Status resolved => closed