Align viewbox center with vacuum map center #433
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #433 +/- ##
==========================================
+ Coverage 85.71% 86.33% +0.61%
==========================================
Files 87 87
Lines 3262 3270 +8
Branches 530 532 +2
==========================================
+ Hits 2796 2823 +27
+ Misses 414 395 -19
Partials 52 52 ☔ View full report in Codecov by Sentry. |
|
Thanks for the PR. I like this more than your previous one :) |
|
Added a couple more tests. |
philek
force-pushed
the
feature/align_viewbox_to_map
branch
from
93748e5 to
7d3231b
Compare
This removes the need to adjust all the coordinates by the bounding box. It is enough to divide by 50.
philek
force-pushed
the
feature/align_viewbox_to_map
branch
from
7d3231b to
07ee8f3
Compare
|
Rebased on latest dev. |
|
I'm not familiar with Codecov. Does that failure require a re-run? |
|
As this PR is only needed for PiotrMachowski/lovelace-xiaomi-vacuum-map-card. What's the status there? Can the card used with these changes only, or does the card itself need changes? |
|
@coderabbitai review |
Actions PerformedReview triggered.
|
|
Warning Rate limit exceeded@edenhaus has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 22 minutes and 56 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe recent updates to the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant User
participant MapClient
participant ViewBoxFloat
participant Util
User->>MapClient: Request map visualization
MapClient->>ViewBoxFloat: Convert values to float
ViewBoxFloat-->>MapClient: Return float values
MapClient->>Util: Decompress data
Util-->>MapClient: Return decompressed data
MapClient->>User: Return SVG map visualization
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration 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
Out of diff range and nitpick comments (1)
deebot_client/map.py (1)
Line range hint
420-430: The function_get_svg_traces_pathneeds test coverage for the added line.- if len(self._map_data.trace_values) > 0: + if self._map_data.trace_values:Ensure this line is covered by unit tests to verify correct functionality.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- deebot_client/map.py (11 hunks)
- tests/test_map.py (8 hunks)
Additional Context Used
GitHub Check Runs (1)
codecov/patch failure (2)
deebot_client/map.py: [warning] 562-562: deebot_client/map.py#L562
Added line #L562 was not covered by tests
deebot_client/map.py: [warning] 615-615: deebot_client/map.py#L615
Added line #L615 was not covered by tests
Additional comments not posted (16)
tests/test_map.py (9)
56-56: Good simplification of the_calc_pointfunction test.
77-78: The test for_calc_point_in_viewboxcorrectly asserts the expected results.
177-183: The test forcompact_pathcorrectly verifies the SVG path compacting logic.
263-263: The test for_get_svg_subsetcorrectly handles different types of map subsets.
305-311: The test for_get_svg_positionseffectively checks the SVG element creation based on positions.
44-46: Ensure test coverage for edge cases in_calc_point.
61-65: Consider adding more diverse scenarios to_test_calc_point_in_viewbox_datato cover edge cases.
Line range hint
198-219: Consider adding more test cases to cover additional scenarios intest_points_to_svg_path.
144-170: Ensure that the mock fordecompress_7z_base64_datais correctly utilized in the test for_get_svg_traces_path.Verification successful
The mock for
decompress_7z_base64_datais correctly utilized in the test for_get_svg_traces_path. The method_update_trace_pointswithin theMapclass callsdecompress_7z_base64_data, and this method is invoked in the test functiontest_Map_svg_traces_path.
tests/test_map.py: The mock setup fordecompress_7z_base64_datais present in the test functiontest_Map_svg_traces_path.deebot_client/map.py: The method_update_trace_pointscallsdecompress_7z_base64_data.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct usage of the mock in `_get_svg_traces_path`. # Test: Search for the mock usage in the test. Expect: Correct setup and utilization of the mock. rg --type python $'decompress_7z_base64_data'Length of output: 75
Script:
#!/bin/bash # Description: Verify the correct usage of the mock in `_get_svg_traces_path`. # Test: Search for the mock usage in the test. Expect: Correct setup and utilization of the mock. rg --type py 'decompress_7z_base64_data'Length of output: 680
Script:
#!/bin/bash # Description: Verify the correct usage of the mock in `_get_svg_traces_path`. # Step 1: Search for the `_get_svg_traces_path` method implementation. ast-grep --lang python --pattern $'def _get_svg_traces_path($_) { $$$ }' --json # Step 2: Search for the usage of `decompress_7z_base64_data` within the `_get_svg_traces_path` method. rg '_get_svg_traces_path' --context 10 --type pyLength of output: 4049
Script:
#!/bin/bash # Description: Verify the correct usage of the mock in `_get_svg_traces_path`. # Step 1: Search for the `Map` class implementation. ast-grep --lang python --pattern $'class Map($_) { $$$ }' --json # Step 2: Search for the usage of `decompress_7z_base64_data` within the `Map` class methods. rg 'decompress_7z_base64_data' --context 10 --type pyLength of output: 9204
deebot_client/map.py (7)
219-221: The refactoring of_calc_pointsimplifies the function and improves readability.
224-234: The implementation of_calc_point_in_viewboxcorrectly handles the bounding of points within the specified viewbox.
266-276: The function_get_svg_positionseffectively translates positions into SVG elements.
537-549: The setup of the SVG map's viewBox and background image is well-implemented.
566-566: The placement of positions within the SVG map is correctly handled.
615-615: The update logic inupdate_pointsforMapPieceneeds test coverage for the added line.- if self._crc32 != old_crc32: + if self._crc32 != old_crc32 and self.in_use:Ensure this line is covered by unit tests to verify correct functionality.
557-562: Ensure that the subsets are correctly transformed into SVG elements.Verification successful
The
_get_svg_subsetfunction correctly transforms subsets into SVG elements by converting the subset coordinates into points and then creating either aPathor aPolygonbased on the number of points. This ensures that the subsets are properly represented as SVG elements.
- The function handles subsets with exactly two points by creating a
Pathwith specific stroke properties.- For subsets with more than two points, it creates a
Polygonwith appropriate fill and stroke properties.This confirms that the subsets are correctly transformed into SVG elements.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the correct transformation of subsets into SVG elements. # Test: Search for the transformation logic in the code. Expect: Correct handling and transformation logic. rg --type python $'_get_svg_subset'Length of output: 65
Script:
#!/bin/bash # Description: Verify the correct transformation of subsets into SVG elements. # Test: Search for the transformation logic in the code. Expect: Correct handling and transformation logic. rg '_get_svg_subset'Length of output: 380
Script:
#!/bin/bash # Description: Extract the definition of `_get_svg_subset` to verify its transformation logic. # Extract the function definition from the codebase. rg --multiline --multiline-dotall --context 10 'def _get_svg_subset' deebot_client/map.pyLength of output: 603
Script:
#!/bin/bash # Description: Extract the complete definition of `_get_svg_subset` to verify its transformation logic. # Extract the complete function definition from the codebase. rg --multiline --multiline-dotall --context 50 'def _get_svg_subset' deebot_client/map.pyLength of output: 3276
philek
force-pushed
the
feature/align_viewbox_to_map
branch
from
07ee8f3 to
4ce9449
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (2)
deebot_client/map.py (2)
20-20: Ensure consistent import organization.Organize imports in a consistent manner. Group and alphabetize them to improve readability.
Line range hint
388-420: Improve logging for trace updates.Enhance the logging in
_update_trace_pointsto include more detailed information about the trace points being processed, which could be useful for debugging.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- deebot_client/map.py (11 hunks)
- tests/test_map.py (7 hunks)
Additional Context Used
Path-based Instructions (1)
tests/test_map.py (1)
Pattern
tests/**.py: Use pytest as the test framework, not unittest.
Additional comments not posted (7)
tests/test_map.py (5)
65-71: Ensure test coverage for edge cases.Consider adding more test cases to cover edge cases and null values for the
test_calc_pointfunction to ensure robustness.
83-93: Good use of parameterized tests.The use of
pytest.mark.parametrizefortest_calc_point_in_viewboxis effective for testing multiple scenarios efficiently.
159-186: Mocking external dependencies correctly.The use of
patchto mockdecompress_7z_base64_dataintest_Map_svg_traces_pathis appropriate to isolate the test environment from external dependencies.
Line range hint
257-272: Clarify the expected behavior in test cases.The test
test_get_svg_subsetshould include comments or documentation to clarify the expected behavior, especially how the coordinates translate to SVG paths or polygons.
311-326: Validate the structure of SVG elements.In
test_get_svg_positions, consider adding assertions to validate the structure and attributes of the SVG elements being created, not just their count and type.deebot_client/map.py (2)
266-276: Validate SVG element creation.In
_get_svg_positions, ensure that the SVG elements created are correctly formatted and contain all necessary attributes for proper rendering.
537-566: Ensure accurate SVG map generation.Verify that the SVG map generated in
get_svg_mapaccurately represents the map data, especially the viewBox calculations and element placements.
In its current state it's very hard to calibrate the card correctly. With these changes it can be done quite easily by taking the min_x, min_y from the viewbox, even if done manually. I'm focusing on this part first. |
|
I tested your change successfully, and the map looks correct :) Waiting for the last tweaks so I can merge it |
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
deebot_client/map.py (1)
Line range hint
384-384: Define or importdecompress_7z_base64_data.+ from deebot_client.util import decompress_7z_base64_dataAlso applies to: 611-611
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- deebot_client/map.py (10 hunks)
- tests/test_map.py (7 hunks)
Files skipped from review as they are similar to previous changes (1)
- tests/test_map.py
Additional context used
Ruff
deebot_client/map.py
20-20:
deebot_client.utilimported but unused
384-384: Undefined name
decompress_7z_base64_data
611-611: Undefined name
decompress_7z_base64_data
Additional comments not posted (5)
deebot_client/map.py (5)
219-221: The logic for calculating points aligns well with the PR objectives.
266-270: The function effectively uses the viewbox to position SVG elements, supporting the PR's calibration simplification.
Line range hint
416-426: The function correctly handles the generation of SVG paths for trace points, including appropriate scaling.
533-562: The function effectively compiles the SVG map, aligning the viewbox with the map's center as intended in the PR.
20-20: Remove unused import.- from deebot_client import utilLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- deebot_client/map.py (9 hunks)
- tests/test_map.py (7 hunks)
Files not reviewed due to errors (1)
- deebot_client/map.py (no review received)
Additional context used
Path-based instructions (1)
tests/test_map.py (1)
Pattern
tests/**.py: Use pytest as the test framework, not unittest.
Learnings (1)
tests/test_map.py (1)
User: edenhaus PR: DeebotUniverse/client.py#433 File: tests/test_map.py:5-5 Timestamp: 2024-05-28T11:23:47.409Z Learning: Mocks from `unittest.mock` are allowed in the `tests/test_map.py` file as per user preference.
Additional comments not posted (5)
tests/test_map.py (5)
18-18: Ensure theScaleclass is used appropriately in the tests.Verification successful
The
Scaleclass is used in thetests/test_map.pyfile, specifically on lines 18 and 20. The usage appears to be appropriate within the context of the tests.
tests/test_map.py:
- Line 18:
Scale,- Line 20:
Scale(0.2, -0.2),Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `Scale` class in the tests. # Test: Search for the `Scale` class usage. Expect: Proper usage in context. rg --type python $'Scale'Length of output: 55
Script:
#!/bin/bash # Description: Verify the usage of the `Scale` class in the tests. # Test: Search for the `Scale` class usage. Expect: Proper usage in context. rg 'Scale' --glob '*.py'Length of output: 164
22-22: Ensure theViewBoxSpecclass is used appropriately in the tests.Verification successful
The
ViewBoxSpecclass is used appropriately in the tests, as evidenced by multiple instances intests/test_map.py.
tests/test_map.py: Multiple lines show the usage ofViewBoxSpec.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `ViewBoxSpec` class in the tests. # Test: Search for the `ViewBoxSpec` class usage. Expect: Proper usage in context. rg --type python $'ViewBoxSpec'Length of output: 61
Script:
#!/bin/bash # Description: Verify the usage of the `ViewBoxSpec` class in the tests. # Test: Search for the `ViewBoxSpec` class usage. Expect: Proper usage in context. rg --type py 'ViewBoxSpec'Length of output: 906
43-43: Ensure theViewBoxFloatclass is used appropriately in the tests.Verification successful
The
ViewBoxFloatclass is used appropriately in the tests and other parts of the codebase.
tests/test_map.py: The class is used in test cases to calculate points and get SVG positions.deebot_client/map.py: The class is defined and used in functions to handle view box calculations.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `ViewBoxFloat` class in the tests. # Test: Search for the `ViewBoxFloat` class usage. Expect: Proper usage in context. rg --type python $'ViewBoxFloat'Length of output: 62
Script:
#!/bin/bash # Description: Verify the usage of the `ViewBoxFloat` class in the tests. # Test: Search for the `ViewBoxFloat` class usage. Expect: Proper usage in context. rg --type py 'ViewBoxFloat'Length of output: 559
45-45: Ensure the_calc_point_in_viewboxfunction is used appropriately in the tests.Verification successful
Ensure the
_calc_point_in_viewboxfunction is used appropriately in the tests.
tests/test_map.pycontains multiple references to_calc_point_in_viewbox, including its usage in a test function.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `_calc_point_in_viewbox` function in the tests. # Test: Search for the `_calc_point_in_viewbox` function usage. Expect: Proper usage in context. rg --type python $'_calc_point_in_viewbox'Length of output: 72
Script:
#!/bin/bash # Description: Verify the usage of the `_calc_point_in_viewbox` function in the tests. # Test: Search for the `_calc_point_in_viewbox` function usage. Expect: Proper usage in context. rg --type py '_calc_point_in_viewbox'Length of output: 557
20-20: Ensure theUseclass is used appropriately in the tests.Verification successful
Ensure the
Useclass is used appropriately in the tests.
- The
Useclass is utilized intests/test_map.pyin various contexts, such as initializing instances with different parameters and being part of expected lists.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `Use` class in the tests. # Test: Search for the `Use` class usage. Expect: Proper usage in context. rg --type python $'Use'Length of output: 53
Script:
#!/bin/bash # Description: Verify the usage of the `Use` class in the tests. # Test: Search for the `Use` class usage. Expect: Proper usage in context. rg --type py 'Use'Length of output: 710
edenhaus
force-pushed
the
feature/align_viewbox_to_map
branch
from
cd1d35e to
6cca195
Compare
|
Thank you for finishing it off :) |
This is an alternative approach for supporting the Vacuum Map card calibration from DeebotUniverse/Deebot-4-Home-Assistant#321. This will make the calibration much simpler. Object coordinates inside the SVG are vacuum coordinates divided by 50 with y axis flipped. That information, combined with the viewbox, will be enough to translate the vacuum coordinates to a position on even a scaled image on the frontend.
This has the benefit over my previous PR of not changing the interface. The calibration points can be static and added outside of this library.
Summary by CodeRabbit
New Features
ViewBoxFloatclass.Bug Fixes
Tests
Refactor