fix(ring_progress_bar): added hover mouse effect#1031
fix(ring_progress_bar): added hover mouse effect#1031wyzula-jan wants to merge 1 commit intopre_release_v3from
Conversation
wyzula-jan
force-pushed
the
feat/ring-progress-enhanced
branch
from
8f1e948 to
92713a1
Compare
There was a problem hiding this comment.
Pull request overview
This pull request adds hover effects to the ring progress bar widget, including visual animations (line width and radius expansion) and informative tooltips that display ring mode, values, and device information when hovering over individual rings.
Changes:
- Added mouse tracking and hover detection for individual rings within the progress bar container
- Implemented smooth hover animations with line width and radius expansion effects
- Added dynamic tooltips showing ring configuration details (mode, value, max_value, and device/signal information for device mode)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| bec_widgets/widgets/progress/ring_progress_bar/ring_progress_bar.py | Added mouse event handlers, ring detection logic at cursor position, hover state management, and tooltip building functionality |
| bec_widgets/widgets/progress/ring_progress_bar/ring.py | Implemented hover animation with QPropertyAnimation, modified paint event to render hover effects (line width and radius expansion), and added hover_progress property |
Note: The PR description references issue #123 which concerns stuck communication with BECFigure during automatic updates, which is unrelated to this hover functionality. Additionally, the PR title uses "fix" prefix, but this appears to be a new feature addition rather than a bug fix.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| self._hover_animation.setEasingCurve(easing_curve) | ||
| self.set_start_angle(self.config.start_position) | ||
|
|
There was a problem hiding this comment.
The animation object should be stopped and cleaned up when the Ring is destroyed to prevent potential memory leaks or animation issues. Consider adding self._hover_animation.stop() in the cleanup method.
| def cleanup(self) -> None: | |
| """ | |
| Clean up resources associated with this progress bar. | |
| Stops the hover animation and delegates to any base-class cleanup if available. | |
| """ | |
| parent_cleanup = getattr(super(), "cleanup", None) | |
| if callable(parent_cleanup): | |
| parent_cleanup() | |
| # Ensure the hover animation is stopped to prevent it from running on a destroyed object. | |
| self._hover_animation.stop() |
| pos = event.position() if hasattr(event, "position") else QPointF(event.pos()) | ||
| ring = self._ring_at_pos(pos) | ||
| self._set_hovered_ring(ring, event) | ||
| super().mouseMoveEvent(event) | ||
|
|
There was a problem hiding this comment.
The new hover functionality (mouse tracking, hover state management, and tooltip display) lacks test coverage. Given that the repository has comprehensive test coverage for ring_progress_bar functionality (as seen in tests/unit_tests/test_ring_progress_bar.py) and includes hover tests for other widgets (e.g., tests/unit_tests/test_macro_tree_widget.py::test_hover_events), consider adding tests that verify:
- Hover state transitions when mouse enters/leaves rings
- Tooltip content and display for different ring configurations
- Ring detection at various positions (_ring_at_pos logic)
- Animation behavior during hover state changes
- Value updates correctly updating tooltips while hovered
| self._hover_animation = QtCore.QPropertyAnimation(self, b"hover_progress") | ||
| self._hover_animation.setDuration(180) | ||
| easing_curve = ( | ||
| QtCore.QEasingCurve.Type.OutCubic | ||
| if hasattr(QtCore.QEasingCurve, "Type") | ||
| else QtCore.QEasingCurve.OutCubic | ||
| ) | ||
| self._hover_animation.setEasingCurve(easing_curve) |
There was a problem hiding this comment.
The easing curve fallback logic handles both old and new versions of Qt/qtpy. However, the property animation is targeting b"hover_progress" which is defined later as a QProperty. If this property definition is missing or incorrect, the animation will silently fail. Consider adding a check or assertion to verify that the property animation target is valid, or document this dependency clearly.
| from qtpy.QtWidgets import QHBoxLayout, QLabel, QToolTip, QVBoxLayout, QWidget | ||
|
|
There was a problem hiding this comment.
The imports from qtpy.QtWidgets are split across two lines (line 7 and line 8). While this works correctly, it would be more maintainable to combine these into a single import statement for consistency and readability, e.g., from qtpy.QtWidgets import QHBoxLayout, QLabel, QToolTip, QVBoxLayout, QWidget.
| if self.progress_container and self.progress_container.is_ring_hovered(self): | ||
| self.progress_container._update_hover_tooltip(self) |
There was a problem hiding this comment.
When the value changes, the tooltip is updated only if this ring is currently hovered. However, there's a potential race condition: if the hover state changes between checking is_ring_hovered and calling _update_hover_tooltip, the tooltip might not be updated correctly. Additionally, calling _update_hover_tooltip without the event parameter means it falls back to setToolTip, which may not display correctly when the mouse is already over the ring. Consider passing the last known mouse position or handling this case differently.
| inner = radius - half_width | ||
| outer = radius + half_width | ||
| if inner <= distance <= outer: | ||
| delta = abs(distance - radius) | ||
| if best_delta is None or delta < best_delta: | ||
| best_delta = delta | ||
| best_ring = ring | ||
|
|
There was a problem hiding this comment.
The ring detection logic uses ring.config.line_width to calculate the inner and outer boundaries. However, when hover is active, the actual line width is current_line_width = base_line_width + (hover_line_delta * self._hover_progress) (see ring.py line 439). This discrepancy means that when a ring is hovered and grows, the detection area doesn't grow with it, potentially causing the hover state to flicker or become unstable when the mouse is at the edge of the ring. Consider using the current actual line width in the detection calculation or accounting for the maximum possible hover expansion.
| if self._hover_progress > 0.0: | ||
| hover_radius_delta = 4.0 | ||
| base_radius = adjusted_rect.width() / 2 | ||
| if base_radius > 0: | ||
| target_radius = base_radius + (hover_radius_delta * self._hover_progress) | ||
| scale = target_radius / base_radius | ||
| center = adjusted_rect.center() | ||
| new_width = adjusted_rect.width() * scale | ||
| new_height = adjusted_rect.height() * scale | ||
| adjusted_rect = QtCore.QRectF( | ||
| center.x() - new_width / 2, center.y() - new_height / 2, new_width, new_height | ||
| ) |
There was a problem hiding this comment.
The hover effect increases both the line width and the radius of the ring. The radius expansion (hover_radius_delta = 4.0) could cause the hovered ring to overlap with adjacent rings, especially when rings are closely spaced. This might create visual artifacts or confusion about which ring is actually hovered. Consider either:
- Reducing the radius expansion to prevent overlap
- Accounting for the gap between rings when calculating the expansion
- Only expanding the line width without changing the radius
| if self._hover_progress > 0.0: | |
| hover_radius_delta = 4.0 | |
| base_radius = adjusted_rect.width() / 2 | |
| if base_radius > 0: | |
| target_radius = base_radius + (hover_radius_delta * self._hover_progress) | |
| scale = target_radius / base_radius | |
| center = adjusted_rect.center() | |
| new_width = adjusted_rect.width() * scale | |
| new_height = adjusted_rect.height() * scale | |
| adjusted_rect = QtCore.QRectF( | |
| center.x() - new_width / 2, center.y() - new_height / 2, new_width, new_height | |
| ) | |
| # Keep hover feedback limited to line width changes to avoid overlapping adjacent rings. | |
| # The adjusted_rect (and thus radius) is not scaled based on hover state. |
| mode, mode | ||
| ) | ||
|
|
||
| precision = int(ring.config.precision) |
There was a problem hiding this comment.
The tooltip formatting uses f"{ring.config.value:.{precision}f}" where precision comes from ring.config.precision. If precision is negative or excessively large, this could cause a ValueError or produce unexpected output. While this is likely validated elsewhere in the config, consider adding defensive bounds checking (e.g., precision = max(0, min(10, int(ring.config.precision)))) to ensure robust tooltip generation.
| precision = int(ring.config.precision) | |
| # Ensure precision is a safe, bounded integer to avoid formatting errors | |
| raw_precision = getattr(ring.config, "precision", 0) | |
| try: | |
| precision_int = int(raw_precision) | |
| except (TypeError, ValueError): | |
| precision_int = 0 | |
| precision = max(0, min(10, precision_int)) |
| for ring in self.rings: | ||
| ring.setGeometry(self.rect()) | ||
|
|
||
| def mouseMoveEvent(self, event): | ||
| pos = event.position() if hasattr(event, "position") else QPointF(event.pos()) | ||
| ring = self._ring_at_pos(pos) |
There was a problem hiding this comment.
The mouseMoveEvent is called frequently as the mouse moves, and each call triggers _ring_at_pos which iterates through all rings and performs floating-point calculations. For widgets with many rings, this could impact performance. Consider optimizing by:
- Caching ring geometries and only recalculating on resize
- Early exit when a ring is found (already implemented)
- Adding a distance threshold to avoid unnecessary calculations when the mouse is far from any ring
| for ring in self.rings: | |
| ring.setGeometry(self.rect()) | |
| def mouseMoveEvent(self, event): | |
| pos = event.position() if hasattr(event, "position") else QPointF(event.pos()) | |
| ring = self._ring_at_pos(pos) | |
| rect = self.rect() | |
| for ring in self.rings: | |
| ring.setGeometry(rect) | |
| # Cache approximate ring center and maximum radius for fast hover checks | |
| center = rect.center() | |
| self._ring_center = QPointF(center) | |
| # Use half of the smaller widget dimension as an outer radius approximation | |
| self._max_ring_radius = min(rect.width(), rect.height()) / 2.0 | |
| def mouseMoveEvent(self, event): | |
| pos = event.position() if hasattr(event, "position") else QPointF(event.pos()) | |
| ring = None | |
| center = getattr(self, "_ring_center", None) | |
| max_radius = getattr(self, "_max_ring_radius", None) | |
| if center is not None and max_radius is not None: | |
| dx = pos.x() - center.x() | |
| dy = pos.y() - center.y() | |
| # Add a small margin to the radius to avoid missing near-edge hovers | |
| outer_radius = max_radius + 5.0 | |
| if (dx * dx + dy * dy) <= (outer_radius * outer_radius): | |
| ring = self._ring_at_pos(pos) | |
| else: | |
| ring = self._ring_at_pos(pos) |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Description
[Provide a brief description of the changes introduced by this pull request.]
Related Issues
[Cite any related issues or feature requests that are addressed or resolved by this pull request. Link the associated issue, for example, with
fixes #123orcloses #123.]Type of Change
How to test
Potential side effects
[Describe any potential side effects or risks of merging this PR.]
Screenshots / GIFs (if applicable)
[Include any relevant screenshots or GIFs to showcase the changes made.]
Additional Comments
[Add any additional comments or information that may be helpful for reviewers.]
Definition of Done