View Issue Details

IDProjectCategoryView StatusLast Update
0033331PackagesLCLpublic2018-04-05 18:14
ReporterToru Takubo Assigned Towp  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
PlatformWindowsOSWinXP SP3 32bit 
Product Version1.8.2 
Target Version1.10 
Summary0033331: The TreeNode brought by TTreeView.OnChanging may be wrong.
DescriptionTTreeView.OnChanging event brings a parameter Node:TTreeNode which is currently selected one. Shouldn't it be a being selected one? I already know the old (selected) but not know the new, which is needed to decide AllowChange true or not.

Actually, TTreeNode on Delphi 7 worked as I expected. This behavior must be Delphi
incompatible. (I only tested on D7, not later version.)
TagsNo tags attached.
Fixed in Revision57457
LazTarget1.10
WidgetsetWin32/Win64
Attached Files

Activities

Juha Manninen

2018-03-06 08:48

developer   ~0106911

How to reproduce? Changing and selecting a node are 2 different things. I guess you must select before you can change.

Toru Takubo

2018-03-06 09:55

reporter   ~0106920

Last edited: 2018-03-06 14:23

View 2 revisions

Steps to reproduce is following.

1) Create a new application.
2) Put TTreeView and TMemo on the Form1.
3) By using the object inspector, add multiple TTreeNode items on the TreeView1.
4) Create the OnChanging event handler on the TTreeView and put the line "Memo1.Lines.Add(Node.Text);".
5) Build and run the application.
6) First nothing is selected. If you select one of the items (suppose item3) in the TTreeNode, nothing is shown on the Memo1.
7) Next if you select item5, then "item3" is shown on the Memo1.

I mean I expect "item3" on the step 6) and "item5" on then step 7).

Bart Broersma

2018-03-06 12:28

developer   ~0106932

Please attach a sample application, sources only, that demonstrates the issue.

Toru Takubo

2018-03-06 14:12

reporter  

Sample.zip (130,130 bytes)

Toru Takubo

2018-03-06 14:32

reporter   ~0106945

I attached a sample project which is described in my previous comment. I had a typo there, not OnChange event but OnChanging event, so I corrected. Anyway, please select one of the items in the TreeView, and see what is returned as a "Node" parameter of the OnChanging event.

wp

2018-03-06 15:12

developer  

wp

2018-03-06 15:14

developer   ~0106950

Attaching a modified version of the demo which allows also whether the AllowChange works.

I can confirm that the Lazarus behavior is different from Delphi which has the destination node in the parameter - which exactly makes sense.

wp

2018-03-06 15:24

developer   ~0106953

Fixed. Please test, and close if ok.

Toru Takubo

2018-03-07 01:28

reporter   ~0106961

I confirmed the fix. Thank you.

Toru Takubo

2018-03-07 01:30

reporter   ~0106962

close done.

Maxim Ganetsky

2018-04-03 14:41

developer   ~0107560

Reopened by request of Torsten Bonde Christiansen in mailing list.

Torsten Bonde Christiansen

2018-04-03 14:54

reporter   ~0107562

Thank you Maxim Ganetsky.

I object to this change because the previous was is a long standing behaviour of the component, which breaks backwards compatibility - hard!

Also this is not in line with other components that also has OnChanging/OnChange callback methods where OnChanging contains the current selected item, and OnChange contains the newly selected item.

wp

2018-04-03 19:23

developer   ~0107563

I don't see anything wrong in the logics of the new behavior: The OnChanging event is fired before the selected node is changed. It has the node which will become the new selected node as a parameter, and you can do some validation and signal in AllowChange whether the change to the new node will be allowed or not. In the event handler you still have access to the currently selected node as TreeView.Selected. Therefore, there is no need at all to duplicate the currently selected node in the node parameter of the event.

This behavior is now compatible with Delphi (it was not before the fix).

Like any bugfix the new code breaks backward compatibility - sorry for the inconvenience if you have relied on the old behavior.

Which other controls have the OnChanging event? I only found TPageControl/TTabControl but they don't pass the changing tab as a parameter.

jamie philbrook

2018-04-03 23:39

reporter   ~0107567

Last edited: 2018-04-03 23:42

View 2 revisions

I just checked my old Delphi and yes it should be reporting the node that it's
going to or wants to go to, not report the one that is already selected before
the desired change.

EDIT:
 I wanted to add that in its current state 1.8.2, it's useless, I wonder how long it has been there ?

Toru Takubo

2018-04-04 04:22

reporter   ~0107571

I totally agree with wp. I also searched for OnChanging event in lazarus\lcl and lazarus\components, but I could not find any which contains a parameter representing either current or coming item. Only one close result is TUpDown.OnChangingEx which includes "NewValue".
Even when OnChanging event of some other component contains current item, shouldn't that be fixed to keep consistency?

wp

2018-04-04 09:27

developer   ~0107574

I'd prefer to continue the discussion in the mailing list where the chance of hearing additional opinions is greater.

wp

2018-04-05 10:08

developer   ~0107627

No change in committed code. Just increased the target version to 1.10 to give users more time to update their code.

Issue History

Date Modified Username Field Change
2018-03-06 04:38 Toru Takubo New Issue
2018-03-06 08:48 Juha Manninen Note Added: 0106911
2018-03-06 09:55 Toru Takubo Note Added: 0106920
2018-03-06 12:28 Bart Broersma Note Added: 0106932
2018-03-06 14:12 Toru Takubo File Added: Sample.zip
2018-03-06 14:23 Toru Takubo Note Edited: 0106920 View Revisions
2018-03-06 14:32 Toru Takubo Note Added: 0106945
2018-03-06 15:05 wp Assigned To => wp
2018-03-06 15:05 wp Status new => assigned
2018-03-06 15:12 wp File Added: 33331_treeview_onchanging.zip
2018-03-06 15:14 wp Note Added: 0106950
2018-03-06 15:24 wp Fixed in Revision => 57457
2018-03-06 15:24 wp LazTarget => 1.8.4
2018-03-06 15:24 wp Note Added: 0106953
2018-03-06 15:24 wp Status assigned => resolved
2018-03-06 15:24 wp Resolution open => fixed
2018-03-06 15:24 wp Target Version => 1.8.4
2018-03-07 01:28 Toru Takubo Note Added: 0106961
2018-03-07 01:30 Toru Takubo Note Added: 0106962
2018-03-07 01:30 Toru Takubo Status resolved => closed
2018-04-03 14:41 Maxim Ganetsky Note Added: 0107560
2018-04-03 14:41 Maxim Ganetsky Status closed => assigned
2018-04-03 14:41 Maxim Ganetsky Resolution fixed => reopened
2018-04-03 14:54 Torsten Bonde Christiansen Note Added: 0107562
2018-04-03 19:23 wp Note Added: 0107563
2018-04-03 19:23 wp Status assigned => feedback
2018-04-03 23:39 jamie philbrook Note Added: 0107567
2018-04-03 23:42 jamie philbrook Note Edited: 0107567 View Revisions
2018-04-04 04:22 Toru Takubo Note Added: 0107571
2018-04-04 04:22 Toru Takubo Status feedback => assigned
2018-04-04 09:27 wp Note Added: 0107574
2018-04-05 10:08 wp LazTarget 1.8.4 => 1.10
2018-04-05 10:08 wp Note Added: 0107627
2018-04-05 10:08 wp Status assigned => resolved
2018-04-05 10:08 wp Resolution reopened => fixed
2018-04-05 10:08 wp Target Version 1.8.4 => 1.10