Addressing TODO from https://github.com/10up/classifai/issues/260 ComputerVision::reset_settings() #264
Conversation
I looked the serialized array data in the database for what is there when the plugin is activated upon a fresh install and modelled the reset_settings() method off of that data and the includes/Classifai/Providers/Watson/NLU.php reset_settings() method. Not sure if there is more that you are looking for or if you would also like a button on this page that triggers an event to reset the settings. If there is something like that you are looking for please let me know :)
There was a problem hiding this comment.
The PR looks good. But I think we can go one step further. The settings are in two places which is a code smell to me. What do you think about creating a function that returns the default settings, then using that function in your function and setup_fields_sections?
Created a get_default_settings() method that returns the default settings, then used that function in reset_settings() method and setup_fields_sections() method
|
Hey @dinhtungdu I created a get_default_settings() method that returns the default settings, then used that function in reset_settings() method and setup_fields_sections() method as you requested. Let me know what you think 😄 |
There was a problem hiding this comment.
Hi @ActuallyConnor, thanks for the update. I still have some questions, can you please take another look?
| @@ -603,16 +623,18 @@ protected function generate_image_tags( $tags, $attachment_id ) { | |||
| */ | |||
| public function setup_fields_sections() { | |||
| add_settings_section( $this->get_option_name(), $this->provider_service_name, '', $this->get_option_name() ); | |||
| $settings = $this->get_default_settings(); | |||
There was a problem hiding this comment.
We can be more explicit here, $default_settings sounds better to me.
There was a problem hiding this comment.
Sounds good, I can implement this request.
| add_settings_field( | ||
| 'caption-threshold', | ||
| esc_html__( 'Caption Confidence Threshold', 'classifai' ), | ||
| [ $this, 'render_input' ], | ||
| $this->get_option_name(), | ||
| $this->get_option_name(), | ||
| [ | ||
| 'label_for' => 'caption_threshold', | ||
| 'input_type' => 'number', | ||
| 'default_value' => $settings['caption_threshold'], | ||
| 'description' => __( 'Minimum confidence score for automatically applied image captions, numeric value from 0-100. Recommended to be set to at least 75.', 'classifai' ), | ||
| ] | ||
| ); | ||
| add_settings_field( | ||
| 'image-tag-threshold', | ||
| esc_html__( 'Tag Confidence Threshold', 'classifai' ), | ||
| [ $this, 'render_input' ], | ||
| $this->get_option_name(), | ||
| $this->get_option_name(), | ||
| [ | ||
| 'label_for' => 'tag_threshold', | ||
| 'input_type' => 'number', | ||
| 'default_value' => $settings['tag_threshold'], | ||
| 'description' => __( 'Minimum confidence score for automatically applied image tags, numeric value from 0-100. Recommended to be set to at least 70.', 'classifai' ), | ||
| ] | ||
| ); | ||
| // What taxonomy should we tag images with? | ||
| $attachment_taxonomies = get_object_taxonomies( 'attachment', 'objects' ); | ||
| $options = []; | ||
| foreach ( $attachment_taxonomies as $name => $taxonomy ) { | ||
| $options[ $name ] = $taxonomy->label; | ||
| } | ||
| add_settings_field( | ||
| 'image-tag-taxonomy', | ||
| esc_html__( 'Tag Taxonomy', 'classifai' ), | ||
| [ $this, 'render_select' ], | ||
| $this->get_option_name(), | ||
| $this->get_option_name(), | ||
| [ | ||
| 'label_for' => 'image_tag_taxonomy', | ||
| 'options' => $options, | ||
| 'default_value' => $settings['image_tag_taxonomy'], | ||
| ] | ||
| ); |
There was a problem hiding this comment.
Could you explain why we should move this code here?
There was a problem hiding this comment.
I decided to reorder the settings fields in the function to match the order that appears in the default settings array. I ordered the default settings array based on the order that was returned in the database when I created a fresh install of the plugin.
I think it's best if the settings field order matches the order of the default settings field array, but I am not married to the current order of things (as I said, I just copied the order based on the serialized array that was returned in the database).
In my opinion we have two routes to choose from:
- Keep it as is since the settings fields order matches the default settings array order which matches the order of the serialized array.
- Revert the settings fields order back to the way it was, then match the default settings array order to the original settings fields order (this would just mean that we would ignore the database serialized array settings order which is fine as well since no one will really be looking at the database serialized array settings order).
Let me know what you would prefer, I am fine with going either route.
There was a problem hiding this comment.
Hi @ActuallyConnor, I'm sorry for this very late reply. I think we should keep the order of fields as they were before. The order of fields in an associated array doesn't affect the way we're using it. And by keeping the order of settings fields, our users won't be surprised after updating the plugin. Do you want to continue working on this one (and updating the default setting variable name)? If not, please let me know, I can handle the rest.
Description of the Change
Addressing TODO from #260
Alternate Designs
n/a
Benefits
Addressing issue related to resolving TODOs in the repository. Presumably this function will allow for a quick reset of the settings back to the default so that the user doesn't have to go through resetting settings manually.
Possible Drawbacks
none identified
Verification Process
I looked the serialized array data in the database for what is there when the plugin is activated upon a fresh install and modelled the reset_settings() method off of that data and the includes/Classifai/Providers/Watson/NLU.php reset_settings() method.
Not sure if there is more that you are looking for or if you would also like a button on this page that triggers an event to reset the settings. If there is something like that you are looking for please let me know :)
Checklist:
Applicable Issues
#260
Changelog Entry
Addressed TODO to implement ComputerVision::reset_settings() method