fix(review): address Copilot review findings from PRs #1391, #1387

[yashhzd]

Summary

Follow-up PR addressing Copilot bot review findings from #1391 and #1387.

Changes

Polymorphism fix (8 files):

• Changed BaseWindow.center_window_on_screen(self.root) → self.center_window_on_screen(self.root) in all 8 window classes that inherit from BaseWindow
• Enables proper subclass overrides and makes instance-level patching/mocking work as expected

Locale-safe test assertions (PR #1391 finding):

• Test match= patterns now use repr(value) instead of _("translated message"), so they won't break under non-English locales
• Removed unused _() import from test_data_model_par_dict.py

Progress window test isolation (PR #1387 finding):

• Reset mocks after ProgressWindow.__init__() before calling update_progress_bar() so the test actually verifies the update path, not just init

New test coverage (PR #1387 finding):

• Added test_uses_rendered_size_when_window_is_mapped — verifies center_window_on_screen() uses winfo_width/height (rendered size) instead of winfo_reqwidth/height when the window is already mapped

Files changed

File	Change
8 frontend_tkinter_*.py files	BaseWindow.center_window_on_screen() → self.center_window_on_screen()
tests/test_data_model_par_dict.py	Locale-independent test assertions, removed unused import
tests/test_frontend_tkinter_progress_window.py	Mock reset for proper test isolation
tests/test_frontend_tkinter_base_window.py	New rendered-size test case

Signed-off-by: Yash Goel yashhzd@users.noreply.github.com

[Copilot]

Pull request overview

Follow-up changes to address prior Copilot review findings by improving Tkinter window-centering polymorphism and strengthening/isolating tests (locale-safe assertions and more precise centering behavior verification).

Changes:

• Switched window-centering calls from BaseWindow.center_window_on_screen(...) to self.center_window_on_screen(...) to enable subclass overrides/mocking.
• Updated tests to be locale-independent by matching on repr(value)-style fragments rather than translated strings.
• Improved progress window test isolation and added test coverage for mapped-window rendered-size centering.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
ardupilot_methodic_configurator/frontend_tkinter_template_overview.py	Uses instance dispatch for centering to support overrides/mocking.
ardupilot_methodic_configurator/frontend_tkinter_software_update.py	Uses instance dispatch for centering to support overrides/mocking.
ardupilot_methodic_configurator/frontend_tkinter_project_opener.py	Uses instance dispatch for centering to support overrides/mocking.
ardupilot_methodic_configurator/frontend_tkinter_project_creator.py	Uses instance dispatch for centering to support overrides/mocking.
ardupilot_methodic_configurator/frontend_tkinter_parameter_editor.py	Uses instance dispatch for centering to support overrides/mocking.
ardupilot_methodic_configurator/frontend_tkinter_flightcontroller_info.py	Uses instance dispatch for centering to support overrides/mocking.
ardupilot_methodic_configurator/frontend_tkinter_connection_selection.py	Uses instance dispatch for centering to support overrides/mocking.
ardupilot_methodic_configurator/frontend_tkinter_component_editor_base.py	Uses instance dispatch for centering to support overrides/mocking.
tests/test_data_model_par_dict.py	Makes exception message matching locale-safe and removes translation dependency.
tests/test_frontend_tkinter_progress_window.py	Resets mocks post-__init__ to verify update path behavior.
tests/test_frontend_tkinter_base_window.py	Adds coverage for using rendered size when window is mapped.

[Copilot]

assert_called_once() makes this test more brittle than the previous call_count >= 1 assertion. If update_progress_bar() legitimately re-centers more than once (e.g., due to internal layout/geometry adjustments), this will fail even though behavior is correct. Consider asserting the expected relationship (e.g., “screen centering was used at least once and parent-centering was never used”) by checking mock_center_screen.assert_called() or call_count >= 1, while keeping mock_center_parent.assert_not_called() for the key behavior.

[Copilot]

assert_called_once() makes this test more brittle than the previous call_count >= 1 assertion. If update_progress_bar() legitimately re-centers more than once (e.g., due to internal layout/geometry adjustments), this will fail even though behavior is correct. Consider asserting the expected relationship (e.g., “screen centering was used at least once and parent-centering was never used”) by checking mock_center_screen.assert_called() or call_count >= 1, while keeping mock_center_parent.assert_not_called() for the key behavior.

Suggested change

	mock_center_screen.assert_called_once()
	mock_center_screen.assert_called()

[Copilot]

This hardcoded regex is inconsistent with the next test’s approach (re.escape(repr(bad_value))) and is less systematic. For consistency and to avoid regex pitfalls if the matched value ever changes, prefer match=re.escape(repr("not_a_number")) (or a shared helper) here as well.

Suggested change

	with pytest.raises(SystemExit, match=r"'not_a_number'"):
	with pytest.raises(SystemExit, match=re.escape(repr("not_a_number"))):

[amilcarlucas]

Some of these are already fixed in #1394

[yashhzd]

Fixed both Copilot findings:

• assert_called_once() too brittle — switched to assert_called(), good point about internal re-centering.
• Inconsistent match pattern — now using re.escape(repr(...)) in both tests for consistency.

Pushed in 3b53294.

[amilcarlucas]

You could try to rebase this on master to see if something was missed there.