[SOLVED] Improvement of TApplication.GetControlAtPos

[lagprogramming]

lcl/include/application.inc has

Code: Pascal  [Select][+][-]

1. function TApplication.GetControlAtPos(P: TPoint): TControl;
2. begin
3.   //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]);
4.   if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then
5.     Result := FLastMouseControl
6.   else
7.     Result := FindControlAtPosition(P, False);
8.  
9.   if Assigned(Result) and (csDesigning in Result.ComponentState) then
10.     Result := nil;
11.   if Assigned(Result) then
12.   begin
13.     FLastMouseControlValid := True;
14.     FLastMousePos := p;
15.     FLastMouseControl := Result;
16.   end;
17. end;

The following code avoids the last if Assigned(Result) then condition that is found in the original code. In addition to that, by switching the then and else parts of csDesigning in Result.ComponentState condition, the modified code follows FPC's programmer's guide recommendation: "Write your if/then/else statements so that the code in the "then"-part gets executed most of the time (improves the rate of successful jump prediction)."

Code: Pascal  [Select][+][-]

1. function TApplication.GetControlAtPos(P: TPoint): TControl;
2. begin
3.   //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]);
4.   if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then
5.     Result := FLastMouseControl
6.   else
7.     Result := FindControlAtPosition(P, False);
8.  
9.   if Assigned(Result) then
10.   begin
11.     if not(csDesigning in Result.ComponentState) then
12.       begin
13.       FLastMouseControlValid := True;
14.       FLastMousePos := p;
15.       FLastMouseControl := Result;
16.       end
17.     else
18.       Result := nil;
19.   end;
20. end;

The patch is attached.

[domasz]

Isn't your Result := nil in the wrong place?

Code: Pascal  [Select][+][-]

1.     function TApplication.GetControlAtPos(P: TPoint): TControl;
2.     begin
3.       //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]);
4.       if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then
5.         Result := FLastMouseControl
6.       else
7.         Result := FindControlAtPosition(P, False);
8.      
9.       if not Assigned(Result) then Exit(nil);
10.  
11.         if not(csDesigning in Result.ComponentState) then
12.           begin
13.           FLastMouseControlValid := True;
14.           FLastMousePos := p;
15.           FLastMouseControl := Result;
16.           end;
17.  
18.       end;
19.     end;

[TRon]

Isn't your Result := nil in the wrong place?

Nope.

In case the first return check is valid (FLastMouseControl or FIndControlAtPosition) only /then/ there is a check if the componentstate is in designmode and in case it is, then (and only then) the function needs to return nil.

Your correction seem to change that behaviour in that whenever the control /is in/ designmode the function will result the FLastMouseControl or FindControlAtPos (and not nil as intended).

[AlexTP]

Posted to https://gitlab.com/freepascal.org/lazarus/lazarus/-/issues/40465

[TRon]

I do (personal matter) have an issue with classifying it as a bug/improvement though because imho the heart of the matter is that it is preference. I prefer clear code before anything else even if that means that it requires some extra lines or not follow standard guidelines. Posts like that from user domasz is a good example for that (the original code was much more clear imho).

The following is much more clear imho and does not require a second eye-blink:

Code: Pascal  [Select][+][-]

1. function TApplication.GetControlAtPos(P: TPoint): TControl;
2. begin
3.   //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]);
4.   if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then
5.     Result := FLastMouseControl
6.   else
7.     Result := FindControlAtPosition(P, False);
8.      
9.   if Assigned(Result) then
10.   begin
11.     if (csDesigning in Result.ComponentState)
12.       then exit(nil);
13.  
14.     FLastMouseControlValid := True;
15.     FLastMousePos := p;
16.     FLastMouseControl := Result;
17.   end;
18. end;
19.  

or (more in line with TS proposal)

Code: Pascal  [Select][+][-]

1. function TApplication.GetControlAtPos(P: TPoint): TControl;
2. begin
3.   //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]);
4.   if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then
5.     Result := FLastMouseControl
6.   else
7.     Result := FindControlAtPosition(P, False);
8.      
9.   if Assigned(Result) then
10.   begin
11.     if (csDesigning in Result.ComponentState) then
12.       Result := nil
13.     else
14.       begin
15.         FLastMouseControlValid := True;
16.         FLastMousePos := p;
17.         FLastMouseControl := Result;
18.       end;
19.   end;
20. end;
21.  

So the fixes seem to become more and more a personal preference matter (as the posts above is mine), which is not suggesting to stop posting these kind of things as they are sometimes interesting approaches and mostly do matter, as for instance in this case preventing the double compare.

[lagprogramming]

So the fixes seem to become more and more a personal preference matter (as the posts above is mine), which is not suggesting to stop posting these kind of things as they are sometimes interesting approaches and mostly do matter, as for instance in this case preventing the double compare.

Both your code suggestions avoid the useless "if Assigned(Result) then" double check, which is fine. My proposal was based also on FPC's recommendation on arranging if/then/else code, but that's not very important for LCL. You can propose the two versions in gitlab so there would be 3 versions to choose from .
This is the subject of interest.
If the readability of the source file is not significantly reduced, are LCL developers interested in decreasing the number of conditional jumps? The only official answer is by proposing patches for a couple of routines. If the patches are accepted in the development source files then the answer is yes.
Some might consider that avoiding a single conditional jump in a routine is something insignificant. Well, looking at the routines in official source files I estimate that most of them have less than 10 conditional jumps, on average, which means that removing even a single one is a reduction of minimum 10%. For old CPUs and low powered devices that may be noticeable and measurable, especially when the conditional jump is removed from a while/repeat/for loop. MSEgui has very efficient code with very few useless conditional jumps(branches) making it very responsive.