fix: #678 refresh the engine configuration if settings change

[mathPi]

To fix issue #678, we need to refresh the engine parameters as the new settings can impact those parameters.

Summary by CodeRabbit

• New Features

  • Introduced a new method for refreshing window preferences, enhancing user interface responsiveness.
  • Added a validation method that improves form handling and updates the main GUI upon user interaction.
  • Implemented a method to close dialogs while ensuring UI refresh, providing immediate feedback on preference changes.

• Improvements

  • Enhanced modularity in the GUI component initialization process for better maintainability and consistency of displayed information.

[coderabbitai]

Walkthrough

The changes introduce new methods for managing interface preferences and refreshing settings within the GUI components of the application. A refresh_window_preferences_change method is added to enable dynamic updates of preferences, while the engine's initialization logic is modified to enhance modularity by calling reload_settings(). Additionally, event handling in the preferences class is improved with form validation and GUI refresh capabilities.

Changes

File Path	Change Summary
src/main/python/main/ayab/ayab.py	New method refresh_window_preferences_change added; updated __set_prefs to include self.engine.reload_settings().
src/main/python/main/ayab/engine/engine.py	Modified __init__ method to call reload_settings() for setting the window title; reload_settings() method added.
src/main/python/main/ayab/preferences.py	Replaced connection of the enter button's click event to accept with __validate_form, which includes validation and refresh functionality; new done method added to close the dialog and refresh the GUI.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Preferences
    participant Ayab
    participant Engine

    User->>Preferences: Click enter
    Preferences->>Preferences: Call __validate_form
    Preferences->>Ayab: Invoke refresh_window_preferences_change
    Ayab->>Engine: Invoke reload_settings
    Engine->>Engine: Refresh configuration
    Engine->>Engine: Update window title
    Preferences->>Preferences: Accept form
    Preferences->>Preferences: Call done
    Preferences->>Ayab: Invoke refresh_window_preferences_change
    Ayab->>Engine: Invoke reload_settings
    Engine->>Engine: Refresh configuration
    Engine->>Engine: Update window title

Loading

---

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?

Share

• X
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai generate interesting stats about this repository and render them as a table.
  • @coderabbitai show all the console.log statements in this repository.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between a03774e and 9c390f4.

Files selected for processing (3)

• src/main/python/main/ayab/ayab.py (1 hunks)
• src/main/python/main/ayab/engine/engine.py (2 hunks)
• src/main/python/main/ayab/preferences.py (2 hunks)

Additional comments not posted (7)

src/main/python/main/ayab/ayab.py (2)

202-203: Approved: New method refresh_window_preferences_change added.

The method correctly delegates the task of refreshing settings to the engine object, maintaining separation of concerns.

---

202-203: Verify the existence and implementation of refresh_settings_change in the engine class.

Ensure that the method refresh_settings_change is correctly implemented in the engine class to handle the settings refresh appropriately.

Verification successful

---

Method refresh_settings_change is implemented in the Engine class.

The refresh_settings_change method exists and is implemented in the Engine class within src/main/python/main/ayab/engine/engine.py. The verification is successful.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the existence and correct implementation of `refresh_settings_change` in the `engine` class.

# Test: Search for the method definition. Expect: The method should exist and contain relevant logic for refreshing settings.
ast-grep --lang python --pattern $'class Engine {
  $$$
  refresh_settings_change($_) {
    $$$
  }
  $$$  
}'

Length of output: 456

---

Script:

#!/bin/bash
# Corrected script to verify the existence and implementation of `refresh_settings_change` in the `Engine` class.

# Search for the Engine class and the refresh_settings_change method in Python syntax.
ast-grep --lang python --pattern $'class Engine:
    $$$
    def refresh_settings_change(self):
        $$$
    $$$'

Length of output: 117

---

Script:

#!/bin/bash
# Search for any class definitions similar to Engine.
rg 'class\s+\w+' -A 2

# Search for any method definitions similar to refresh_settings_change.
rg 'def\s+\w+' -A 2

Length of output: 134062

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

73-73: Approved: Modular initialization in __init__.

The change to call refresh_settings_change during initialization is a good practice as it ensures that the engine starts with the latest settings.

---

93-95: Approved: New method refresh_settings_change added.

This method correctly encapsulates the logic for refreshing the configuration and updating the window title, enhancing modularity.

---

93-95: Verify the call pattern of refresh_settings_change.

Ensure that refresh_settings_change is called appropriately within the class to maintain consistency in the engine's configuration.

Verification successful

---

Call Pattern of refresh_settings_change Verified

The method refresh_settings_change is called appropriately within the codebase, both internally in the engine.py file and externally in ayab.py, ensuring that the engine's settings are refreshed consistently. No issues found with its usage pattern.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the call pattern of `refresh_settings_change` within the `engine` class.

# Test: Search for calls to the method. Expect: The method should be called in appropriate places to ensure settings are refreshed correctly.
rg --type python -A 5 $'refresh_settings_change'

Length of output: 78

---

Script:

#!/bin/bash
# Description: Verify the call pattern of `refresh_settings_change` within the `engine` class.

# Test: Search for calls to the method. Expect: The method should be called in appropriate places to ensure settings are refreshed correctly.
rg --type py -A 5 $'refresh_settings_change'

Length of output: 1027

src/main/python/main/ayab/preferences.py (2)

258-263: Approved: New method __validate_form added.

This method correctly encapsulates the logic for form validation and GUI refresh, improving the user experience.

---

258-263: Verify the call to refresh_window_preferences_change from __validate_form.

Ensure that refresh_window_preferences_change is called correctly from __validate_form to trigger the necessary GUI refresh.

Verification successful

---

Verification Successful: refresh_window_preferences_change is correctly called from __validate_form.

The method refresh_window_preferences_change is called within preferences.py, ensuring the necessary GUI refresh is triggered. The method is defined in ayab.py, confirming its implementation and accessibility.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the correct call to `refresh_window_preferences_change` from `__validate_form`.

# Test: Search for the method call within `__validate_form`. Expect: The method should be called correctly to ensure the GUI is refreshed.
rg --type python -A 5 $'refresh_window_preferences_change'

Length of output: 88

---

Script:

#!/bin/bash
# Description: Verify the correct call to `refresh_window_preferences_change` from `__validate_form`.

# Test: Search for the method call within `__validate_form`. Expect: The method should be called correctly to ensure the GUI is refreshed.
rg --type py -A 5 'refresh_window_preferences_change'

Length of output: 610

[coderabbitai]

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 9c390f4 and d18d60f.

Files selected for processing (1)

• src/main/python/main/ayab/preferences.py (1 hunks)

Additional comments not posted (1)

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

258-265: Review the implementation of the done method.

The done method correctly refreshes the GUI by calling refresh_window_preferences_change on the parent GUI object before closing the dialog. This ensures that any changes made in the preferences dialog are reflected immediately in the main interface, which aligns with the PR objectives.

However, the method's documentation could be enhanced to better explain its role in the context of the application's functionality. Additionally, the method's argument arg__1 is not described, and it's unclear from the context what its purpose is. It would be beneficial to either document this parameter or rename it to something more descriptive if it's used internally.

The method implementation is sound, but consider improving the documentation for clarity.

258   def done(self, arg__1):
259       """
260       Method called to close the dialog and ensure the GUI is updated with any changes.
261       This method triggers the necessary refresh in the main GUI to reflect changes made in the preferences dialog.
262       """
263       # call the main GUI to trigger the necessary refresh
264       parent: GuiMain = self.parent()
265       parent.refresh_window_preferences_change()
266       super().done(arg__1)

[jonathanperret]

There are already two different places (that I could find!) where the UI is refreshed following a preferences update:

1. In preferences.py, right after the (blocking) call that opens the Preferences dialog, an image_resizer signal is (sometimes) emitted:

   ayab-desktop/src/main/python/main/ayab/preferences.py

   Lines 194 to 198 in a03774e

   	def open_dialog(self) -> None:
   	machine_width = Machine(self.value("machine")).width
   	PrefsDialog(self.parent).exec()
   	if machine_width != Machine(self.value("machine")).width:
   	self.emit_image_resizer()

2. In ayab.py, there's a call to scene.refresh() after the (blocking) call to the above method:

   ayab-desktop/src/main/python/main/ayab/ayab.py

   Lines 129 to 131 in a03774e

   	def __set_prefs(self) -> None:
   	self.prefs.open_dialog()
   	self.scene.refresh()

I think it would be good not to add a third place to that list…

[mathPi]

There are already two different places (that I could find!) where the UI is refreshed following a preferences update:

1. In preferences.py, right after the (blocking) call that opens the Preferences dialog, an image_resizer signal is (sometimes) emitted:

   ayab-desktop/src/main/python/main/ayab/preferences.py

   Lines 194 to 198 in a03774e

   	def open_dialog(self) -> None:
   	machine_width = Machine(self.value("machine")).width
   	PrefsDialog(self.parent).exec()
   	if machine_width != Machine(self.value("machine")).width:
   	self.emit_image_resizer()

2. In ayab.py, there's a call to scene.refresh() after the (blocking) call to the above method:

   ayab-desktop/src/main/python/main/ayab/ayab.py

   Lines 129 to 131 in a03774e

   	def __set_prefs(self) -> None:
   	self.prefs.open_dialog()
   	self.scene.refresh()

I think it would be good not to add a third place to that list…

Ohh yes, the second one is perfect, I update the PR. Is it better now ?

[jonathanperret]

Ohh yes, the second one is perfect, I update the PR. Is it better now ?

Yes, that's definitely better (less code, yay). Although the Qt way would be to use a signal so that things are less coupled (e.g. Engine could listen to a preferences_changed signal), but that's not how the code is structured now unfortunately, so that's another change that will have to wait.

There is a change in behavior introduced by this though. The Engine.refresh() method is slightly misnamed as it actually resets (most) options to their default values. With the previous code, the Infinite repeat checkbox for example was not reset to the default when closing the Preferences dialog. This is probably not a big deal, just maybe a bit surprising.
In any case, the same options are also reset when opening a new image, which I find annoying by the way — sometimes you just want to reload the image that you've modified in an editor — so this new behavior wouldn't be so much out of line with the rest.

I have one last nit to pick with the name of that new method: I'm not sure refresh_settings_change makes sense as a phrase, but maybe I'll let a native English speaker give their opinion on that one. I'd have gone with reload_settings maybe?

Finally, thank you for taking my feedback into account but I don't have permission to approve PRs in this repo, you'll have to wait for a maintainer to drop by.

[dl1com]

I have one last nit to pick with the name of that new method: I'm not sure refresh_settings_change makes sense as a phrase, but maybe I'll let a native English speaker give their opinion on that one. I'd have gone with reload_settings maybe?

Waiting for clarification on this, then merge

(Conversations have to be closed as well)

[sonarqubecloud]

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: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d18d60f and bfc3ca7.

Files selected for processing (2)

• src/main/python/main/ayab/ayab.py (1 hunks)
• src/main/python/main/ayab/engine/engine.py (3 hunks)

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

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

Additional comments not posted (1)

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

132-132: Potential issue with resetting options and method naming.

The addition of self.engine.reload_settings() ensures that changes to preferences are reflected in the engine's settings. However, the Engine.refresh() method resets most options to their default values, which might be surprising for users. Consider renaming the method to reload_settings as suggested in the PR comments.

Ensure that the behavior of resetting options aligns with user expectations and does not cause unintended disruptions.

- self.engine.reload_settings()
+ self.engine.reload_settings()

Consider renaming the method to reload_settings for better clarity.