View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0033331||Packages||LCL||public||2018-03-06 03:38||2018-04-05 16:14|
|Reporter||Toru Takubo||Assigned To||wp|
|Platform||Windows||OS||WinXP SP3 32bit|
|Summary||0033331: The TreeNode brought by TTreeView.OnChanging may be wrong.|
|Description||TTreeView.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.)
|Tags||No tags attached.|
|Fixed in Revision||57457|
||How to reproduce? Changing and selecting a node are 2 different things. I guess you must select before you can change.|
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).
||Please attach a sample application, sources only, that demonstrates the issue.|
Sample.zip (130,130 bytes)
||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.|
33331_treeview_onchanging.zip (2,649 bytes)
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.
||Fixed. Please test, and close if ok.|
||I confirmed the fix. Thank you.|
||Reopened by request of Torsten Bonde Christiansen in mailing list.|
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.
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.
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.
I wanted to add that in its current state 1.8.2, it's useless, I wonder how long it has been there ?
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?
||I'd prefer to continue the discussion in the mailing list where the chance of hearing additional opinions is greater.|
||No change in committed code. Just increased the target version to 1.10 to give users more time to update their code.|
|2018-03-06 03:38||Toru Takubo||New Issue|
|2018-03-06 07:48||Juha Manninen||Note Added: 0106911|
|2018-03-06 08:55||Toru Takubo||Note Added: 0106920|
|2018-03-06 11:28||Bart Broersma||Note Added: 0106932|
|2018-03-06 13:12||Toru Takubo||File Added: Sample.zip|
|2018-03-06 13:23||Toru Takubo||Note Edited: 0106920||View Revisions|
|2018-03-06 13:32||Toru Takubo||Note Added: 0106945|
|2018-03-06 14:05||wp||Assigned To||=> wp|
|2018-03-06 14:05||wp||Status||new => assigned|
|2018-03-06 14:12||wp||File Added: 33331_treeview_onchanging.zip|
|2018-03-06 14:14||wp||Note Added: 0106950|
|2018-03-06 14:24||wp||Fixed in Revision||=> 57457|
|2018-03-06 14:24||wp||LazTarget||=> 1.8.4|
|2018-03-06 14:24||wp||Note Added: 0106953|
|2018-03-06 14:24||wp||Status||assigned => resolved|
|2018-03-06 14:24||wp||Resolution||open => fixed|
|2018-03-06 14:24||wp||Target Version||=> 1.8.4|
|2018-03-07 00:28||Toru Takubo||Note Added: 0106961|
|2018-03-07 00:30||Toru Takubo||Note Added: 0106962|
|2018-03-07 00:30||Toru Takubo||Status||resolved => closed|
|2018-04-03 12:41||Maxim Ganetsky||Note Added: 0107560|
|2018-04-03 12:41||Maxim Ganetsky||Status||closed => assigned|
|2018-04-03 12:41||Maxim Ganetsky||Resolution||fixed => reopened|
|2018-04-03 12:54||Torsten Bonde Christiansen||Note Added: 0107562|
|2018-04-03 17:23||wp||Note Added: 0107563|
|2018-04-03 17:23||wp||Status||assigned => feedback|
|2018-04-03 21:39||jamie philbrook||Note Added: 0107567|
|2018-04-03 21:42||jamie philbrook||Note Edited: 0107567||View Revisions|
|2018-04-04 02:22||Toru Takubo||Note Added: 0107571|
|2018-04-04 02:22||Toru Takubo||Status||feedback => assigned|
|2018-04-04 07:27||wp||Note Added: 0107574|
|2018-04-05 08:08||wp||LazTarget||1.8.4 => 1.10|
|2018-04-05 08:08||wp||Note Added: 0107627|
|2018-04-05 08:08||wp||Status||assigned => resolved|
|2018-04-05 08:08||wp||Resolution||reopened => fixed|
|2018-04-05 08:08||wp||Target Version||1.8.4 => 1.10|