View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0031679 | Lazarus | LCL | public | 2017-04-18 20:30 | 2018-06-06 09:59 |
| Reporter | Gabor Boros | Assigned To | Zoran Vučenović | ||
| Priority | normal | Severity | minor | Reproducibility | always |
| Status | closed | Resolution | fixed | ||
| Platform | Linux 64bit | OS | Kubuntu | ||
| Product Version | 1.7 (SVN) | ||||
| Summary | 0031679: TDateTimePicker - OnChange event fired more once than needed | ||||
| Description | The OnChange event fired when ESC pressed, fired twice when up/down arrow key pressed,... | ||||
| Steps To Reproduce | Start the attached project, press the ESC key, press up/down arrow keys, ... | ||||
| Tags | No tags attached. | ||||
| Fixed in Revision | r54645, r54651, r54652, r54653, r55133 | ||||
| LazTarget | - | ||||
| Widgetset | |||||
| Attached Files |
| ||||
|
|
|
|
|
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)." |
|
|
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. |
|
|
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
|
|
|
1. The ESC problem. Attached a patch (created against trunk 54635) which solve the problem. |
|
|
I applied the patch in r54645. Thanks. |
|
|
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. |
|
|
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). |
|
|
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. |
|
|
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. |
|
|
OK, thanks, Juha. |
|
|
Tried with the attached example and trunk 54656. Undo(ESC) (after changed the date ) fire the OnChange event. Is this the expected behaviour? |
|
|
@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. |
|
|
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? |
|
|
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. |
|
|
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. |
|
|
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. |
| 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 |
