Fix bottom display when pattern is near the edge #737
Conversation
WalkthroughThe pull request introduces changes across several classes related to knitting operations. The Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jonathanperret
force-pushed
the
fix-progress-display
branch
2 times, most recently
from
ff6b346 to
4501d1b
Compare
jonathanperret
force-pushed
the
fix-progress-display
branch
from
4501d1b to
120cf6c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/main/python/main/ayab/engine/control.py (1)
153-161: Architectural improvement: Better separation of concernsThe consolidation of display-related values (
machine_width,knit_start_needle,passes_per_row) into the Status class represents a significant improvement in code organization. This change:
- Reduces coupling between display logic and control logic
- Makes the code more maintainable by centralizing display-related state
- Simplifies the logic by removing the indirect
midlinecalculationThe new architecture will make it easier to handle future display-related changes.
src/main/python/main/ayab/knitprogress.py (2)
58-65: Consider making the display geometry configurableThe table widget has a fixed geometry (700x220) which might not be optimal for different screen sizes and resolutions. Consider making these dimensions configurable through preferences or responsive to the parent widget's size.
210-216: Use f-strings consistently for string formattingFor better readability and consistency, consider using f-strings instead of string concatenation.
- info_text = ( - info_text - + " " - + tr_("KnitProgress", "Color") - + " " - + status.color_symbol - ) + info_text = f'{info_text} {tr_("KnitProgress", "Color")} {status.color_symbol}'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/main/python/main/ayab/engine/control.py(1 hunks)src/main/python/main/ayab/engine/engine.py(1 hunks)src/main/python/main/ayab/engine/status.py(3 hunks)src/main/python/main/ayab/knitprogress.py(7 hunks)src/main/python/main/ayab/signal_receiver.py(1 hunks)src/main/python/main/ayab/signal_sender.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/python/main/ayab/engine/engine.py
- src/main/python/main/ayab/signal_receiver.py
- src/main/python/main/ayab/signal_sender.py
- src/main/python/main/ayab/engine/status.py
🔇 Additional comments (6)
src/main/python/main/ayab/engine/control.py (3)
153-154: LGTM: Machine width properly stored in status
This change appropriately moves the machine width into the Status class, supporting the consolidation of display-related values.
161-161: LGTM: Pass count properly stored in status
This change appropriately stores the passes per row in the Status class, supporting the improved display of pass numbers in multi-pass modes.
155-160: Core fix: Improved needle position calculation
The new logic correctly handles needle position calculations by:
- Using
pattern.knit_start_needlefor modes with flanking needles - Using
start_needledirectly for single-bed mode, which doesn't emit extra needles
This change directly fixes the blank display issue when edges are wider than the image.
Let's verify the impact of this change on the display calculations:
✅ Verification successful
Needle position calculation changes verified as correct
The codebase analysis confirms that the changes to knit_start_needle calculation are consistent with the overall needle position handling:
- In
pattern.py,knit_start_needleis properly managed as part of the pattern's core properties and is used for alignment calculations - In
knitprogress.py, the display logic correctly usesknit_start_needlefor needle position visualization - The special case for single-bed mode (using
start_needledirectly) aligns with the comment about not emitting extra needles
The changes maintain consistency with both the display system and the knitting engine's needle position calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of knit_start_needle and related display calculations
# Check for any other references to midline or related display calculations
rg -A 3 "knit_start_needle|start_needle|display.*needle" --type python
# Check for any potential edge cases in tests
rg -A 5 "test.*display.*edge|test.*needle.*position" --type python
Length of output: 203
Script:
#!/bin/bash
# Let's try without file type filtering since python extension wasn't recognized
# Check for any other references to knit_start_needle and start_needle
rg -A 3 "knit_start_needle|start_needle"
# Look for test files that might verify edge cases
fd "test.*\.py$" --exec rg -A 5 "test.*display.*edge|test.*needle.*position" {}
Length of output: 20470
src/main/python/main/ayab/knitprogress.py (3)
96-98: LGTM: Status comparison properly includes new attributes
The addition of knit_start_needle, machine_width, and passes_per_row to the status comparison ensures the display updates correctly when these critical values change.
203-206: LGTM: Pass display logic correctly handles single-bed mode
The pass information is now only displayed when passes_per_row > 1, which correctly implements the requirement to omit pass numbers in single-bed mode.
136-150: Verify the needle number calculation logic
The new logic for calculating needle positions uses knit_start_needle and machine_width instead of midline. This should fix the blank display issue when patterns are near the edge.
Let's verify this fixes the reported issue:
✅ Verification successful
Based on the search results, I can now generate the final response. The code shows consistent needle position calculation logic across the codebase:
Needle position calculation is correctly implemented
The new logic for calculating needle positions is consistent throughout the codebase:
knit_start_needlerepresents the absolute needle positionneedle_number_from_r1 = needle - machine_width // 2correctly calculates the relative position from the machine center (R1)- This calculation is used consistently in both the progress display and header labeling
- The logic properly handles both positive (green) and negative (orange) needle numbers
The implementation aligns with the machine's physical characteristics where:
- The machine center is at
machine_width // 2 - Left side needles (negative numbers) are displayed in orange
- Right side needles (positive numbers) are displayed in green
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of needle position calculations
# to ensure consistent logic across the codebase
rg -l "machine_width|knit_start_needle" | \
xargs ast-grep --pattern 'needle = $x + $y' -j
Length of output: 246
Script:
#!/bin/bash
# Let's search for needle position calculations with ripgrep
rg -A 5 -B 5 "knit_start_needle"
# Also search for machine_width usage
rg -A 5 -B 5 "machine_width"
Length of output: 29552
|
As this fixes not only #733 but also tweaks some more things, I'd love to get some feedback of testing. |
Agreed, more feedback is always welcome. @Adrienne200 did you get a chance to try this one out maybe? |
jonathanperret
force-pushed
the
fix-progress-display
branch
from
120cf6c to
522d1a6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/python/main/ayab/knitprogress.py (1)
232-242: Consider using integer division operatorWhile the current implementation using
floor()is correct, you could simplify it by using Python's integer division operator//for the HSL calculations:- floor(background_color.hslHue() * 0.85), - floor(background_color.hslSaturation() * 0.85), + background_color.hslHue() * 85 // 100, + background_color.hslSaturation() * 85 // 100,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/main/python/main/ayab/engine/control.py(1 hunks)src/main/python/main/ayab/engine/engine.py(1 hunks)src/main/python/main/ayab/engine/status.py(3 hunks)src/main/python/main/ayab/knitprogress.py(7 hunks)src/main/python/main/ayab/signal_receiver.py(1 hunks)src/main/python/main/ayab/signal_sender.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/python/main/ayab/engine/engine.py
- src/main/python/main/ayab/signal_receiver.py
- src/main/python/main/ayab/signal_sender.py
- src/main/python/main/ayab/engine/control.py
- src/main/python/main/ayab/engine/status.py
🧰 Additional context used
📓 Learnings (1)
src/main/python/main/ayab/knitprogress.py (1)
Learnt from: jonathanperret
PR: AllYarnsAreBeautiful/ayab-desktop#737
File: src/main/python/main/ayab/knitprogress.py:267-270
Timestamp: 2024-12-04T23:30:20.505Z
Learning: In the `onStitchSelect` method in `src/main/python/main/ayab/knitprogress.py`, avoid suggesting replacing magic numbers with named constants unless it fundamentally improves the logic, as the user prefers to leave them as is.
🔇 Additional comments (5)
src/main/python/main/ayab/knitprogress.py (5)
58-65: LGTM: Header configuration improvements
The header configuration changes improve the UI layout and interaction behavior by:
- Setting fixed resize mode for headers
- Disabling section clicks
- Using preferences for section size
96-98: LGTM: Enhanced state tracking for edge display fixes
Added tracking of knit_start_needle, machine_width, and passes_per_row to properly detect UI state changes, which is essential for fixing the edge display issues mentioned in PR #733.
136-150: LGTM: Improved needle position calculation
The new implementation correctly calculates needle positions using knit_start_needle and machine_width instead of midline, which fixes the edge display issues. The code is now more maintainable and logically clearer.
203-206: LGTM: Fixed pass number display logic
The pass number is now correctly displayed only when passes_per_row > 1, addressing the PR objective of fixing pass number display in single-bed and ribber modes.
267-270: LGTM: Color comparison logic
The color comparison logic is working as intended.
Values of `row_multiplier` and `midline` were passed along with `status` to UI update events, move them into `status`. Pass `knit_start_needle` and `machine_width` properties instead of `midline` in the hope of making needle number computations clearer.
dl1com
force-pushed
the
fix-progress-display
branch
from
522d1a6 to
3baa653
Compare
|
|
Reported working on Discord by @Adrienne200 . |
Problem
Fixes #733.
Proposed solution
The bug was caused by an edge case where the
midlinevalue was apparently wrong but unfortunately I could not really make sense of what that value was supposed to represent. Instead I changed the code to use the index of the first patterning needle, along with the machine width, to compute the displayed (orange/green) needle numbers.Along the way I simplified the logic to transmit the current status to the UI. Some values were transported as extra arguments alongside the
Statusobject; moving them into theStatusclass made the code more regular. I also removed a couple of parameters that were entirely unused.I also fixed what appeared to be a logic error that caused
Pass 1to be shown in row headers in single-bed mode (where each row always has a single pass), and never be shown on ribber modes (where multiple passes are required per pattern row) — interestingly this appears to be a regression of a once-fixed issue: #548. Now single-bed knitting shows no pass number, and multi-pass modes showPass x/n.For code review, a note on the changes in
knitprogress.py: most of the lines that are modified have no change in logic, but the configured style checker (flake8), which apparently had never been run on that file, required splitting a number of lines before I could commit the file. Sorry for the churn.How to test
Here's a test release built from the code in this PR: https://github.com/jonathanperret/ayab-desktop/releases/tag/1.0.0-knitprogress-1
Summary by CodeRabbit
Release Notes
New Features
machine_width,knit_start_needle, andpasses_per_row.Improvements
Control,KnitProgress, andSignalSenderclasses for better status management and code readability.SignalReceiverclass to enhance clarity.Bug Fixes