Fix "default knit side image" behavior

[jonathanperret]

Problem

Fixes #711.

Proposed solution

Instead of Scene modifying AyabImage's internal image, it now reverses the image (if requested) only at render time. Pattern does likewise when generating the pattern from the image.

This makes it easier to avoid getting confused with how many times the image has been reversed.

Summary by CodeRabbit

• New Features

  • Introduced an auto_mirror option for automatic image mirroring based on user preference.
  • Added a new method to control image reversal settings more flexibly.

• Bug Fixes

  • Removed outdated image reversal functionality that was no longer needed.

• Refactor

  • Streamlined image transformation handling by centralizing control over image reversal.
  • Updated signal handling for image reversal to improve clarity and functionality.

[coderabbitai]

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 79fc553 and 83822aa.

Files selected for processing (7)

• src/main/python/main/ayab/ayab.py (0 hunks)
• src/main/python/main/ayab/engine/options.py (4 hunks)
• src/main/python/main/ayab/engine/pattern.py (1 hunks)
• src/main/python/main/ayab/image.py (0 hunks)
• src/main/python/main/ayab/scene.py (3 hunks)
• src/main/python/main/ayab/signal_receiver.py (2 hunks)
• src/main/python/main/ayab/signal_sender.py (1 hunks)

Files not reviewed due to no reviewable changes (2)

• src/main/python/main/ayab/ayab.py
• src/main/python/main/ayab/image.py

Additional comments not posted (12)

src/main/python/main/ayab/signal_receiver.py (1)

56-56: Verify the impact of the signal signature change.

The change in the image_reverser signal signature to accept a boolean parameter is a good improvement. It allows the signal to directly control the image reversal state, potentially simplifying its usage.

Please ensure that all connected slots have been updated to handle the boolean parameter correctly. You can use the following script to search for the signal usage:

Verification successful

---

Signal signature change verified successfully.

The image_reverser signal now correctly accepts a boolean parameter, and the connected slot set_image_reversed handles it appropriately.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all connected slots handle the boolean parameter correctly.

# Test: Search for the signal connection. Expect: Matching slot signatures.
rg --type python -A 5 $'self.image_reverser.connect'

Length of output: 82

---

Script:

#!/bin/bash
# Description: Verify all connected slots handle the boolean parameter correctly.

# Test: Search for the signal connection. Expect: Matching slot signatures.
rg --type py -A 5 'self.image_reverser.connect'

Length of output: 478

---

Script:

#!/bin/bash
# Description: Locate the definition of 'set_image_reversed' to verify it accepts a boolean parameter.

# Test: Search for the method definition. Expect: Method signature includes a boolean parameter.
rg --type py 'def set_image_reversed'

Length of output: 132

src/main/python/main/ayab/signal_sender.py (1)

89-90: LGTM!

The change to the emit_image_reverser method signature to include the reversed parameter is consistent with the method's purpose and the signal it emits. This change allows the caller to specify the desired reversed state when emitting the signal, providing more control.

Please verify that all callers of this method have been updated to pass the reversed parameter. You can use the following script to search for the method usage:

Verification successful

---

Verification Successful!

All callers of emit_image_reverser have been updated to pass the reversed parameter.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify all callers of `emit_image_reverser` pass the `reversed` parameter.

# Test: Search for the method usage. Expect: All occurrences pass the `reversed` parameter.
rg --type python -A 5 $'emit_image_reverser'

Length of output: 74

---

Script:

#!/bin/bash
# Description: Verify all callers of `emit_image_reverser` pass the `reversed` parameter.

# Test: Search for the method usage in Python files. Expect: All occurrences pass the `reversed` parameter.
rg --type py -A 5 'emit_image_reverser\('

Length of output: 1418

src/main/python/main/ayab/scene.py (4)

26-26: LGTM!

The import of the QTransform class from PySide6.QtGui is appropriate for implementing image transformations using Qt's capabilities.

---

71-71: LGTM!

The introduction of the reverse_image attribute as a boolean flag to track the reversed state of the image is a clear and concise way to manage image mirroring.

---

77-83: LGTM!

The addition of the set_image_reversed method is a clean and effective way to provide external control over the reverse_image attribute. Calling the refresh method after updating the attribute ensures that the displayed image is updated accordingly.

---

106-107: LGTM!

The conditional application of the horizontal flip transformation based on the reverse_image attribute is a clean and effective way to handle image mirroring. Utilizing Qt's QTransform class for image transformation is a good practice and provides flexibility in image manipulation.

src/main/python/main/ayab/engine/pattern.py (1)

34-36: LGTM! The code segment correctly handles image mirroring based on the configuration.

The change introduces a conditional transposition of the input image based on the auto_mirror attribute of the config parameter. If config.auto_mirror is True, the image is flipped horizontally using image.transpose(Image.FLIP_LEFT_RIGHT). The resulting image (flipped or original) is then assigned to the __pattern attribute.

This change centralizes the image mirroring logic within the Pattern class initialization and ensures that the __pattern attribute consistently holds the correctly mirrored image based on the configuration. It aligns with the objective of automatically flipping the image when the "Default Knit Side Image" preference is enabled.

src/main/python/main/ayab/engine/options.py (5)

85-85: LGTM!

The addition of the auto_mirror attribute with a default value of False is a clean way to introduce this new feature without affecting existing functionality.

---

126-134: LGTM!

The __auto_mirror_changed method correctly updates the auto_mirror_icon based on the state of the checkbox and emits the image_reverser signal with the current state of the auto_mirror variable. This is the correct way to handle the change in the checkbox state and communicate it to other parts of the application.

---

190-190: LGTM!

Emitting the current state of auto_mirror instead of relying on the checkbox state directly is a good refactoring that improves the consistency and reliability of the code. This ensures that the state of auto_mirror is always in sync with the state of the checkbox.

---

Line range hint 192-208: LGTM!

The inclusion of the auto_mirror attribute in the dictionary representation returned by the as_dict method is correct. This ensures that the state of auto_mirror is included when the OptionsTab instance is serialized, which is necessary for consistency and completeness.

---

Line range hint 209-223: LGTM!

Reading the state of the auto_mirror_checkbox and updating the auto_mirror attribute in the read method is correct. This ensures that the state of auto_mirror is always in sync with the state of the checkbox when the configuration options are read from the UI elements.

[coderabbitai]

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 83822aa and d59bdf1.

Files selected for processing (8)

• src/main/python/main/ayab/ayab.py (0 hunks)
• src/main/python/main/ayab/engine/options.py (4 hunks)
• src/main/python/main/ayab/engine/pattern.py (1 hunks)
• src/main/python/main/ayab/image.py (0 hunks)
• src/main/python/main/ayab/scene.py (3 hunks)
• src/main/python/main/ayab/signal_receiver.py (2 hunks)
• src/main/python/main/ayab/signal_sender.py (1 hunks)
• src/main/python/main/ayab/tests/test_control.py (1 hunks)

Files not reviewed due to no reviewable changes (2)

• src/main/python/main/ayab/ayab.py
• src/main/python/main/ayab/image.py

Files skipped from review as they are similar to previous changes (5)

• src/main/python/main/ayab/engine/options.py
• src/main/python/main/ayab/engine/pattern.py
• src/main/python/main/ayab/scene.py
• src/main/python/main/ayab/signal_receiver.py
• src/main/python/main/ayab/signal_sender.py

Additional context used

Ruff

src/main/python/main/ayab/tests/test_control.py

48-48: Undefined name self

(F821)

---

48-48: Undefined name auto_mirror

(F821)

[sonarcloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[coderabbitai]

Actionable comments posted: 1

Outside diff range and nitpick comments (1)

src/main/python/main/ayab/scene.py (1)

79-83: LGTM: New method improves image reversal control

The set_image_reversed method centralizes control over the image's reversed state, aligning with the PR objectives. It updates the image_reversed flag and refreshes the display, which is a clean approach to handling image transformations.

Consider enhancing the method's docstring for clarity:

def set_image_reversed(self, image_reversed: bool) -> None:
    """
    Sets the image reversed flag and refreshes the display.

    Args:
        image_reversed (bool): True to display the image reversed, False otherwise.
    """
    self.image_reversed = image_reversed
    self.refresh()

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d59bdf1 and c2b9c5d.

Files selected for processing (8)

• src/main/python/main/ayab/ayab.py (0 hunks)
• src/main/python/main/ayab/engine/options.py (4 hunks)
• src/main/python/main/ayab/engine/pattern.py (1 hunks)
• src/main/python/main/ayab/image.py (0 hunks)
• src/main/python/main/ayab/scene.py (3 hunks)
• src/main/python/main/ayab/signal_receiver.py (2 hunks)
• src/main/python/main/ayab/signal_sender.py (1 hunks)
• src/main/python/main/ayab/tests/test_control.py (1 hunks)

Files not reviewed due to no reviewable changes (2)

• src/main/python/main/ayab/ayab.py
• src/main/python/main/ayab/image.py

Files skipped from review as they are similar to previous changes (5)

• src/main/python/main/ayab/engine/options.py
• src/main/python/main/ayab/engine/pattern.py
• src/main/python/main/ayab/signal_receiver.py
• src/main/python/main/ayab/signal_sender.py
• src/main/python/main/ayab/tests/test_control.py

Additional comments not posted (3)

src/main/python/main/ayab/scene.py (3)

26-26: LGTM: Import changes align with new transformation approach

The addition of QTransform import and removal of the custom Transform import indicate a shift towards using Qt's built-in transformation capabilities. This change aligns well with the PR objectives of simplifying image handling.

---

71-71: LGTM: Initialization changes improve image state management

The addition of self.image_reversed = False introduces a flag to track the image's reversed state, which aligns with the PR objectives. Calling self.refresh() at the end of initialization ensures that the scene is properly set up with the initial state. These changes contribute to a more robust image handling system.

Also applies to: 77-77

---

Line range hint 1-214: Summary: Significant improvements in image reversal handling

The changes in scene.py successfully address the PR objectives related to fixing the "Default Knit Side Image" behavior. Key improvements include:

1. Introduction of the image_reversed flag for tracking the image state.
2. New set_image_reversed method for centralized control of image reversal.
3. Simplified image transformation using Qt's QTransform in the refresh method.

These changes result in a more robust, flexible, and maintainable implementation of image reversal. The new approach should resolve the issues reported in #711, ensuring that images are correctly displayed based on the "Default Knit Side Image" preference.

To ensure the changes are correctly implemented across the codebase, please run the following verification script:

This script will help ensure that the new image reversal approach is consistently applied throughout the codebase and that there are no remnants of the old implementation.