Same timer and lazy popup creation for all OS

[amilcarlucas]

Description

Concerns & Potential Issues ⚠️
Breaking Change Without Validation: The commit removes macOS-specific tooltip behavior entirely. Questions:

• Was this behavior tested and verified to work on macOS with lazy loading?
• Are there any macOS-specific Tcl/Tk issues that the original deferred creation was avoiding?
• The commit message doesn't explain why the platform-specific code was removed.
• Lost Methods Not Fully Retired: While show() and _do_hide() methods are removed, there's no evidence they weren't being called elsewhere in the codebase:

Did you search for all usages of these methods before removing them?
If they were public API, this is a breaking change.
Test Coverage Gaps:

• The test test_tooltip_initialization_macos was removed, but no new macOS-specific test replaced it
• Tests now only verify Linux behavior; macOS regression potential exists
• No performance benchmarks provided to validate the "fast loading" claim
• Timer Management Risk: The lazy loading relies on after() timers:

schedule_show() calls self.widget.after(TOOLTIP_SHOW_DELAY_MS, self.create_show)
What if the widget is destroyed before the timer fires? The _on_widget_destroy handler should catch this, but this edge case isn't tested.
Race Condition Possibility: In create_show(), the code checks if self.tooltip is None to avoid redundant creation, but:

Multiple rapid Enter/Leave events could queue multiple create_show calls
The timer cancellation logic may not fully prevent duplicate creations under edge cases
Incomplete Test Migration:

test_tooltip_do_hide_withdraws_on_non_macos was renamed to test_tooltip_force_hide_destroys_globally, but the semantics changed significantly
The old test verified non-macOS behavior; the new test claims to be cross-platform, but only tests the new behavior
Documentation:

Docstrings mention "on-demand load" but don't explain the performance implications
No migration guide for developers using the public Tooltip API
Critical Questions ❓

• Was this tested on actual macOS hardware/VMs, or just code review?
• Why was the TOOLTIP_HIDE_DELAY_MS removed? It controlled UX behavior on hide—was this intentional?
• The comment says "Catches protected member access error" but the error handling now includes tk.TclError—are these related?
• Performance claim: Do you have metrics showing parameter table loading improved?

Checklist

• Run pre-commit checks locally
• Verified by a human programmer
• All commits are signed off (use git commit --signoff)
• Code follows our coding standards
• Documentation updated if needed
• No breaking changes or properly documented

Testing

Describe how you tested these changes:

• Unit tests pass
• Integration tests pass
• Manual testing performed
• Tested on flight controller hardware

[Copilot]

Pull request overview

This PR updates the Tkinter Tooltip implementation to use delayed/on-demand creation (“lazy loading”) aimed at reducing UI overhead/flicker in dense/large tables, and adjusts the corresponding BDD tests.

Changes:

• Unify tooltip behavior across platforms by scheduling tooltip creation via after() and creating the Toplevel only when needed.
• Remove the old non-macOS eager-create + show/hide flow (including hide-delay-based behavior).
• Update tooltip-related tests to match the new lazy-loading and destroy-on-leave behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
ardupilot_methodic_configurator/frontend_tkinter_show.py	Refactors Tooltip to lazy-create and destroy tooltips across platforms; removes legacy hide/show flow.
tests/bdd_frontend_tkinter_show.py	Updates tooltip tests to assert lazy creation, new bindings, and destroy-based hiding.

[github-actions]

☂️ Python Coverage

current status: ✅

Overall Coverage

Lines	Covered	Coverage	Threshold	Status
12561	11842	94%	89%	🟢

New Files

No new covered files...

Modified Files

No covered modified files...

updated for commit: 1304eec by action🐍

[github-actions]

Test Results

     4 files  ± 0       4 suites  ±0   40m 49s ⏱️ -41s
 3 649 tests + 8   3 647 ✅ + 8   2 💤 ±0  0 ❌ ±0 
14 388 runs  +32  14 365 ✅ +33  23 💤  - 1  0 ❌ ±0 

Results for commit 1304eec. ± Comparison against base commit 7c0e6fd.

This pull request removes 2 and adds 10 tests. Note that renamed tests count towards both.

tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_cancel_hide
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_do_hide_withdraws_on_non_macos

tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_behavior_consistent_across_platforms
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_cancel_timer_removes_arbitrary_timer
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_force_hide_destroys_globally
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_force_hide_suppresses_tcl_error_when_already_destroyed
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_macos_alpha_animation_timer_canceled_on_destroy
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_on_widget_destroy_cancels_all_timers
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_pointer_check_prevents_creation_on_widget_exit
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_schedule_show_cancels_pending_show_timer
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_timer_cancellation_handles_already_fired_timers
tests.bdd_frontend_tkinter_show.TestTooltipFunctionality ‑ test_tooltip_widget_destruction_during_create_show

♻️ This comment has been updated with latest results.

[Copilot]

Pull request overview

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

[Copilot]

This changes the toplevel_class calling convention (previously toplevel_class(widget, bg=...), now only toplevel_class(widget) + configure). If external callers provided a custom toplevel_class that requires bg (or other kwargs) at construction time, this becomes a breaking change. A more compatible approach is to attempt construction with the previous kwargs and fall back to the simpler call on TypeError, while still keeping configure() for consistency.

Suggested change

	self.tooltip = cast("tk.Toplevel", self.toplevel_class(self.widget))
	try:
	self.tooltip = cast("tk.Toplevel", self.toplevel_class(self.widget, bg="#ffffe0"))
	except TypeError:
	self.tooltip = cast("tk.Toplevel", self.toplevel_class(self.widget))

[Copilot]

The MacWindowStyle call is attempted unconditionally for every platform, and the fallback unconditionally calls wm_attributes('-alpha', ...) / ('-topmost', ...). On non-macOS (or on Tk builds/WM configs that don't support these attributes), wm_attributes can raise tk.TclError and crash tooltip creation. Consider gating the MacWindowStyle block behind self._is_aqua (or at least platform_system() == 'Darwin'), and/or suppressing/handling tk.TclError around the wm_attributes fallback as well.

[Copilot]

Because this file lives under tests/ and matches pytest discovery naming (test_*.py), pytest will import it during collection even though it contains no tests. That can add unintended dependencies/import-time behavior to normal CI runs (and can fail in environments missing Tk). Consider moving it to a non-test location (e.g. benchmarks/ or scripts/) or renaming it so it’s not collected by default, and optionally integrate it via an explicit benchmark runner/marker.

Suggested change

	import logging
	import time
	from typing import NamedTuple
	from unittest.mock import MagicMock, patch
	from ardupilot_methodic_configurator.frontend_tkinter_show import MonitorBounds, Tooltip
	import importlib
	import logging
	import time
	from typing import NamedTuple
	from unittest.mock import MagicMock, patch
	class _LazyFrontendSymbol:
	"""Lazily resolve symbols from the Tk frontend module when first used."""
	def __init__(self, symbol_name: str) -> None:
	self._symbol_name = symbol_name
	def _resolve(self):
	module = importlib.import_module("ardupilot_methodic_configurator.frontend_tkinter_show")
	return getattr(module, self._symbol_name)
	def __call__(self, *args, **kwargs):
	return self._resolve()(*args, **kwargs)
	def __getattr__(self, item):
	return getattr(self._resolve(), item)
	MonitorBounds = _LazyFrontendSymbol("MonitorBounds")
	Tooltip = _LazyFrontendSymbol("Tooltip")

[Copilot]

The doc claims 'subsequent hovers instant', but the current implementation destroys the tooltip on every leave (destroy_hide() → force_hide() → destroy()), so each hover will recreate the Toplevel again. Please update this section to reflect the actual lifecycle (on-demand creation happens on each hover unless a reuse/cache strategy is implemented).

Suggested change

	- **Result**: First hover noticeable (~3-5ms delay), subsequent hovers instant
	- **Result**: Each hover pays the on-demand creation cost (~3-5ms total) because the tooltip is destroyed on leave; subsequent hovers are only instant if a reuse/cache strategy is implemented

[coveralls]

Coverage Report for CI Build 25103895234

Warning

No base build found for commit ae313db on master.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 94.375%

Details

• Patch coverage: No coverable lines changed in this PR.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →

---

Coverage Stats

Relevant Lines:	12568
Covered Lines:	11861
Line Coverage:	94.37%
Relevant Branches:	1881
Covered Branches:	1820
Branch Coverage:	96.76%
Branches in Coverage %:	No
Coverage Strength:	2.82 hits per line

---

💛 - Coveralls

[OmkarSarkar204]

I tested it and it was working Great but sometimes the FC loader is completely blank.
Everything else looks good.