Add reverse checkbox to ServoFunctionEditorDialog #3444
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR adds a reverse-direction toggle to the ServoFunctionEditorDialog by introducing a new ParameterCheckbox component and integrating it into the dialog layout with a computed reverse_param binding. Sequence diagram for toggling the reverse direction checkboxsequenceDiagram
actor User
participant ServoFunctionEditorDialog
participant ParameterCheckbox
participant mavlink2rest
participant autopilot_data
User->>ServoFunctionEditorDialog: Open dialog
ServoFunctionEditorDialog->>ParameterCheckbox: Render with reverse_param
User->>ParameterCheckbox: Toggle checkbox
ParameterCheckbox->>ParameterCheckbox: onCheckboxChange(value)
ParameterCheckbox->>ParameterCheckbox: saveEditedParam()
alt param.rebootRequired
ParameterCheckbox->>autopilot_data: setRebootRequired(true)
end
ParameterCheckbox->>mavlink2rest: setParam(param.name, value, system_id, paramType)
mavlink2rest-->>ParameterCheckbox: Confirmation
ParameterCheckbox-->>User: Checkbox reflects new state
Class diagram for new ParameterCheckbox component and integrationclassDiagram
class ServoFunctionEditorDialog {
+Parameter reverse_param
}
class ParameterCheckbox {
+Parameter param
+string label
+number checkedValue
+number uncheckedValue
-number internal_value
-number last_sent_value
+onCheckboxChange(value: number): void
+saveEditedParam(): Promise<void>
}
ServoFunctionEditorDialog --> ParameterCheckbox : uses
ParameterCheckbox --> Parameter : param
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Williangalvani - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `core/frontend/src/components/parameter-editor/ParameterCheckbox.vue:63` </location>
<code_context>
+ }
+ return this.param?.value !== this.internal_value
+ },
+ param_value(): number {
+ return this.param?.value ?? this.uncheckedValue
+ },
+ },
</code_context>
<issue_to_address>
Defaulting 'param_value' to 'uncheckedValue' could mask issues.
Defaulting to 'uncheckedValue' when 'param' is undefined may hide errors. Consider returning undefined or throwing to surface issues during development.
Suggested implementation:
```
param_value(): number | undefined {
return this.param?.value
},
```
If you prefer to throw an error instead of returning `undefined`, replace the body of `param_value` with:
```js
if (!this.param) {
throw new Error("Parameter 'param' is undefined in ParameterCheckbox");
}
return this.param.value;
```
You may also need to update any code that uses `param_value` to handle the possibility of `undefined` or the thrown error.
</issue_to_address>
### Comment 2
<location> `core/frontend/src/components/parameter-editor/ParameterCheckbox.vue:68` </location>
<code_context>
+ },
+ },
+ watch: {
+ param(newParam) {
+ if (newParam) {
+ this.internal_value = newParam.value
+ }
+ },
</code_context>
<issue_to_address>
The watcher on 'param' may overwrite user input.
If 'param' changes during user interaction, 'internal_value' may be reset and overwrite user input. Consider updating 'internal_value' only for external changes, not user actions.
</issue_to_address>
### Comment 3
<location> `core/frontend/src/components/parameter-editor/ParameterCheckbox.vue:91` </location>
<code_context>
+ this.saveEditedParam()
+ },
+
+ async saveEditedParam(): Promise<void> {
+ if (this.param_value === this.internal_value) {
+ return
+ }
+ if (this.param === undefined || this.internal_value === undefined) {
+ return
+ }
+ if (this.param?.rebootRequired) {
+ autopilot_data.setRebootRequired(true)
+ }
+ this.last_sent_value = this.internal_value
+
+ await mavlink2rest.setParam(
+ this.param.name,
+ this.internal_value,
</code_context>
<issue_to_address>
No error handling for failed parameter save.
Add error handling for 'mavlink2rest.setParam' failures to provide user feedback or implement a retry mechanism.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
await mavlink2rest.setParam(
this.param.name,
this.internal_value,
autopilot_data.system_id,
this.param.paramType.type,
)
=======
try {
await mavlink2rest.setParam(
this.param.name,
this.internal_value,
autopilot_data.system_id,
this.param.paramType.type,
)
} catch (error) {
// Provide user feedback on failure
// Replace with your preferred notification system if available
// For now, use alert as a placeholder
alert('Failed to save parameter. Please try again.');
// Optionally, you could implement a retry mechanism here
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `core/frontend/src/components/parameter-editor/ServoFunctionEditorDialog.vue:20` </location>
<code_context>
- />
-
+ <v-row>
+ <v-col cols="6">
+ <inline-parameter-editor
+ :auto-set="true"
+ :label="param.name"
+ :param="param"
+ />
+ </v-col>
+ <v-col cols="6">
+ <parameter-checkbox
</code_context>
<issue_to_address>
Splitting the editor and checkbox into two columns may cause layout issues on small screens.
Consider applying Vuetify's grid breakpoints to stack these components vertically on small screens for better responsiveness.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
<v-col cols="6">
<inline-parameter-editor
:auto-set="true"
:label="param.name"
:param="param"
/>
</v-col>
<v-col cols="6">
<parameter-checkbox
:param="reverse_param"
label="Reverse Direction"
:checked-value="1"
:unchecked-value="0"
/>
</v-col>
=======
<v-col cols="12" sm="6">
<inline-parameter-editor
:auto-set="true"
:label="param.name"
:param="param"
/>
</v-col>
<v-col cols="12" sm="6">
<parameter-checkbox
:param="reverse_param"
label="Reverse Direction"
:checked-value="1"
:unchecked-value="0"
/>
</v-col>
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+63
to
+64
| param_value(): number { | ||
| return this.param?.value ?? this.uncheckedValue |
There was a problem hiding this comment.
suggestion (bug_risk): Defaulting 'param_value' to 'uncheckedValue' could mask issues.
Defaulting to 'uncheckedValue' when 'param' is undefined may hide errors. Consider returning undefined or throwing to surface issues during development.
Suggested implementation:
param_value(): number | undefined {
return this.param?.value
},
If you prefer to throw an error instead of returning undefined, replace the body of param_value with:
if (!this.param) {
throw new Error("Parameter 'param' is undefined in ParameterCheckbox");
}
return this.param.value;You may also need to update any code that uses param_value to handle the possibility of undefined or the thrown error.
Comment on lines
+68
to
+70
| param(newParam) { | ||
| if (newParam) { | ||
| this.internal_value = newParam.value |
There was a problem hiding this comment.
issue (bug_risk): The watcher on 'param' may overwrite user input.
If 'param' changes during user interaction, 'internal_value' may be reset and overwrite user input. Consider updating 'internal_value' only for external changes, not user actions.
Comment on lines
+103
to
+108
| await mavlink2rest.setParam( | ||
| this.param.name, | ||
| this.internal_value, | ||
| autopilot_data.system_id, | ||
| this.param.paramType.type, | ||
| ) |
There was a problem hiding this comment.
suggestion (bug_risk): No error handling for failed parameter save.
Add error handling for 'mavlink2rest.setParam' failures to provide user feedback or implement a retry mechanism.
Suggested change
| await mavlink2rest.setParam( | |
| this.param.name, | |
| this.internal_value, | |
| autopilot_data.system_id, | |
| this.param.paramType.type, | |
| ) | |
| try { | |
| await mavlink2rest.setParam( | |
| this.param.name, | |
| this.internal_value, | |
| autopilot_data.system_id, | |
| this.param.paramType.type, | |
| ) | |
| } catch (error) { | |
| // Provide user feedback on failure | |
| // Replace with your preferred notification system if available | |
| // For now, use alert as a placeholder | |
| alert('Failed to save parameter. Please try again.'); | |
| // Optionally, you could implement a retry mechanism here | |
| } |
Comment on lines
+20
to
+34
| <v-col cols="6"> | ||
| <inline-parameter-editor | ||
| :auto-set="true" | ||
| :label="param.name" | ||
| :param="param" | ||
| /> | ||
| </v-col> | ||
| <v-col cols="6"> | ||
| <parameter-checkbox | ||
| :param="reverse_param" | ||
| label="Reverse Direction" | ||
| :checked-value="1" | ||
| :unchecked-value="0" | ||
| /> | ||
| </v-col> |
There was a problem hiding this comment.
suggestion: Splitting the editor and checkbox into two columns may cause layout issues on small screens.
Consider applying Vuetify's grid breakpoints to stack these components vertically on small screens for better responsiveness.
Suggested change
| <v-col cols="6"> | |
| <inline-parameter-editor | |
| :auto-set="true" | |
| :label="param.name" | |
| :param="param" | |
| /> | |
| </v-col> | |
| <v-col cols="6"> | |
| <parameter-checkbox | |
| :param="reverse_param" | |
| label="Reverse Direction" | |
| :checked-value="1" | |
| :unchecked-value="0" | |
| /> | |
| </v-col> | |
| <v-col cols="12" sm="6"> | |
| <inline-parameter-editor | |
| :auto-set="true" | |
| :label="param.name" | |
| :param="param" | |
| /> | |
| </v-col> | |
| <v-col cols="12" sm="6"> | |
| <parameter-checkbox | |
| :param="reverse_param" | |
| label="Reverse Direction" | |
| :checked-value="1" | |
| :unchecked-value="0" | |
| /> | |
| </v-col> |
|
we need to be context-aware here. motors currently have their own reverse parameter, for example |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add a reverse direction toggle to the servo function editor and introduce a reusable ParameterCheckbox component for editing checkbox-style parameters.
New Features:
Enhancements: