Skip to content

fix(param editor): Fix asking to save changes multiple times. #984

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.

Sign up for GitHub

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

Merged
amilcarlucas merged 1 commit into from
Oct 26, 2025
Merged

fix(param editor): Fix asking to save changes multiple times. #984

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions ardupilot_methodic_configurator/configuration_manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1418,10 +1418,11 @@ def export_current_file(self, annotate_doc: bool) -> None:
# Update the filesystem's file_parameters to match what was saved
self._local_filesystem.file_parameters[self.current_file] = export_params

# Note: We don't need to clear tracking sets or dirty flags on domain model parameters
# because after export, the user always navigates to a different file, which triggers
# repopulate_configuration_step_parameters() that clears tracking sets and rebuilds
# self.parameters fresh.
self._added_parameters.clear()
self._deleted_parameters.clear()
# copy parameters new values to their _values_on_file
for param in self.current_step_parameters.values():
param.copy_new_value_to_file()

def open_documentation_in_browser(self, filename: str) -> None:
_blog_text, blog_url = self.get_documentation_text_and_url("blog", filename)
Expand Down
5 changes: 5 additions & 0 deletions ardupilot_methodic_configurator/data_model_ardupilot_parameter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,11 @@ def set_forced_or_derived_change_reason(self, change_reason: str) -> None:

self._change_reason = change_reason

def copy_new_value_to_file(self) -> None:
"""Copy the new value to the value on file, marking it as saved, resetting the is_dirty property."""
self._value_on_file = self._new_value
self._change_reason_on_file = self._change_reason


class BitmaskHelper:
"""Helper class for working with ArduPilot parameter bitmasks."""
Expand Down
10 changes: 5 additions & 5 deletions ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ def __create_parameter_area_widgets(self) -> None:

# Create a Scrollable parameter editor table
self.parameter_editor_table = ParameterEditorTable(self.main_frame, self.configuration_manager, self)
self.repopulate_parameter_table()
self.repopulate_parameter_table(regenerate_from_disk=True)
self.parameter_editor_table.pack(side="top", fill="both", expand=True)

# Create a frame for the buttons
Expand Down Expand Up @@ -654,7 +654,7 @@ def on_param_file_combobox_change(self, _event: Union[None, tk.Event], forced: b
self.configuration_manager.current_file = selected_file
self.documentation_frame.refresh_documentation_labels()
self.documentation_frame.update_why_why_now_tooltip()
self.repopulate_parameter_table()
self.repopulate_parameter_table(regenerate_from_disk=True)
self._update_skip_button_state()

def _update_progress_bar_from_file(self, selected_file: str) -> None:
Expand All @@ -676,14 +676,14 @@ def download_flight_controller_parameters(self, redownload: bool = False) -> Non
if not redownload:
self.on_param_file_combobox_change(None, forced=True) # the initial param read will trigger a table update

def repopulate_parameter_table(self) -> None:
def repopulate_parameter_table(self, regenerate_from_disk: bool) -> None:
if not self.configuration_manager.current_file:
return # no file was yet selected, so skip it
# Re-populate the table with the new parameters
self.parameter_editor_table.repopulate(self.show_only_differences.get(), self.gui_complexity)
self.parameter_editor_table.repopulate(self.show_only_differences.get(), self.gui_complexity, regenerate_from_disk)

def on_show_only_changed_checkbox_change(self) -> None:
self.repopulate_parameter_table()
self.repopulate_parameter_table(regenerate_from_disk=False)

def on_upload_selected_click(self) -> None:
self.write_changes_to_intermediate_parameter_file()
Expand Down
17 changes: 9 additions & 8 deletions ardupilot_methodic_configurator/frontend_tkinter_parameter_editor_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def _create_headers_and_tooltips(self, show_upload_column: bool) -> tuple[tuple[
return tuple(base_headers), tuple(base_tooltips)

def repopulate( # pylint: disable=too-many-locals
self, show_only_differences: bool, gui_complexity: str
self, show_only_differences: bool, gui_complexity: str, regenerate_from_disk: bool
) -> None:
for widget in self.view_port.winfo_children():
widget.destroy()
Expand All @@ -150,12 +150,13 @@ def repopulate( # pylint: disable=too-many-locals
self.upload_checkbutton_var = {}

# Process configuration step and create domain model parameters
(ui_errors, ui_infos) = self.configuration_manager.repopulate_configuration_step_parameters()
if regenerate_from_disk:
(ui_errors, ui_infos) = self.configuration_manager.repopulate_configuration_step_parameters()

for title, msg in ui_errors:
messagebox.showerror(title, msg)
for title, msg in ui_infos:
messagebox.showinfo(title, msg)
for title, msg in ui_errors:
messagebox.showerror(title, msg)
for title, msg in ui_infos:
messagebox.showinfo(title, msg)

if show_only_differences:
# Filter to show only different parameters
Expand Down Expand Up @@ -718,7 +719,7 @@ def _on_parameter_delete(self, param_name: str) -> None:

# Delete the parameter
self.configuration_manager.delete_parameter_from_current_file(param_name)
self.parameter_editor.repopulate_parameter_table()
self.parameter_editor.repopulate_parameter_table(regenerate_from_disk=False)

# Restore the scroll position
self.canvas.yview_moveto(current_scroll_position)
Expand Down Expand Up @@ -769,7 +770,7 @@ def _confirm_parameter_addition(self, param_name: str) -> bool:
try:
if self.configuration_manager.add_parameter_to_current_file(param_name):
self._pending_scroll_to_bottom = True
self.parameter_editor.repopulate_parameter_table()
self.parameter_editor.repopulate_parameter_table(regenerate_from_disk=False)
return True
except InvalidParameterNameError as exc:
messagebox.showerror(_("Invalid parameter name."), str(exc))
Expand Down
26 changes: 15 additions & 11 deletions tests/test_frontend_tkinter_parameter_editor_table.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ def test_repopulate_empty_parameters(parameter_editor_table: ParameterEditorTabl
parameter_editor_table.configuration_manager.filesystem.file_parameters = ParDict({test_file: ParDict({})})

# Act: Repopulate the table
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple")
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple", regenerate_from_disk=False)

# Assert: No parameter rows were added
parameter_editor_table.add_parameter_row.assert_not_called()
Expand All @@ -296,7 +296,7 @@ def test_repopulate_clears_existing_content(parameter_editor_table: ParameterEdi
parameter_editor_table.configuration_manager.filesystem.param_default_dict = ParDict({"PARAM1": Par(0.0, "default")})

# Act: Repopulate the table
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple")
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple", regenerate_from_disk=False)

# Assert: Existing content was cleared
assert not dummy_widget.winfo_exists()
Expand All @@ -318,7 +318,7 @@ def test_repopulate_handles_none_current_file(parameter_editor_table: ParameterE
parameter_editor_table.configuration_manager.filesystem.param_default_dict = ParDict({})

# Act: Attempt to repopulate with no current file
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple")
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple", regenerate_from_disk=False)

# Assert: No parameter rows were added
parameter_editor_table.add_parameter_row.assert_not_called()
Expand All @@ -344,7 +344,7 @@ def test_repopulate_single_parameter(parameter_editor_table: ParameterEditorTabl

# Act: Repopulate with single parameter
with patch.object(parameter_editor_table, "grid_slaves", return_value=[]):
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple")
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple", regenerate_from_disk=False)

# Assert: Parameter row was added (implicitly tested through repopulate call)

Expand Down Expand Up @@ -387,7 +387,7 @@ def test_repopulate_multiple_parameters(parameter_editor_table: ParameterEditorT

# Act: Repopulate with multiple parameters
with patch.object(parameter_editor_table, "grid_slaves", return_value=[]):
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple")
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple", regenerate_from_disk=False)

# Assert: All parameters were processed (implicitly tested through repopulate call)

Expand Down Expand Up @@ -418,7 +418,7 @@ def test_repopulate_preserves_checkbutton_states(parameter_editor_table: Paramet
)

# Act: Repopulate the table
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple")
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple", regenerate_from_disk=False)

# Assert: Checkbutton states were preserved (implicitly tested through repopulate call)

Expand Down Expand Up @@ -459,7 +459,7 @@ def test_repopulate_show_only_differences(parameter_editor_table: ParameterEdito
)

# Act: Repopulate showing only differences
parameter_editor_table.repopulate(show_only_differences=True, gui_complexity="simple")
parameter_editor_table.repopulate(show_only_differences=True, gui_complexity="simple", regenerate_from_disk=False)

# Assert: Only differing parameters were processed (implicitly tested through repopulate call)

Expand All @@ -485,7 +485,7 @@ def test_repopulate_uses_scroll_helper(parameter_editor_table: ParameterEditorTa

# Act: Repopulate and check scroll behavior
with patch.object(parameter_editor_table, "_apply_scroll_position") as mock_scroll:
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple")
parameter_editor_table.repopulate(show_only_differences=False, gui_complexity="simple", regenerate_from_disk=False)

# Assert: Scroll position was applied correctly
mock_scroll.assert_called_once_with(pending_scroll)
Expand Down Expand Up @@ -918,7 +918,9 @@ def test_on_parameter_delete_confirmed(self, parameter_editor_table: ParameterEd

# Assert: Parameter is deleted and table repopulated
assert "TEST_PARAM" not in parameter_editor_table.configuration_manager.filesystem.file_parameters["test_file"]
parameter_editor_table.parameter_editor.repopulate_parameter_table.assert_called_once_with()
parameter_editor_table.parameter_editor.repopulate_parameter_table.assert_called_once_with(
regenerate_from_disk=False
)

def test_on_parameter_delete_cancelled(self, parameter_editor_table: ParameterEditorTable) -> None:
"""
Expand Down Expand Up @@ -1650,7 +1652,7 @@ def test_user_can_add_parameter_to_configuration_file(self, parameter_editor_tab
# Assert: Parameter addition was successful
assert result is True
parameter_editor_table.configuration_manager.add_parameter_to_current_file.assert_called_once_with("NEW_PARAM")
parameter_editor_table.parameter_editor.repopulate_parameter_table.assert_called_once()
parameter_editor_table.parameter_editor.repopulate_parameter_table.assert_called_once_with(regenerate_from_disk=False)

def test_user_can_delete_parameter_from_configuration_file(self, parameter_editor_table: ParameterEditorTable) -> None:
"""
Expand All @@ -1674,7 +1676,9 @@ def test_user_can_delete_parameter_from_configuration_file(self, parameter_edito
parameter_editor_table.configuration_manager.delete_parameter_from_current_file.assert_called_once_with(
"TEST_PARAM"
)
parameter_editor_table.parameter_editor.repopulate_parameter_table.assert_called_once()
parameter_editor_table.parameter_editor.repopulate_parameter_table.assert_called_once_with(
regenerate_from_disk=False
)

def test_user_cannot_delete_parameter_when_cancelled(self, parameter_editor_table: ParameterEditorTable) -> None:
"""
Expand Down