Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug fix]: Auto trigger some new actions #486

Merged
merged 5 commits into from
Jul 30, 2024
Merged

Conversation

PairZhu
Copy link
Contributor

@PairZhu PairZhu commented Jul 1, 2024

The "show_labels" and "show_texts" configuration items do not load the local configuration correctly when they are first opened, and the corresponding slot functions have some unnecessary code operations.

@PairZhu PairZhu force-pushed the fix/actions branch 4 times, most recently from 68fe930 to 1993b72 Compare July 1, 2024 09:37
@PairZhu
Copy link
Contributor Author

PairZhu commented Jul 29, 2024

@CVHub520 Can you help review it sometime?

@CVHub520
Copy link
Owner

@CVHub520 Can you help review it sometime?

Hey there! @PairZhu,

Thank you for submitting this pull request! I appreciate your effort in adding the auto_trigger functionality to the new_action function.

However, I have a few concerns about automatically triggering actions upon creation. Typically, actions are triggered by user interactions or specific events within the application. Automatically executing slot functions might lead to unexpected behavior, especially if the application's state is not ready for the action to be performed.

Additionally, I wanted to let you know that our project is currently in an active development phase, with frequent updates and changes. This means that there’s a higher likelihood of merge conflicts arising from contributions at this time.

Given this, it might be more practical to wait until we’re closer to a release to consider integrating new features. This way, we can avoid the inconvenience of dealing with repeated merge conflicts and ensure that new features are integrated smoothly with the latest version of the codebase.

We truly appreciate your willingness to contribute and your patience with our development process. Once we’re ready for new feature integration, we’ll be more than happy to revisit your pull request and work with you to get it merged.

Thank you for your understanding and continued support of our project!

@PairZhu
Copy link
Contributor Author

PairZhu commented Jul 29, 2024

@CVHub520 Can you help review it sometime?

Hey there! @PairZhu,

Thank you for submitting this pull request! I appreciate your effort in adding the auto_trigger functionality to the new_action function.

However, I have a few concerns about automatically triggering actions upon creation. Typically, actions are triggered by user interactions or specific events within the application. Automatically executing slot functions might lead to unexpected behavior, especially if the application's state is not ready for the action to be performed.

Additionally, I wanted to let you know that our project is currently in an active development phase, with frequent updates and changes. This means that there’s a higher likelihood of merge conflicts arising from contributions at this time.

Given this, it might be more practical to wait until we’re closer to a release to consider integrating new features. This way, we can avoid the inconvenience of dealing with repeated merge conflicts and ensure that new features are integrated smoothly with the latest version of the codebase.

We truly appreciate your willingness to contribute and your patience with our development process. Once we’re ready for new feature integration, we’ll be more than happy to revisit your pull request and work with you to get it merged.

Thank you for your understanding and continued support of our project!

This PR is actually intended to fix a bug, not to add new features. Currently, some configurations (such as show_labels) are not loading correctly. There is no difference between automatically triggering and manually calling the slot function once; similar operations already exist in the original code. The newly added auto-trigger actions only require the canvas to complete initialization before execution, and they do not produce side effects. Since the canvas initialization occurs above the action definitions, this modification is safe.

@CVHub520
Copy link
Owner

Thank you for your patience and for the detailed explanation, @PairZhu. This PR looks very promising and I appreciate the effort you've put into addressing the bug with configuration loading.

I understand now that the auto_trigger functionality is intended to ensure proper initialization rather than adding new features. Given your clarification that the auto-trigger actions are safe and do not produce side effects, I'm more comfortable with the approach.

Would you mind resolving the code conflicts so that we can proceed with merging this PR? Once the conflicts are resolved, I'll be happy to merge it into the main codebase.

Thank you again for your contribution and for working through this with us! We're grateful for your support. 🚀

@CVHub520
Copy link
Owner

Hi @PairZhu,

I noticed that there is a conflict with the latest branch. Could you please pull the latest changes from the main branch and make the necessary modifications to resolve the conflict? If possible, I would appreciate it if you could submit the updated changes by today.

I will check the PR later today, and if everything looks good, I will proceed with merging it.

Thanks again for your contribution!

@CVHub520 CVHub520 added the enhancement New feature or request label Jul 30, 2024
@PairZhu
Copy link
Contributor Author

PairZhu commented Jul 30, 2024

self.actions.save_with_image_data.setChecked(enabled)

What does this line of code do? The Checked state should be switched automatically when the event is triggered @CVHub520

@CVHub520
Copy link
Owner

CVHub520 commented Jul 30, 2024

self.actions.save_with_image_data.setChecked(enabled)

What does this line of code do? The Checked state should be switched automatically when the event is triggered @CVHub520

The line of code you've posted:

self.actions.save_with_image_data.setChecked(enabled)

is intended to set the checked state of the "save_with_image_data" action based on the enabled parameter. This effectively triggers the operation to either save or not save the image data when the corresponding action is performed.

However, you are correct in pointing out that there seems to be a missing step. To ensure that the configuration is actually saved after modifying it, you should include the following line of code:

save_config(self._config)

This save_config function call is necessary to persist the changes to the configuration so that the "save with image data" setting is remembered across sessions.

Thanks for bringing this to our attention. If you add the save_config call after setting the checked state, it should work as expected.

@PairZhu
Copy link
Contributor Author

PairZhu commented Jul 30, 2024

self.actions.save_with_image_data.setChecked(enabled)

What does this line of code do? The Checked state should be switched automatically when the event is triggered @CVHub520

The line of code you've posted:

self.actions.save_with_image_data.setChecked(enabled)

is intended to set the checked state of the "save_with_image_data" action based on the enabled parameter. This effectively triggers the operation to either save or not save the image data when the corresponding action is performed.

However, you are correct in pointing out that there seems to be a missing step. To ensure that the configuration is actually saved after modifying it, you should include the following line of code:

save_config(self._config)

This save_config function call is necessary to persist the changes to the configuration so that the "save with image data" setting is remembered across sessions.

Thanks for bringing this to our attention. If you add the save_config call after setting the checked state, it should work as expected.

There is no need to save the configuration file once for all operations, I fixed the call to closeEvent and can write the configuration file when closing the application

Copy link
Owner

@CVHub520 CVHub520 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I've reviewed the changes and everything looks good. No issues found. I'll go ahead and merge this PR. Great work, and thanks for the contribution!

👍️ Approved and ready to merge.

anylabeling/views/labeling/label_widget.py Outdated Show resolved Hide resolved
@CVHub520 CVHub520 merged commit be78c41 into CVHub520:main Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants