Fix Frame Selection and Frame Count in GUI (Shift + Drag and Shift + Double Click) #2078
Conversation
…rning expected values
WalkthroughThe pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
participant User
participant Slider
participant App
User->>Slider: Interact with slider
Slider->>Slider: Transform mouse coordinates
Slider->>App: Update selection
App->>App: Calculate frame count
App->>User: Display status message
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
sleap/gui/widgets/slider.py (2)
372-381: Improve docstring and add edge case handling.The docstring improvement is good, but consider adding:
- Example usage
- Edge case handling for invalid inputs
Consider this implementation:
def _toVal(self, x: float, center=False) -> float: """ Converts x position to slider value. Args: x: x position on the slider. center: Whether to offset by half the width of the handle. Returns: Slider value (frame index). Example: >>> slider._toVal(100) # converts position 100 to frame index 50 # (assuming slider range 0-100 and width 200) Raises: ValueError: If x is None or not a number. """ + if x is None: + raise ValueError("x position cannot be None") + try: + val = float(x) + except (TypeError, ValueError): + raise ValueError("x position must be a number") val /= self._slider_width val *= max(1, self._val_max - self._val_min) val += self._val_min val = round(val) return val
392-392: LGTM! Consider adding validation.The simplified implementation correctly fixes the frame count issue by returning the full width. However, consider adding validation to ensure the width is always positive.
Consider this implementation:
@property def _slider_width(self) -> float: """Returns visual width of slider.""" width = self.box_rect.width() + if width <= 0: + return 1.0 # Prevent division by zero in _toVal return widthtests/gui/test_slider.py (1)
44-85: Enhance test coverage for _toVal.Good test coverage with parametrized tests. Consider adding:
- Tests for center=True parameter
- Tests for edge cases (None, non-numeric)
Add these test cases:
@pytest.mark.parametrize( "slider_width, x_value, min_value, max_value", [ # Existing test cases... + # Test center=True parameter + (1000, 500, 0, 1000), # Compare with center=True + + # Edge cases + (1000, 0, 0, 1000), # Minimum x position + (1000, 1000, 0, 1000), # Maximum x position ] ) def test_toVal(qtbot, slider_width, x_value, min_value, max_value): slider = VideoSlider(min=0, max=1000, val=15, marks=(10, 15)) slider.setMinimum(min_value) slider.setMaximum(max_value) slider.box_rect.setWidth(slider_width) # Test without center expected_value = round( (x_value / slider_width) * (max_value - min_value) + min_value ) assert slider._toVal(x_value) == expected_value + # Test with center=True + expected_center_value = round( + ((x_value - slider.handle.rect().width() / 2.0) / slider_width) + * (max_value - min_value) + min_value + ) + assert slider._toVal(x_value, center=True) == expected_center_value +@pytest.mark.parametrize( + "invalid_value", + [None, "invalid", [], {}] +) +def test_toVal_invalid_input(qtbot, invalid_value): + slider = VideoSlider(min=0, max=1000, val=15) + with pytest.raises(ValueError): + slider._toVal(invalid_value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
sleap/gui/app.py(1 hunks)sleap/gui/widgets/slider.py(2 hunks)tests/gui/test_slider.py(2 hunks)
🔇 Additional comments (2)
tests/gui/test_slider.py (1)
87-104: LGTM! Comprehensive test coverage.
The test coverage is thorough and includes:
- Various box_rect widths
- Edge cases (0 and large values)
sleap/gui/app.py (1)
1307-1307: LGTM! Frame count calculation fixed.
The removal of +1 from the frame count calculation correctly fixes the issue where Shift + Double Click was adding 1 to the frame count.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2078 +/- ##
===========================================
+ Coverage 75.43% 76.15% +0.71%
===========================================
Files 134 134
Lines 24749 24816 +67
===========================================
+ Hits 18670 18899 +229
+ Misses 6079 5917 -162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sleap/gui/widgets/slider.py (1)
375-377: Consider adding type hints to parameters.For improved code maintainability and IDE support, consider adding type hints to the parameters:
- x: x position on the slider. - center: Whether to offset by half the width of the handle. + x: float # x position on the slider + center: bool # Whether to offset by half the width of the handle
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sleap/gui/widgets/slider.py(2 hunks)
🔇 Additional comments (3)
sleap/gui/widgets/slider.py (3)
372-384: Improved documentation and error handling in _toVal method.The added docstring and None check enhance the robustness and maintainability of the code.
395-398: Fixed frame selection bug by removing handle width subtraction.The changes correctly address the frame count discrepancy issue mentioned in the PR objectives. The added check for non-positive width with a minimum return value of 1.0 prevents potential division by zero errors.
Line range hint
400-402: Well-designed property for accessing visible value range.The new property provides a clean interface to get the visible value range, which is useful for UI calculations and zoom operations.
these are good suggestions I have made the suggested changes and added a test for the center=true case. |
… into eric/shift-drag-fix
actually i found that center=True is called but the logic is not different for true or false, so i got rid of the input variable and got rid of the variable from the tests |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/gui/test_slider.py (3)
45-106: Add test case for zero width scenario.The test cases are comprehensive but missing a critical edge case where the slider width is zero. This would help verify the division by zero protection in the
_toValmethod.Add this test case to the parametrize decorator:
@pytest.mark.parametrize( "slider_width, x_value, handle_width, min_value, max_value", [ + (0, 0, 0, 0, 1000), # Zero width edge case # ---- Cases from test_toVal (center=False) ---- (1000, 500, 0, 0, 1000), # Midpoint w/o centering
126-126: Fix incorrect assertion message.The assertion message doesn't match the actual assertion. It states "Expected _slider_width to be 1" but the assertion checks for 0.
- ), "Expected _slider_width to be 1 when box_rect width is 0" + ), "Expected _slider_width to be 0 when box_rect width is 0"
157-158: Remove unnecessary loop.The loop with
range(1)is unnecessary as it only executes once.- # We use tqdm to track progress across multiple invalid inputs (optional). - for _ in range(1): - slider = VideoSlider(min=0, max=1000, val=15) + slider = VideoSlider(min=0, max=1000, val=15)sleap/gui/widgets/slider.py (3)
382-389: Improve error handling in_toValmethod.The error handling can be improved by:
- Using
raise ... from errto preserve the exception chain- Making the error messages more descriptive
if x is None: raise ValueError("x position cannot be None") # Force conversion to float here. try: val = float(x) except (TypeError, ValueError) as err: - raise ValueError(f"x position must be a number, got {x}") + raise ValueError(f"x position must be a number, got {x!r}") from err🧰 Tools
🪛 Ruff (0.8.2)
389-389: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
Line range hint
391-397: Consider adding input range validation.The method could benefit from validating that the input x position is within the valid range of the slider's width.
# Proceed with arithmetic only after we have a valid float. val = x + if not (0 <= val <= self._slider_width): + raise ValueError(f"x position {val} must be between 0 and {self._slider_width}") val /= self._slider_width val *= max(1, self._val_max - self._val_min) val += self._val_min val = round(val)🧰 Tools
🪛 Ruff (0.8.2)
389-389: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
568-570: Consider caching the start value.The
_toValcalculation forxis performed twice, once for storing and once for comparison. Consider caching the result.if getattr(self, "_zoom_start_val", None) is None: - self._zoom_start_val = self._toVal(x) + current_val = self._toVal(x) + self._zoom_start_val = current_val + else: + current_val = self._toVal(x) - current_val = self._toVal(x) self._draw_zoom_box(current_val, self._zoom_start_val)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sleap/gui/widgets/slider.py(5 hunks)tests/gui/test_slider.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
sleap/gui/widgets/slider.py
389-389: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (1)
sleap/gui/widgets/slider.py (1)
402-403: LGTM! Good simplification of_slider_width.The removal of handle width subtraction from
_slider_widthfixes the frame count issue by ensuring correct normalization in the_toValfunction.
ok i fixed the incorrect comment and got rid of the unnecessary loop |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
tests/gui/test_slider.py (4)
45-106: Comprehensive test for the_toValmethod, but docstring needs updating.The test thoroughly checks the
_toValmethod using a parameterized approach with various edge cases. However, the docstring mentions testing bothcenter=Trueandcenter=Falsescenarios (lines 60-61), but the actual implementation doesn't use acenterparameter when calling_toValon line 101.Update the docstring to reflect the current implementation:
""" Merged test that checks the slider value transformation for both: - 1) center=True (handle offset subtracted). - 2) center=False (no offset). + Various slider configurations with different widths and value ranges. Args: qtbot: The pytest-qt bot fixture. slider_width: The width of the slider box_rect in pixels. x_value: The x-coordinate on the slider to convert to a value. handle_width: The desired width of the slider handle rect (0 if center=False). min_value: The slider's minimum value. max_value: The slider's maximum value. - center: Whether _toVal() is called with center=True or not.Additionally, consider adding a test case for when
slider_widthis zero to ensure the division by zero protection in line 94 is functioning correctly.
134-164: Thorough validation testing for invalid inputs.This test appropriately verifies error handling for various invalid inputs to the
_toValmethod, ensuring that the correct error messages are raised.There's a comment on line 156 about using
tqdmto track progress, buttqdmisn't actually used in the implementation. Consider removing this comment to avoid confusion:""" Tests _toVal for invalid inputs to ensure ValueError is raised with the correct message. Args: qtbot: Pytest fixture for Qt applications. invalid_value: The invalid value for x. expected_error_msg: The exact error message expected. Returns: None """ - # We use tqdm to track progress across multiple invalid inputs (optional).
93-98: Consider adding parameter validation in test calculation.The test's manual implementation of the value calculation doesn't validate that
x_valueis a numeric value before performing arithmetic operations. While this is fine for a test with controlled inputs, adding validation would better mirror the behavior of the actual_toValmethod.Consider adding validation similar to the actual implementation:
# Manually compute the expected slider value. # If center=True, subtract half the handle width. - val = float(x_value) + try: + val = float(x_value) + except (TypeError, ValueError): + # Skip validation in test calculation since we're testing invalid inputs separately + val = float(x_value) # This will fail for non-numeric inputs, which is expected effective_width = max(1.0, slider_width) # Prevent zero-division val /= effective_width val *= max(1, max_value - min_value) val += min_value expected_val = round(val)
103-106: Improve assertion error message.The assertion error message includes
handle_widthbut doesn't include the expected and actual values, which would be helpful for debugging.Enhance the assertion error message for better debugging:
assert ( actual_val == expected_val - ), f"For x={x_value}, handle_width={actual_handle_width}, slider_width={slider_width}, " + ), f"For x={x_value}, handle_width={actual_handle_width}, slider_width={slider_width}, expected {expected_val} but got {actual_val}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/gui/test_slider.py(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
tests/gui/test_slider.py (2)
sleap/gui/widgets/slider.py (5)
box_rect(420-421)box_rect(424-430)_toVal(371-397)_slider_width(400-403)value(247-249)sleap/gui/widgets/video.py (1)
setRect(2188-2196)
🔇 Additional comments (1)
tests/gui/test_slider.py (1)
2-2: Good addition ofpytestimport.Adding the pytest import is necessary for using the parameterized test functionality introduced in the new test functions.
| def test_slider_width_property(qtbot): | ||
| """ | ||
| Test the _slider_width property to ensure it accurately reflects | ||
| the visual width of the slider's box_rect. | ||
| """ | ||
| slider = VideoSlider(min=0, max=1000, val=15, marks=(10, 15)) # Initialize slider | ||
|
|
||
| # Test various box_rect widths | ||
| for width in [800, 1000, 1200, 1500]: | ||
| slider.box_rect.setWidth(width) # Simulate setting the visual width | ||
| assert ( | ||
| slider._slider_width == width | ||
| ), f"Expected _slider_width to be {width}, but got {slider._slider_width}" | ||
|
|
||
| # Test edge cases with very small and large widths | ||
| slider.box_rect.setWidth(0) | ||
| assert ( | ||
| slider._slider_width == 0 | ||
| ), "Expected _slider_width to be 0 when box_rect width is 0" | ||
|
|
||
| slider.box_rect.setWidth(10000) | ||
| assert ( | ||
| slider._slider_width == 10000 | ||
| ), "Expected _slider_width to handle large values correctly" | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Good coverage of edge cases in _slider_width property test.
This test thoroughly validates the _slider_width property across various scenarios, including very small and large widths. The test cases appropriately verify that the property directly returns the width of the box_rect.
While the test allows for a slider width of 0, this could potentially cause division by zero in the _toVal method if not handled properly. I see that line 94 in test_toVal includes safeguarding against division by zero. Let's verify if this protection exists in the actual implementation:
🏁 Script executed:
#!/bin/bash
# Check if _toVal implementation guards against division by zero
rg -A 5 "_slider_width" sleap/gui/widgets/slider.py
rg "val /= " --context 5 sleap/gui/widgets/slider.pyLength of output: 1140
Attention: Potential Division-by-Zero in _toVal Implementation
The test for _slider_width in tests/gui/test_slider.py is solid and covers edge cases well. However, our verification of the _toVal method in sleap/gui/widgets/slider.py shows that it directly uses division by self._slider_width without any safeguard against a zero value. This is concerning because—even though the test allows a slider width of 0—the corresponding logic in _toVal has no protection and could lead to a runtime error.
Actionable items:
- Review the
_toValmethod: Add a conditional check to handle the case whenself._slider_widthis 0 before performing the division. - Align tests and implementation: If safeguarding already exists in the test (as referenced around line 94 in
test_toVal), ensure the implementation reflects that protection.
Description
This PR addresses Issue #2074 Shift + Drag Frame Selection and Frame Count Not Matching Video. Previously, if a whole video using the Shift + Drag frame selection, the max selected frame range and frame count did not match the number of frames in the video listed in the Videos table. Shift + Double Click also did not matched in the Selection display but added 1 to the frame count.
Shift + Drag was fixed by changing the _slider_width function. Previously, self.handle.rect().width() was being subtracted from self.box_rect.width() to calculate _slider_width. If we remove this subtraction, and just set it to self.box_rect.width() the frame count bug appears to be fixed.
But why would subtracting it lead to an increase in frame count? That is because of the normalization by slider width which occurs in the _toVal function val /= self._slider_width. So that explains why we were getting the bug in the first place.
The Shift + Double Click issue of adding 1 to frame count when everything was selected was fixed by removing the +1 from the frame count display in app.py.
I added tests for the _toVal function and the _slider_width function to match expected output avoid this problem in the future.
I successfully tested this fix on Windows and Mac M1 Pro.
Types of changes
Does this address any currently open issues?
#2074
Outside contributors checklist
Thank you for contributing to SLEAP!
❤️
Summary by CodeRabbit
Summary by CodeRabbit
New Features
VideoSliderfunctionality, including improved error handling and a new property for visible value range.Bug Fixes
VideoSliderto ensure correct visual representation.Tests
_toValmethod,_slider_widthproperty, and error handling in theVideoSliderclass, improving test coverage.