From d2ebe7fe04008f9f829a2e224f8e66d5fa9e76c0 Mon Sep 17 00:00:00 2001 From: Antoine HAFFREINGUE Date: Wed, 6 May 2026 14:26:46 +0200 Subject: [PATCH 1/2] Adding boundary security in Current coordinates setter --- source/_magnifier/magnifier.py | 39 ++++++++- tests/unit/test_magnifier/test_magnifier.py | 88 ++++++++++++++++----- 2 files changed, 104 insertions(+), 23 deletions(-) diff --git a/source/_magnifier/magnifier.py b/source/_magnifier/magnifier.py index 4a757c88a70..60b0df52548 100644 --- a/source/_magnifier/magnifier.py +++ b/source/_magnifier/magnifier.py @@ -89,6 +89,25 @@ def zoomLevel(self, value: float) -> None: value = closestZoom self._zoomLevel = value + @property + def currentCoordinates(self) -> Coordinates: + """ + Get the current coordinates of the magnifier. + + :return: The current coordinates + """ + return self._currentCoordinates + + @currentCoordinates.setter + def currentCoordinates(self, coordinates: Coordinates) -> None: + """ + Set the current coordinates of the magnifier, applying screen boundary protection. + The magnifier will never move beyond the visible screen boundaries. + + :param coordinates: The new coordinates to set + """ + self._currentCoordinates = self._clampCoordinates(coordinates) + def _getScreenLimits(self) -> tuple[int, int, int, int]: """ Get screen coordinate limits based on current mode. @@ -108,6 +127,20 @@ def _getScreenLimits(self) -> tuple[int, int, int, int]: maxY = int(self._displayOrientation.height - (visibleHeight / 2)) return (minX, minY, maxX, maxY) + def _clampCoordinates(self, coordinates: Coordinates) -> Coordinates: + """ + Clamp coordinates to stay within screen boundaries. + Ensures the magnified view always displays content within the visible range. + + :param coordinates: The coordinates to clamp + :return: The clamped coordinates + """ + x, y = coordinates + minX, minY, maxX, maxY = self._getScreenLimits() + x = max(minX, min(x, maxX)) + y = max(minY, min(y, maxY)) + return Coordinates(x, y) + def _setZoomRawValue(self, value: float) -> None: """ Set zoom level directly without validation. @@ -143,7 +176,7 @@ def _startMagnifier(self) -> None: return self._isActive = True - self._currentCoordinates = self._focusManager.getCurrentFocusCoordinates() + self.currentCoordinates = self._focusManager.getCurrentFocusCoordinates() def _updateMagnifier(self) -> None: """ @@ -157,7 +190,7 @@ def _updateMagnifier(self) -> None: try: self._managePanning() if not self._isManualPanning: - self._currentCoordinates = self._focusManager.getCurrentFocusCoordinates() + self.currentCoordinates = self._focusManager.getCurrentFocusCoordinates() if shouldKeepMouseCentered(): self._keepMouseCentered() self._doUpdate() @@ -307,7 +340,7 @@ def _pan(self, action: MagnifierAction) -> bool: log.error(f"Unknown pan action: {action}") self._isManualPanning = True - self._currentCoordinates = Coordinates(x, y) + self.currentCoordinates = Coordinates(x, y) self._doUpdate() return (x, y) != (originalX, originalY) diff --git a/tests/unit/test_magnifier/test_magnifier.py b/tests/unit/test_magnifier/test_magnifier.py index 270fa8f43c6..9f69891671a 100644 --- a/tests/unit/test_magnifier/test_magnifier.py +++ b/tests/unit/test_magnifier/test_magnifier.py @@ -94,8 +94,10 @@ def testZoomLevelProperty(self): def testStartMagnifier(self): """Activating the magnifier.""" + # Use center coordinates which will always be within bounds + focusCoords = Coordinates(self.screenWidth // 2, self.screenHeight // 2) self.magnifier._focusManager.getCurrentFocusCoordinates = MagicMock( - return_value=Coordinates(100, 200), + return_value=focusCoords, ) # Test starting from inactive state @@ -103,7 +105,7 @@ def testStartMagnifier(self): self.magnifier._startMagnifier() self.assertTrue(self.magnifier._isActive) - self.assertEqual(self.magnifier._currentCoordinates, Coordinates(100, 200)) + self.assertEqual(self.magnifier.currentCoordinates, focusCoords) self.magnifier._focusManager.getCurrentFocusCoordinates.assert_called_once() # Test starting when already active (should not call getCurrentFocusCoordinates again) @@ -115,8 +117,10 @@ def testStartMagnifier(self): def testUpdateMagnifier(self): """Updating the magnifier's properties.""" + # Use center coordinates which will always be within bounds + focusCoords = Coordinates(self.screenWidth // 2, self.screenHeight // 2) self.magnifier._focusManager.getCurrentFocusCoordinates = MagicMock( - return_value=Coordinates(100, 200), + return_value=focusCoords, ) self.magnifier._doUpdate = MagicMock() self.magnifier._startTimer = MagicMock() @@ -131,7 +135,7 @@ def testUpdateMagnifier(self): self.magnifier._isActive = True self.magnifier._updateMagnifier() - # getCurrentFocusCoordinates is called twice: once in _managePanning and once to update _currentCoordinates + # getCurrentFocusCoordinates is called twice: once in _managePanning and once to update currentCoordinates self.assertEqual( self.magnifier._focusManager.getCurrentFocusCoordinates.call_count, 2, @@ -140,15 +144,16 @@ def testUpdateMagnifier(self): self.magnifier._startTimer.assert_called_once_with( self.magnifier._updateMagnifier, ) - self.assertEqual(self.magnifier._currentCoordinates, Coordinates(100, 200)) + self.assertEqual(self.magnifier.currentCoordinates, focusCoords) # Successful update should reset error counter self.assertEqual(self.magnifier._consecutiveErrors, 0) def testUpdateMagnifierResumesAfterSingleError(self): """Timer must always be rescheduled even when _doUpdate raises an exception.""" self.magnifier._isActive = True + focusCoords = Coordinates(self.screenWidth // 2, self.screenHeight // 2) self.magnifier._focusManager.getCurrentFocusCoordinates = MagicMock( - return_value=Coordinates(100, 200), + return_value=focusCoords, ) self.magnifier._doUpdate = MagicMock(side_effect=OSError("COM failure")) self.magnifier._startTimer = MagicMock() @@ -164,8 +169,9 @@ def testUpdateMagnifierResumesAfterSingleError(self): def testUpdateMagnifierTriggersRecoveryAfterMaxErrors(self): """After _MAX_CONSECUTIVE_ERRORS failures, _attemptRecovery is called instead of restarting timer.""" self.magnifier._isActive = True + focusCoords = Coordinates(self.screenWidth // 2, self.screenHeight // 2) self.magnifier._focusManager.getCurrentFocusCoordinates = MagicMock( - return_value=Coordinates(100, 200), + return_value=focusCoords, ) self.magnifier._doUpdate = MagicMock(side_effect=OSError("COM failure")) self.magnifier._startTimer = MagicMock() @@ -182,8 +188,9 @@ def testUpdateMagnifierTriggersRecoveryAfterMaxErrors(self): def testUpdateMagnifierCatchesCOMError(self): """COMError from UIA must be caught and the timer rescheduled.""" self.magnifier._isActive = True + focusCoords = Coordinates(self.screenWidth // 2, self.screenHeight // 2) self.magnifier._focusManager.getCurrentFocusCoordinates = MagicMock( - return_value=Coordinates(100, 200), + return_value=focusCoords, ) self.magnifier._doUpdate = MagicMock(side_effect=COMError(-2147417848, "RPC_E_DISCONNECTED", None)) self.magnifier._startTimer = MagicMock() @@ -196,8 +203,9 @@ def testUpdateMagnifierCatchesCOMError(self): def testUpdateMagnifierRecoveryFailureSafelyRestartsTimer(self): """If _attemptRecovery itself raises, the timer must still be restarted to prevent a freeze.""" self.magnifier._isActive = True + focusCoords = Coordinates(self.screenWidth // 2, self.screenHeight // 2) self.magnifier._focusManager.getCurrentFocusCoordinates = MagicMock( - return_value=Coordinates(100, 200), + return_value=focusCoords, ) self.magnifier._doUpdate = MagicMock(side_effect=OSError("API failure")) self.magnifier._startTimer = MagicMock() @@ -214,8 +222,9 @@ def testUpdateMagnifierResetsErrorCountOnSuccess(self): """A successful update after errors resets the consecutive error counter.""" self.magnifier._isActive = True self.magnifier._consecutiveErrors = 2 + focusCoords = Coordinates(self.screenWidth // 2, self.screenHeight // 2) self.magnifier._focusManager.getCurrentFocusCoordinates = MagicMock( - return_value=Coordinates(100, 200), + return_value=focusCoords, ) self.magnifier._doUpdate = MagicMock() # Success self.magnifier._startTimer = MagicMock() @@ -287,7 +296,7 @@ def _setupPanTest(self): self.magnifier._panStep = 10 # 10% of screen width centerX = self.screenWidth // 2 centerY = self.screenHeight // 2 - self.magnifier._currentCoordinates = Coordinates(centerX, centerY) + self.magnifier.currentCoordinates = Coordinates(centerX, centerY) expectedPanPixels = int( (self.screenWidth / self.magnifier.zoomLevel) * 10 / 100, ) @@ -318,30 +327,30 @@ def _testSimplePan( # Test normal pan - movement succeeds (position changes) hasMoved = self.magnifier._pan(action) self.assertTrue(hasMoved) - currentValue = getattr(self.magnifier._currentCoordinates, axis) + currentValue = getattr(self.magnifier.currentCoordinates, axis) self.assertEqual(currentValue, centerValue + direction * expectedPanPixels) # Test reaching edge - movement succeeds on first contact (position changes to edge) if axis == "x": - self.magnifier._currentCoordinates = Coordinates( + self.magnifier.currentCoordinates = Coordinates( edgeValue - direction * expectedPanPixels, centerY, ) else: - self.magnifier._currentCoordinates = Coordinates( + self.magnifier.currentCoordinates = Coordinates( centerX, edgeValue - direction * expectedPanPixels, ) hasMoved = self.magnifier._pan(action) self.assertTrue(hasMoved) - currentValue = getattr(self.magnifier._currentCoordinates, axis) + currentValue = getattr(self.magnifier.currentCoordinates, axis) self.assertEqual(currentValue, edgeValue) # Test trying to pan beyond edge - movement fails (already at edge, no movement) hasMoved = self.magnifier._pan(action) self.assertFalse(hasMoved) - currentValue = getattr(self.magnifier._currentCoordinates, axis) + currentValue = getattr(self.magnifier.currentCoordinates, axis) self.assertEqual(currentValue, edgeValue) def _testPanToEdge(self, action: MagnifierAction, axis: str, edgeAttr: str): @@ -361,13 +370,13 @@ def _testPanToEdge(self, action: MagnifierAction, axis: str, edgeAttr: str): # Test jump to edge - movement succeeds (moves to edge) hasMoved = self.magnifier._pan(action) self.assertTrue(hasMoved) - currentValue = getattr(self.magnifier._currentCoordinates, axis) + currentValue = getattr(self.magnifier.currentCoordinates, axis) self.assertEqual(currentValue, edgeValue) # Test trying to pan to edge again - movement fails (already at edge, no movement) hasMoved = self.magnifier._pan(action) self.assertFalse(hasMoved) - currentValue = getattr(self.magnifier._currentCoordinates, axis) + currentValue = getattr(self.magnifier.currentCoordinates, axis) self.assertEqual(currentValue, edgeValue) def testPanLeft(self): @@ -464,8 +473,8 @@ def testManagePanning(self): self.assertEqual(self.magnifier._lastFocusCoordinates, focusB) def testKeepMouseCentered(self): - """Base _keepMouseCentered moves cursor to _currentCoordinates.""" - self.magnifier._currentCoordinates = Coordinates(640, 360) + """Base _keepMouseCentered moves cursor to currentCoordinates.""" + self.magnifier.currentCoordinates = Coordinates(640, 360) with patch("_magnifier.magnifier.winUser.setCursorPos") as mockSetCursor: self.magnifier._keepMouseCentered() mockSetCursor.assert_called_once_with(640, 360) @@ -497,3 +506,42 @@ def testStopTimer(self): # Test stopping when no timer exists (should not raise error) self.magnifier._stopTimer() self.assertIsNone(self.magnifier._timer) + + def testClampCoordinates(self): + """Test all boundary clamps (left, right, top, bottom) for both modes.""" + for isTrueCentered in (False, True): + self.magnifier.zoomLevel = 2.0 + with patch("_magnifier.magnifier.isTrueCentered", return_value=isTrueCentered): + minX, minY, maxX, maxY = self.magnifier._getScreenLimits() + + # Test left boundary (far below minimum) + self.magnifier.currentCoordinates = Coordinates(-1000, 100) + self.assertGreaterEqual(self.magnifier.currentCoordinates.x, minX) + self.assertLessEqual(self.magnifier.currentCoordinates.x, maxX) + + # Test right boundary (far above maximum) + self.magnifier.currentCoordinates = Coordinates(100000, 100) + self.assertGreaterEqual(self.magnifier.currentCoordinates.x, minX) + self.assertLessEqual(self.magnifier.currentCoordinates.x, maxX) + + # Test top boundary (far above minimum) + self.magnifier.currentCoordinates = Coordinates(100, -1000) + self.assertGreaterEqual(self.magnifier.currentCoordinates.y, minY) + self.assertLessEqual(self.magnifier.currentCoordinates.y, maxY) + + # Test bottom boundary (far above maximum) + self.magnifier.currentCoordinates = Coordinates(100, 100000) + self.assertGreaterEqual(self.magnifier.currentCoordinates.y, minY) + self.assertLessEqual(self.magnifier.currentCoordinates.y, maxY) + + def testClampCoordinatesWithinBounds(self): + """Coordinates within bounds are not modified.""" + self.magnifier.zoomLevel = 2.0 + with patch("_magnifier.magnifier.isTrueCentered", return_value=False): + minX, minY, maxX, maxY = self.magnifier._getScreenLimits() + centerX = (minX + maxX) // 2 + centerY = (minY + maxY) // 2 + + validCoords = Coordinates(centerX, centerY) + self.magnifier.currentCoordinates = validCoords + self.assertEqual(self.magnifier.currentCoordinates, validCoords) From a8b1ce0b3eb8728f4ab2ff261031f581c95fced9 Mon Sep 17 00:00:00 2001 From: Antoine HAFFREINGUE Date: Wed, 6 May 2026 15:21:45 +0200 Subject: [PATCH 2/2] copilot revie --- source/_magnifier/fullscreenMagnifier.py | 8 ++++---- source/_magnifier/magnifier.py | 8 ++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/source/_magnifier/fullscreenMagnifier.py b/source/_magnifier/fullscreenMagnifier.py index b9d1cd697d3..b10632cc80e 100644 --- a/source/_magnifier/fullscreenMagnifier.py +++ b/source/_magnifier/fullscreenMagnifier.py @@ -37,7 +37,7 @@ class FullScreenMagnifier(Magnifier): def __init__(self): super().__init__() self._fullscreenMode = getFullscreenMode() - self._currentCoordinates = Coordinates(0, 0) + self.currentCoordinates = Coordinates(0, 0) self._spotlightManager = SpotlightManager(self) self._displaySize = Size(self._displayOrientation.width, self._displayOrientation.height) self._startMagnifier() @@ -102,7 +102,7 @@ def _initializeNativeMagnification(self) -> None: # Applying the first real update verifies the API is usable without # briefly jumping the magnified view to the top-left corner. try: - coordinates = self._getCoordinatesForMode(self._currentCoordinates) + coordinates = self._getCoordinatesForMode(self.currentCoordinates) # Save screen position for mode continuity, matching _doUpdate. self._lastScreenPosition = coordinates self._fullscreenMagnifier(coordinates) @@ -115,7 +115,7 @@ def _doUpdate(self): Perform the actual update of the magnifier """ # Calculate new position based on focus mode - coordinates = self._getCoordinatesForMode(self._currentCoordinates) + coordinates = self._getCoordinatesForMode(self.currentCoordinates) # Always save screen position for mode continuity self._lastScreenPosition = coordinates @@ -267,7 +267,7 @@ def _keepMouseCentered(self) -> None: ): log.debug("Mouse button pressed, skipping cursor repositioning to avoid interfering with click") return - coordinates = self._getCoordinatesForMode(self._currentCoordinates) + coordinates = self._getCoordinatesForMode(self.currentCoordinates) params = self._getMagnifierParameters(coordinates) centerX = params.coordinates.x + params.magnifierSize.width // 2 centerY = params.coordinates.y + params.magnifierSize.height // 2 diff --git a/source/_magnifier/magnifier.py b/source/_magnifier/magnifier.py index 60b0df52548..afe0548d300 100644 --- a/source/_magnifier/magnifier.py +++ b/source/_magnifier/magnifier.py @@ -308,15 +308,11 @@ def _pan(self, action: MagnifierAction) -> bool: :param action: The pan action (left, right, up, down) :return: True if the actions results in the pan successfully moving, False otherwise. """ - x, y = self._currentCoordinates + x, y = self.currentCoordinates originalX, originalY = x, y minX, minY, maxX, maxY = self._getScreenLimits() - # Clamp current position if out of bounds - x = max(minX, min(x, maxX)) - y = max(minY, min(y, maxY)) - panPixels = int((self._displayOrientation.width / self.zoomLevel) * self._panStep / 100) match action: @@ -360,7 +356,7 @@ def _keepMouseCentered(self) -> None: Move the mouse cursor to the center of the magnified view. Subclasses may override this to adapt the behavior for specific modes. """ - centerX, centerY = self._currentCoordinates + centerX, centerY = self.currentCoordinates winUser.setCursorPos(centerX, centerY) def _startTimer(self, callback: Callable[[], None] = None) -> None: