TTreeFilterEdit - OnFilterItem oddity

[guest58172]

Yesterday night I've lost half an hour because of an oddity in this component. The OnFilterItem event has the following type:

TFilterItemEvent = function (Item: TObject; out Done: Boolean): Boolean of object;

So for a TTreeView, I would have expect Item to be cast-able to TTreeNode. After several violation accesses, I've looked at the sources and found that the event is actually called with Item set to TObject(TTreeNode.Data).

still in master

I found this very unsafe. A TTreeNode.Data can be everything and there is no guarantee that it's an object. Also while debuging, a nil data casted as TObject is not displayed as nil, which can be quite confusing. Finally, as usual, there's no doc (description of the parameters, of the result, of how it's used), so the basic reaction I had after reading the sources is 

Is this cast an error, shouldn't a TTreeNode be passed instead of its optional data ?
(even if I supose that it's too late to change this without breakage)

[JuhaManninen]

True, it is not perfect.
Note, the event is used for all filter components, including TListFilterEdit and TListViewFilterEdit.
It could be improved though.

[Bart]

... shouldn't a TTreeNode be passed instead of its optional data ?
(even if I supose that it's too late to change this without breakage)

It should at least pass a TObject, so I would consider it a bug in either the defiition of TFilterItemEvent or in the particular use case of a TTreeView.

Bart

[JuhaManninen]

It should at least pass a TObject, so I would consider it a bug in either the defiition of TFilterItemEvent or in the particular use case of a TTreeView.

Bart, it passes a TObject. The user just must know to put something derived from TObject into the Data.
There is "Data" member in all the supported visual container items. I used it when I originally made the components, it fitted the use cases in Lazarus IDE. The events can be improved of course.

[JuhaManninen]

TFilterItemEvent = function (Item: TObject; out Done: Boolean): Boolean of object;

That one has no Caption, thus there is another recommended event now.
TFilterItemExEvent = function (ACaption: string; ItemData: TObject; out Done: Boolean): Boolean of object;

Those events work with any TCustomControlFilterEdit. Typically their data is owned and maintained by the filter component itself and copied to the visual container as needed when a user filters it.
Luckily an item of all visual containers have "Data" which can be passed to a filter. Yes, it maybe should be passed as Pointer instead of TObject. I guess it could be changed without breaking anything.

Is this cast an error, shouldn't a TTreeNode be passed instead of its optional data ?
(even if I supose that it's too late to change this without breakage)

TTreeFilterEdit is an exception. It has 2 modes which could even deserve 2 separate components.
The first mode is like the other filter edits, it owns and maintains the filtered data.
The second mode however filters a whole existing tree using each TreeNode's Visible property.
See:
 http://wiki.freepascal.org/LazControls#TTreeFilterEdit

Now I have added into TTreeFilterEdit an event
TFilterNodeEvent = function (ItemNode: TTreeNode; out Done: Boolean): Boolean of object;
used in property OnFilterNode: TFilterNodeEvent.
It should be used only with the mode number 2. Please test.

[howardpc]

Is there a compelling reason not to make (ACaption: string ...) a const parameter?

[JuhaManninen]

Is there a compelling reason not to make (ACaption: string ...) a const parameter?

Not really. It could be const. I did not think of this issue here much.
In general however I think people have an obsession to add "const" to every parameter although it is not really needed. Sometimes the parameter must be copied right away to a local variable to be modified, while a parameter without "const" could be used directly. Then it only adds useless extra clutter.
Yes I know, with const String parameters the compiler can potentially optimize a little but the difference is very small. For other parameter types it is useless IMO.

Anyway, it is a common fashion to add "const" to String parameters. I can add it there, too. Wait...

[LuizAmérico]

Yes I know, with const String parameters the compiler can potentially optimize a little but the difference is very small.

Is not that small:

http://lazarusroad.blogspot.com.br/2008/11/effect-of-using-constant-parameter-for.html

In other situations is even more evident (i will update the test in fpc to match delphi code and see how fpc handles that):

https://www.delphitools.info/2010/07/28/all-hail-the-const-parameters

Most of the time you won't modify the string just ignore it or pass to other function.

When needed to modify the argument, adding a variable and assigning the parameter to it the generated code will be equal to using the parameter without const

[LuizAmérico]

I updated my tests with fpc: http://lazarusroad.blogspot.com.br/2016/07/effect-of-using-constant-parameter-for.html

TLDR:
https://www.diffchecker.com/mew4wgor

[JuhaManninen]

I changed the ItemData parameter from TObject to Pointer and added "const" to ACaption: string parameter in r52745.
Normally tweaking event signatures is dangerous but in this case it should be safe.
TObject and Pointer always have the same size. "const" does not affect parameter passing offsets either.
Nothing can go wrong with this change!

[JuhaManninen]

TLDR:
https://www.diffchecker.com/mew4wgor

That shows a much bigger difference than the earlier comparisons. Reference counting should not generate that much code. Something is wrong ...
But yes, accumulated from thousands of functions it becomes substantial.

[Ondrej Pokorny]

Just as exercise (it doesn't really have anything common with Juha's changes): when you discuss constant string parameters, consider this code:

Code: [Select]

program ConstString;

{$APPTYPE CONSOLE}

uses
  SysUtils;

type
  TTest = class
  public
    S: string;

    procedure OhYes(A: string);
    procedure OhNo(const A: string);

    constructor Create;
  end;

{ TTest }

constructor TTest.Create;
begin
  SetLength(S, 4);
  S[1] := 't';
  S[2] := 'e';
  S[3] := 's';
  S[4] := 't';
end;

procedure TTest.OhNo(const A: string);
var
  X: string;
begin
  Writeln(A);
  Free;
  X := A;
  X := A+'hello'; // EInvalidPointer exception
  Writeln(A);
end;

procedure TTest.OhYes(A: string);
var
  X: string;
begin
  Writeln(A);
  Free;
  X := A;
  X := A+'hello'; // fine
  Writeln(A);
end;

var
  T: TTest;
begin
  T := TTest.Create;
  T.OhYes(T.S);
  T := TTest.Create;
  T.OhNo(T.S);
end.
I get the exception in Delphi 7. FPC runs the code fine without exception but the output is wrong (3 lines with "test" insted of 4 lines).

Conclusion: if you use const string parameters, make always sure that the source string lives for the whole execution. (It took me quite some time to find the bug - it wasn't that obvious since the source string was destroyed in other method).

[guest58172]

I changed the ItemData parameter from TObject to Pointer and added "const" to ACaption: string parameter in r52745.
Normally tweaking event signatures is dangerous but in this case it should be safe.
TObject and Pointer always have the same size. "const" does not affect parameter passing offsets either.
Nothing can go wrong with this change!

Thank you. I'm gonna bookmark this in case I forgot to check the change when you'll release Laz 1.8.
Also don't forget that the parameter name is confusing. It should be renamed to ItemData. When I see Item I expect the item to be passed, not its data. The renaming should't affect much user's code, we will have to add Data after item and ok go on.

[Ondrej Pokorny]

I changed the ItemData parameter from TObject to Pointer and added "const" to ACaption: string parameter in r52745.

You should do it for all methods, not only for the events. The method calls have to be fixed as well. See attached patch.

[JuhaManninen]

Thank you. I'm gonna bookmark this in case I forgot to check the change when you'll release Laz 1.8.

You should check it earlier. I understand you want to maintain your production code with Lazarus 1.6.x but having different Lazarus versions side by side is very easy. Get it from SVN, use the same compiler. Easy peacy.

Also don't forget that the parameter name is confusing. It should be renamed to ItemData. When I see Item I expect the item to be passed, not its data. The renaming should't affect much user's code, we will have to add Data after item and ok go on.

It is ItemData already. I changed it almost a week ago in r52716. Please check what was changed.
You can name your parameter as ItemData also with the current Lazarus 1.6. A function signature covers only the parameter types, not their names.

@Ondrej, thanks for the patch. My change was hasty. Applied in r52748.
Your example code on the other hand is weird. Freeing an object in the middle of its member method begs for trouble.