[SOLVED] Improvement of TApplication.GetControlAtPos

Forum > LCL

(1/2) > >>

lagprogramming:
lcl/include/application.inc has

--- Code: Pascal [+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i < x.length; i++) { x[i].style.maxHeight='none'; x[i].style.height = Math.min(x[i].clientHeight+15,306)+'px'; x[i].style.resize = "vertical";}};} ---function TApplication.GetControlAtPos(P: TPoint): TControl;begin //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]); if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then Result := FLastMouseControl else Result := FindControlAtPosition(P, False); if Assigned(Result) and (csDesigning in Result.ComponentState) then Result := nil; if Assigned(Result) then begin FLastMouseControlValid := True; FLastMousePos := p; FLastMouseControl := Result; end;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 [+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i < x.length; i++) { x[i].style.maxHeight='none'; x[i].style.height = Math.min(x[i].clientHeight+15,306)+'px'; x[i].style.resize = "vertical";}};} ---function TApplication.GetControlAtPos(P: TPoint): TControl;begin //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]); if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then Result := FLastMouseControl else Result := FindControlAtPosition(P, False); if Assigned(Result) then begin if not(csDesigning in Result.ComponentState) then begin FLastMouseControlValid := True; FLastMousePos := p; FLastMouseControl := Result; end else Result := nil; end;end;The patch is attached.

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

--- Code: Pascal [+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i < x.length; i++) { x[i].style.maxHeight='none'; x[i].style.height = Math.min(x[i].clientHeight+15,306)+'px'; x[i].style.resize = "vertical";}};} --- function TApplication.GetControlAtPos(P: TPoint): TControl; begin //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]); if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then Result := FLastMouseControl else Result := FindControlAtPosition(P, False); if not Assigned(Result) then Exit(nil); if not(csDesigning in Result.ComponentState) then begin FLastMouseControlValid := True; FLastMousePos := p; FLastMouseControl := Result; end; end; end;

TRon:

--- Quote from: domasz on August 25, 2023, 01:34:42 pm ---Isn't your Result := nil in the wrong place?

--- End quote ---
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 [+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i < x.length; i++) { x[i].style.maxHeight='none'; x[i].style.height = Math.min(x[i].clientHeight+15,306)+'px'; x[i].style.resize = "vertical";}};} ---function TApplication.GetControlAtPos(P: TPoint): TControl;begin //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]); if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then Result := FLastMouseControl else Result := FindControlAtPosition(P, False); if Assigned(Result) then begin if (csDesigning in Result.ComponentState) then exit(nil); FLastMouseControlValid := True; FLastMousePos := p; FLastMouseControl := Result; end;end;
or (more in line with TS proposal)

--- Code: Pascal [+][-]window.onload = function(){var x1 = document.getElementById("main_content_section"); if (x1) { var x = document.getElementsByClassName("geshi");for (var i = 0; i < x.length; i++) { x[i].style.maxHeight='none'; x[i].style.height = Math.min(x[i].clientHeight+15,306)+'px'; x[i].style.resize = "vertical";}};} ---function TApplication.GetControlAtPos(P: TPoint): TControl;begin //debugln(['TApplication.GetControlAtPos p=',dbgs(p),' FLastMousePos=',dbgs(FLastMousePos)]); if FLastMouseControlValid and (P.X = FLastMousePos.x) and (P.Y = FLastMousePos.Y) then Result := FLastMouseControl else Result := FindControlAtPosition(P, False); if Assigned(Result) then begin if (csDesigning in Result.ComponentState) then Result := nil else begin FLastMouseControlValid := True; FLastMousePos := p; FLastMouseControl := Result; end; end;end;
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.

Navigation

[0] Message Index

[#] Next page