Skip to content

Conversation

@rdswift
Copy link
Collaborator

@rdswift rdswift commented Dec 29, 2024

Summary

  • This is a…
    • Bug fix
    • Feature addition
    • Refactoring
    • Minor / simple change (like a typo)
    • Other
  • Describe this change in 1-2 sentences: Adds a signal when file naming related settings are changed

Problem

File naming related settings can be changed in a number of places, and there is no consolidated way to capture whether a setting has been changed. Something like this is required for #2563

  • JIRA ticket (optional): PICARD-XXX

Solution

This PR adds a signal to the config settings class to determine when a file naming related setting has been updated.

Action

Additional actions required:

  • Update Picard documentation (please include a reference to this PR)
  • Other (please specify below)

@rdswift rdswift requested review from phw and zas December 29, 2024 21:18
@rdswift
Copy link
Collaborator Author

rdswift commented Dec 30, 2024

To make this signal more "generic", we could include a function in the SettingConfigSection class to provide the setting names set to check for triggering the signal. The default would be an empty set so that the signal would not be triggered. That way it could be used to signal on different settings as required for different applications. I can make that change if you think there might be some value in having this flexibility.

Copy link
Member

@phw phw left a comment

Choose a reason for hiding this comment

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

Generally this is a good approach. But I think we should avoid adding this very specific logic to the settings module. I'd instead have the settings emit a generic signal when settings have changed and have the update logic in e.g. the metadata box handle the specifics.

We have multiple places where we could make use of this. E.g. syncing the rename andove file settings with the menu and the script editor dialog.

@rdswift
Copy link
Collaborator Author

rdswift commented Dec 30, 2024

Generally this is a good approach. But I think we should avoid adding this very specific logic to the settings module. I'd instead have the settings emit a generic signal when settings have changed and have the update logic in e.g. the metadata box handle the specifics.

Yeah, that makes sense. I'll modify it to emit a signal when a setting value has changed, with the signal providing the name of the setting as an argument. If the new value for the setting is the same as the old value then the signal will not be emitted. This should avoid the situation of emitting signals for every setting when only one setting on an option page has changed (but all settings on the page are saved).

@rdswift rdswift force-pushed the rename_settings_changed_signal branch from 2b0ea4c to da47bbe Compare December 30, 2024 03:45
zas
zas previously approved these changes Dec 30, 2024
Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

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

Good move, it looks good to me.

phw
phw previously approved these changes Dec 30, 2024
Copy link
Member

@phw phw left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Just an idea, but no blocker: should the signal also emit the new value and old value of the setting? Not sure we have a use case for this, but it would avoid querying the settings again.

@rdswift
Copy link
Collaborator Author

rdswift commented Dec 30, 2024

Just an idea, but no blocker: should the signal also emit the new value and old value of the setting? Not sure we have a use case for this, but it would avoid querying the settings again.

That would make it more complete for sure. I'll have a look.

@rdswift rdswift dismissed stale reviews from phw and zas via 835f275 December 30, 2024 14:59
Copy link
Member

@phw phw left a comment

Choose a reason for hiding this comment

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

Great, thanks.

@phw phw requested a review from zas December 30, 2024 15:40
Copy link
Collaborator

@zas zas left a comment

Choose a reason for hiding this comment

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

Even better with old & new values passed to signal, LGTM.

@rdswift rdswift merged commit ccef1a1 into metabrainz:master Dec 31, 2024
48 checks passed
@rdswift rdswift deleted the rename_settings_changed_signal branch December 31, 2024 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants