diff --git a/src/aare/gui/main_window.py b/src/aare/gui/main_window.py index 4022bbed..982923db 100644 --- a/src/aare/gui/main_window.py +++ b/src/aare/gui/main_window.py @@ -167,10 +167,12 @@ class _AlertBannerHost(QWidget): class MainWindow(QMainWindow): sample_geometry = Signal(SampleGeometryModel) - # Set lazily outside __init__ (first use guards with getattr/default); - # declared for the basedpyright gate. + # Class-level defaults: Qt can call overrides (showEvent, closeEvent) + # before __init__ finishes, so these must be readable without a getattr + # guard; also typed for the basedpyright gate. _session_operations_enabled: bool | None = None _default_dock_split_done: bool = False + _cleanup_done: bool = False _pre_watch_dock_state: QByteArray | None = None _pre_watch_visibility: list[tuple[QWidget, bool]] | None = None @@ -207,7 +209,6 @@ class MainWindow(QMainWindow): self._beamline_recovery_dialog = None self._local_contact_dialog = None self._controls_help_dialog = None - self._cleanup_done = False self._default_window_state = None self._pre_automation_window_state = None self._pre_automation_left_column_visible = True @@ -241,18 +242,19 @@ class MainWindow(QMainWindow): self.state_manager = UIStateManager("PSI", "AareGUI") # App-level, not window-level: dialogs and pop-outs get it too. - self._clickable_cursor_filter = ClickableCursorFilter(self) + # Installed once per process and parented to the app, NOT the window: + # the old per-window copies stacked up (one per MainWindow the test + # suite builds) and kept filtering every process event after their + # parent window died — stale wrappers in the hottest Qt→Python path. app = QApplication.instance() assert app is not None - app.installEventFilter(self._clickable_cursor_filter) - - # Wheel safety: sliders/spin boxes/combos only react to the wheel - # while the right mouse button is held; a bare wheel just scrolls - # the page — it can never nudge a value or move a motor. - self._wheel_value_guard = WheelValueGuard(self) - app_instance = QApplication.instance() - if app_instance is not None: - app_instance.installEventFilter(self._wheel_value_guard) + if not app.property("_aare_app_filters_installed"): + app.setProperty("_aare_app_filters_installed", True) + app.installEventFilter(ClickableCursorFilter(app)) + # Wheel safety: sliders/spin boxes/combos only react to the wheel + # while the right mouse button is held; a bare wheel just scrolls + # the page — it can never nudge a value or move a motor. + app.installEventFilter(WheelValueGuard(app)) self.viewer = JFJochDBusClient() try: @@ -2992,7 +2994,7 @@ class MainWindow(QMainWindow): # default dock split AFTER the real (maximized) geometry exists — # the __init__ resizeDocks ran on the pre-show size and Qt hands the # scale-up surplus to the sample list, skewing 50/50 into ~80/20. - if not getattr(self, "_default_dock_split_done", False): + if not self._default_dock_split_done: self._default_dock_split_done = True if not self.state_manager.settings.value("main_window/state"): QTimer.singleShot(0, self._apply_default_dock_split) @@ -3041,7 +3043,7 @@ class MainWindow(QMainWindow): super().closeEvent(event) def cleanup(self): - if getattr(self, "_cleanup_done", False): + if self._cleanup_done: return try: @@ -3209,21 +3211,21 @@ class MainWindow(QMainWindow): obj.setFloating(False) event.ignore() return True - try: - if event.type() in { - QEvent.Type.MouseButtonPress, - QEvent.Type.MouseButtonRelease, - QEvent.Type.MouseMove, - QEvent.Type.Wheel, - QEvent.Type.KeyPress, - QEvent.Type.KeyRelease, - QEvent.Type.FocusIn, - QEvent.Type.TouchBegin, - QEvent.Type.TouchUpdate, - }: - self._mark_user_interaction() - except Exception as e: - logger.debug(f"GUI interaction event filter error: {e}", exc_info=True) + # No try/except: every attribute this touches exists before the first + # install, and the one risky call (backend report) guards itself in + # _refresh_idle_activity. + if event.type() in { + QEvent.Type.MouseButtonPress, + QEvent.Type.MouseButtonRelease, + QEvent.Type.MouseMove, + QEvent.Type.Wheel, + QEvent.Type.KeyPress, + QEvent.Type.KeyRelease, + QEvent.Type.FocusIn, + QEvent.Type.TouchBegin, + QEvent.Type.TouchUpdate, + }: + self._mark_user_interaction() # getattr defaults: this filter also runs for events delivered while # __init__ is still building (or teardown is tearing down) the very # widgets it inspects — a raise here spams every event and breaks diff --git a/src/aare/gui/panels/fluorescence_panel.py b/src/aare/gui/panels/fluorescence_panel.py index b67f25a7..84109e2c 100644 --- a/src/aare/gui/panels/fluorescence_panel.py +++ b/src/aare/gui/panels/fluorescence_panel.py @@ -2,7 +2,7 @@ import numpy as np from aarecommon.config.logger import setup_logger from aarecommon.models.models import DAQStatusModel, FluorescenceSpectrumOutputModel from PySide6.QtCharts import QChart, QChartView, QLineSeries, QValueAxis -from PySide6.QtCore import QEvent, QPointF, Qt, Slot +from PySide6.QtCore import QEvent, Qt, Slot from PySide6.QtGui import QPainter, QPen from PySide6.QtWidgets import QGraphicsSimpleTextItem, QGridLayout, QLabel, QWidget @@ -82,8 +82,7 @@ class FluorescencePanel(QWidget): def eventFilter(self, obj, event): try: if obj is self.chart_view.viewport() and event.type() == QEvent.Type.MouseMove: - pos = event.position() if hasattr(event, "position") else event.pos() - p = QPointF(pos.x(), pos.y()) + p = event.position() plot = self.chart.plotArea() if not plot.contains(p) or self.series.count() == 0: self.chart_view.setToolTip("") @@ -94,19 +93,12 @@ class FluorescencePanel(QWidget): (p.x() - plot.left()) / plot.width() ) - # Snap to the largest Y within +/- 3 indices around nearest index + # Snap to the largest Y within +/- 10 indices around nearest index center = self._nearest_index(x_val) n = self.series.count() left = max(0, center - 10) right = min(n - 1, center + 10) - - best_i = left - best_y = self.series.at(best_i).y() - for i in range(left + 1, right + 1): - yi = self.series.at(i).y() - if yi > best_y: - best_y = yi - best_i = i + best_i = max(range(left, right + 1), key=lambda i: self.series.at(i).y()) pt = self.series.at(best_i) self.chart_view.setToolTip(f"Energy {pt.x():.3f} keV counts {pt.y():.3f}") diff --git a/src/aare/gui/panels/portrait_mode.py b/src/aare/gui/panels/portrait_mode.py index d65231df..50db0af5 100644 --- a/src/aare/gui/panels/portrait_mode.py +++ b/src/aare/gui/panels/portrait_mode.py @@ -154,7 +154,6 @@ class LEDStages(QWidget): class PlayPauseButton(QPushButton): def __init__(self, parent=None): super().__init__(parent) - self._hovered = False self._running = False self.setFixedSize(64, 64) self.setMouseTracking(True) @@ -163,14 +162,6 @@ class PlayPauseButton(QPushButton): self._running = running self.update() - def enterEvent(self, event): - self._hovered = True - self.update() - - def leaveEvent(self, event): - self._hovered = False - self.update() - def paintEvent(self, event): p = QPainter(self) p.setRenderHint(QPainter.Antialiasing) @@ -178,7 +169,10 @@ class PlayPauseButton(QPushButton): cx, cy = rect.width() / 2, rect.height() / 2 r = min(rect.width(), rect.height()) / 2 - 2 - bg_color = qcolor(WHITE) if self._hovered else QColor(ACCENT) + # underMouse() instead of enter/leave overrides tracking a _hovered + # flag: QPushButton already repaints on hover (WA_Hover), so the + # two extra Qt→Python callbacks bought nothing. + bg_color = qcolor(WHITE) if self.underMouse() else QColor(ACCENT) p.setBrush(bg_color) p.setPen(Qt.NoPen) p.drawEllipse(QPointF(cx, cy), r, r) diff --git a/src/aare/gui/widgets/baton_request_dialog.py b/src/aare/gui/widgets/baton_request_dialog.py index 7935145b..4943bab9 100644 --- a/src/aare/gui/widgets/baton_request_dialog.py +++ b/src/aare/gui/widgets/baton_request_dialog.py @@ -180,6 +180,9 @@ class BatonRequestDialog(QDialog): self._timer = QTimer(self) self._timer.setInterval(1000) self._timer.timeout.connect(self._tick) + # finished fires on accept/reject/close alike — replaces the + # closeEvent override that existed only to stop this timer. + self.finished.connect(self._timer.stop) self._timer.start() def _tick(self): @@ -221,22 +224,13 @@ class BatonRequestDialog(QDialog): self._on_accept() def _on_accept(self): - self._timer.stop() self.accepted_signal.emit() self.accept() def _on_refuse(self): - self._timer.stop() self.refused_signal.emit() self.reject() - def closeEvent(self, event): - """Closing the dialog counts as ignoring = auto-accept on timeout.""" - # Don't emit anything here - let the timeout handle it - # or the SSE stream will close the dialog when resolved - self._timer.stop() - super().closeEvent(event) - class BatonPendingDialog(QDialog): """ @@ -331,6 +325,9 @@ class BatonPendingDialog(QDialog): self._timer = QTimer(self) self._timer.setInterval(1000) self._timer.timeout.connect(self._tick) + # finished fires on accept/reject/close alike — replaces the + # closeEvent override that existed only to stop this timer. + self.finished.connect(self._timer.stop) self._timer.start() def _tick(self): @@ -364,10 +361,5 @@ class BatonPendingDialog(QDialog): # Keep cancel button so they can abort the wait if they change their mind def _on_cancel(self): - self._timer.stop() self.cancelled_signal.emit() self.reject() - - def closeEvent(self, event): - self._timer.stop() - super().closeEvent(event) diff --git a/src/aare/gui/widgets/camera_image.py b/src/aare/gui/widgets/camera_image.py index f58f2528..763c5cdf 100644 --- a/src/aare/gui/widgets/camera_image.py +++ b/src/aare/gui/widgets/camera_image.py @@ -609,12 +609,6 @@ class SampleCameraImageLabel(QGraphicsView): if self._pending_load_pos is not None: self.load_image.emit(self._pending_load_pos) self._pending_load_pos = None - return - - if self._pending_load_pos is not None: - self.load_image.emit(self._pending_load_pos) - self._pending_load_pos = None - self.raster_timer.start(self.raster_timer_interval) def mouseReleaseEvent(self, event): if not self._camera_interaction_enabled(): diff --git a/src/aare/gui/widgets/motor_move_group.py b/src/aare/gui/widgets/motor_move_group.py index 2013d110..2248ad48 100644 --- a/src/aare/gui/widgets/motor_move_group.py +++ b/src/aare/gui/widgets/motor_move_group.py @@ -86,24 +86,22 @@ class MotorMoveGroup(QObject): self._set_state(name, "pending") def eventFilter(self, obj, event): - if event.type() == QEvent.Type.KeyPress and event.key() in ( - Qt.Key.Key_Return, - Qt.Key.Key_Enter, + if ( + event.type() == QEvent.Type.KeyPress + and event.key() in (Qt.Key.Key_Return, Qt.Key.Key_Enter) + and obj in self._boxes.values() ): - for box in self._boxes.values(): - if obj is box: - try: - value = float(box.text()) - except ValueError: - break - bottom = box.range_validator.bottom() - if value < bottom: - QToolTip.showText( - box.mapToGlobal(QPoint(0, box.height())), - f"Too small — minimum value: {box.to_string(bottom)}", - box, - ) - break + try: + value = float(obj.text()) + except ValueError: + return super().eventFilter(obj, event) + bottom = obj.range_validator.bottom() + if value < bottom: + QToolTip.showText( + obj.mapToGlobal(QPoint(0, obj.height())), + f"Too small — minimum value: {obj.to_string(bottom)}", + obj, + ) return super().eventFilter(obj, event) @Slot() diff --git a/src/aare/gui/widgets/value_label.py b/src/aare/gui/widgets/value_label.py index 05ffb7ed..ef0f90e5 100644 --- a/src/aare/gui/widgets/value_label.py +++ b/src/aare/gui/widgets/value_label.py @@ -1,9 +1,9 @@ -from PySide6.QtCore import Qt, Signal -from PySide6.QtWidgets import QLabel +from aare.gui.widgets.clickable_label import ClickableLabel -class ValueLabel(QLabel): - clicked = Signal() +class ValueLabel(ClickableLabel): + # clicked signal + left-click mousePressEvent inherited from + # ClickableLabel — this class only adds the "descr: value unit" text. def __init__(self, text: str, unit: str = "", parent=None): super().__init__(parent) @@ -17,9 +17,3 @@ class ValueLabel(QLabel): ) else: self.setText(f"{self._descr}: {s} {self._unit}") - - def mousePressEvent(self, event): - if event.button() == Qt.MouseButton.LeftButton: - self.clicked.emit() - else: - super().mousePressEvent(event) diff --git a/src/aare/gui/widgets/video_image.py b/src/aare/gui/widgets/video_image.py index 263433a5..a03cee45 100644 --- a/src/aare/gui/widgets/video_image.py +++ b/src/aare/gui/widgets/video_image.py @@ -1,5 +1,5 @@ from PySide6.QtCore import QRectF, Qt, Slot -from PySide6.QtGui import QImage, QPainter, QPixmap +from PySide6.QtGui import QImage, QKeySequence, QPainter, QPixmap, QShortcut from PySide6.QtWidgets import QGraphicsPixmapItem, QGraphicsScene, QGraphicsView from aare.gui.widgets.busy_overlay import BusyOverlayStyle, draw_busy_status_text @@ -34,6 +34,20 @@ class VideoGraphicsView(QGraphicsView): self._busy_overlay_style: BusyOverlayStyle | None = None + # QShortcut instead of a keyPressEvent override: one fewer Qt→Python + # callback on the render path, same focus behavior (WidgetShortcut = + # active only while the view has focus). + for key, slot in ( + (Qt.Key.Key_F, self.fit_to_view), + (Qt.Key.Key_R, self.reset_zoom), + (Qt.Key.Key_Plus, self.zoom_in), + (Qt.Key.Key_Equal, self.zoom_in), + (Qt.Key.Key_Minus, self.zoom_out), + ): + shortcut = QShortcut(QKeySequence(key), self) + shortcut.setContext(Qt.ShortcutContext.WidgetShortcut) + shortcut.activated.connect(slot) + @Slot(QImage) def update_frame(self, qt_image: QImage): """Update the video frame""" @@ -92,19 +106,6 @@ class VideoGraphicsView(QGraphicsView): # Normal scrolling super().wheelEvent(event) - def keyPressEvent(self, event): - """Handle keyboard shortcuts""" - if event.key() == Qt.Key.Key_F: - self.fit_to_view() - elif event.key() == Qt.Key.Key_R: - self.reset_zoom() - elif event.key() == Qt.Key.Key_Plus or event.key() == Qt.Key.Key_Equal: - self.zoom_in() - elif event.key() == Qt.Key.Key_Minus: - self.zoom_out() - else: - super().keyPressEvent(event) - def drawForeground(self, painter: QPainter, rect: QRectF): super().drawForeground(painter, rect) diff --git a/tests/unit/gui/test_qt_override_reduction.py b/tests/unit/gui/test_qt_override_reduction.py new file mode 100644 index 00000000..83934a53 --- /dev/null +++ b/tests/unit/gui/test_qt_override_reduction.py @@ -0,0 +1,103 @@ +"""Covers the code paths touched by the reduce-overwriting-qt-method +refactor: Qt-native replacements (signals, shortcuts, underMouse) for +virtual-method overrides, so the diff-coverage gate sees them executed.""" + +from PySide6.QtCore import QEvent, QPointF, Qt +from PySide6.QtGui import QMouseEvent + +from aare.gui.panels.fluorescence_panel import FluorescencePanel +from aare.gui.panels.portrait_mode import PlayPauseButton +from aare.gui.widgets.baton_request_dialog import BatonPendingDialog, BatonRequestDialog +from aare.gui.widgets.value_label import ValueLabel +from aare.gui.widgets.video_image import VideoGraphicsView + + +def test_baton_dialogs_stop_timer_via_finished(qtbot): + """finished.connect replaced the closeEvent overrides: the timer must + stop on accept, reject AND plain close — the path closeEvent used to + handle.""" + req = BatonRequestDialog("someone") + qtbot.addWidget(req) + assert req._timer.isActive() + req._on_accept() + assert not req._timer.isActive() + + req2 = BatonRequestDialog("someone") + qtbot.addWidget(req2) + req2._on_refuse() + assert not req2._timer.isActive() + + # close() only delivers a close event to a SHOWN dialog — same held for + # the old closeEvent override, so showing first keeps the test honest. + pend = BatonPendingDialog("user") + qtbot.addWidget(pend) + pend.show() + qtbot.waitExposed(pend) + assert pend._timer.isActive() + pend.close() + assert not pend._timer.isActive() + + +def test_fluorescence_hover_snaps_to_peak(qtbot): + """Drives a MouseMove through the viewport filter: event.position() + (the PyQt5-era hasattr fallback is gone) and the max(range) peak snap.""" + panel = FluorescencePanel() + qtbot.addWidget(panel) + panel.resize(500, 400) + panel.show() + qtbot.waitExposed(panel) + + panel.axis_x.setRange(0.0, 10.0) + panel.axis_y.setRange(0.0, 100.0) + for i in range(50): + panel.series.append(i * 0.2, 90.0 if i == 25 else 10.0) + + plot = panel.chart.plotArea() + pos = QPointF(plot.center()) + ev = QMouseEvent( + QEvent.Type.MouseMove, + pos, + panel.chart_view.viewport().mapToGlobal(pos.toPoint()), + Qt.MouseButton.NoButton, + Qt.MouseButton.NoButton, + Qt.KeyboardModifier.NoModifier, + ) + assert panel.eventFilter(panel.chart_view.viewport(), ev) is False + assert "keV" in panel.chart_view.toolTip() + + +def test_video_view_shortcuts_replace_keypress_override(qtbot): + view = VideoGraphicsView() + qtbot.addWidget(view) + view.show() + qtbot.waitExposed(view) + # WidgetShortcut context needs real focus; offscreen grants it only + # after the window is active. + view.activateWindow() + view.setFocus() + qtbot.waitUntil(view.hasFocus, timeout=2000) + + qtbot.keyClick(view, Qt.Key.Key_Plus) + assert view.zoom_factor > 1.0 + qtbot.keyClick(view, Qt.Key.Key_R) + assert view.zoom_factor == 1.0 + qtbot.keyClick(view, Qt.Key.Key_Minus) + assert view.zoom_factor < 1.0 + qtbot.keyClick(view, Qt.Key.Key_F) # fit_to_view: just must not raise + + +def test_play_pause_button_paints_without_hover_overrides(qtbot): + btn = PlayPauseButton() + qtbot.addWidget(btn) + btn.set_running(True) + # grab() forces a real paintEvent pass over the underMouse() branch + assert not btn.grab().isNull() + + +def test_value_label_inherits_click(qtbot): + label = ValueLabel("Energy", "keV") + qtbot.addWidget(label) + label.set_value("12.4") + assert "12.4" in label.text() + with qtbot.waitSignal(label.clicked, timeout=1000): + qtbot.mousePress(label, Qt.MouseButton.LeftButton)