fix: #678 refresh the engine configuration if settings change

mathPi commented 2 months ago

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[bot] commented 2 months ago

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

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](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

Tips

### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `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](https://docs.coderabbit.ai/guides/configure-coderabbit) 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](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.

jonathanperret commented 2 months ago

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

In preferences.py, right after the (blocking) call that opens the Preferences dialog, an image_resizer signal is (sometimes) emitted: https://github.com/AllYarnsAreBeautiful/ayab-desktop/blob/a03774e6a62410f3c4e57394d949b8b803860ac4/src/main/python/main/ayab/preferences.py#L194-L198
In ayab.py, there's a call to scene.refresh() after the (blocking) call to the above method: https://github.com/AllYarnsAreBeautiful/ayab-desktop/blob/a03774e6a62410f3c4e57394d949b8b803860ac4/src/main/python/main/ayab/ayab.py#L129-L131

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

mathPi commented 2 months ago

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

In preferences.py, right after the (blocking) call that opens the Preferences dialog, an image_resizer signal is (sometimes) emitted: https://github.com/AllYarnsAreBeautiful/ayab-desktop/blob/a03774e6a62410f3c4e57394d949b8b803860ac4/src/main/python/main/ayab/preferences.py#L194-L198

In ayab.py, there's a call to scene.refresh() after the (blocking) call to the above method: https://github.com/AllYarnsAreBeautiful/ayab-desktop/blob/a03774e6a62410f3c4e57394d949b8b803860ac4/src/main/python/main/ayab/ayab.py#L129-L131

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 commented 2 months ago

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 commented 2 months ago

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)