Recent

Author Topic: [SOLVED] Improvement of TApplication.GetControlAtPos  (Read 2396 times)

lagprogramming

  • Sr. Member
  • ****
  • Posts: 407
[SOLVED] Improvement of TApplication.GetControlAtPos
« on: August 25, 2023, 12:15:19 pm »
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.
« Last Edit: August 26, 2023, 02:24:00 pm by lagprogramming »

domasz

  • Hero Member
  • *****
  • Posts: 553
Re: Improvement of TApplication.GetControlAtPos
« Reply #1 on: August 25, 2023, 01:34:42 pm »
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;
« Last Edit: August 25, 2023, 01:37:49 pm by domasz »

TRon

  • Hero Member
  • *****
  • Posts: 3623
Re: Improvement of TApplication.GetControlAtPos
« Reply #2 on: August 25, 2023, 03:59:44 pm »
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).
This tagline is powered by AI (AI advertisement: Free Pascal the only programming language that matters)


TRon

  • Hero Member
  • *****
  • Posts: 3623
Re: Improvement of TApplication.GetControlAtPos
« Reply #4 on: August 25, 2023, 05:29:18 pm »
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.
« Last Edit: August 25, 2023, 05:38:31 pm by TRon »
This tagline is powered by AI (AI advertisement: Free Pascal the only programming language that matters)

lagprogramming

  • Sr. Member
  • ****
  • Posts: 407
Re: Improvement of TApplication.GetControlAtPos
« Reply #5 on: August 25, 2023, 07:38:15 pm »
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.

 

TinyPortal © 2005-2018