You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Review by Claude (Opus 4.7), driven by the PR author's collaborator. Posted on their behalf.
What it does
Provides a macOS-only ManagedTreeCtrl backed by wx.dataview.TreeListCtrl (better VoiceOver support than wx.TreeCtrl). Re-implements the wx.TreeCtrl API surface used by login_dialog.py on top of TreeListCtrl. Also adds name="Servers and accounts" to the tree control for accessibility.
The non-darwin path is untouched.
Test results
Module imports cleanly on both real Windows and a forced-darwin code path.
Windows MRO unchanged:ManagedTreeCtrl → TreeSelectionManagerMixin → TreeCtrl → Control ✓
API audit of login_dialog.py against the shim: every call site is covered (either explicitly shimmed or inherited from TreeListCtrl with compatible semantics).
Couldn't run the actual GUI test on macOS from a Windows machine. The author needs to confirm VoiceOver behavior in person.
Issues
1. API parity gap — select_after_delete() missing on macOS
The non-macOS class inherits select_after_delete() from TreeSelectionManagerMixin; the macOS class doesn't. login_dialog.py doesn't currently call it, but any future caller hits AttributeError on macOS only — a silent platform regression.
Worse: the macOS __init__ accepts and stores focus_after_delete but the method that would use it doesn't exist. Either port select_after_delete() onto the macOS class, or remove the unused parameter.
2. Class redefinition pattern triggers linter warnings
login_dialog.py passes style=wx.TR_HAS_BUTTONS | wx.TR_SINGLE | wx.TR_HIDE_ROOT. The TR_* flags are TreeCtrl-specific and don't apply to TreeListCtrl, so the silent drop happens to be safe, but it's invisible to readers. Add a comment explaining tree style flags are intentionally ignored, or accept-and-warn on unknown flags.
login_dialog.py only uses these two, so it works today. If anyone later binds EVT_TREE_KEY_DOWN, EVT_TREE_ITEM_EXPANDED, etc., the bind silently passes through to TreeListCtrl which doesn't fire those events — a future invisible breakage. Either expand the table or assert/warn on unknown wx.EVT_TREE_* events.
5. No tests
GUI code is hard to unit-test, but the shim's bookkeeping (_item_key, GetFirstChild/GetNextChild traversal, GetChildrenCount, DeleteAllItems reset) is pure Python and could be exercised against a stubbed TreeListCtrl. The PR is 144 lines with zero tests.
6. Type hints use Any everywhere
parent: Any, item: Any, event: Any, data: Any. The proper types exist (dv.TreeListItem, wx.PyEventBinder, etc.) and would catch the kind of platform-asymmetry bugs called out above. Minor.
Recommendation
The PR's core idea — switch macOS to TreeListCtrl for VoiceOver — is right. The shim is a pragmatic way to avoid touching every caller. Suggested fixes before merge:
Add select_after_delete() to the macOS class (or factor the mixin to be callable on either base) so the API matches across platforms.
Restructure as class _MacManagedTreeCtrl + alias to silence linter warnings and make platform routing explicit.
Optional but worthwhile:
3. Make the Bind translation table exhaustive (or fail loud on unknown EVT_TREE_*).
4. Tighten type hints.
Tested locally on Windows. The new tests catch a real cross-platform issue:
test_managed_tree_ctrl_delete_all_items_resets_bookkeeping fails on Windows because it exercises the macOS shim's bookkeeping (_item_data dict reset, persistent compat-root) on the wx.TreeCtrl path. After DeleteAllItems(), wx.TreeCtrl.GetRootItem() returns an invalid item, so GetFirstChild(root) raises wxAssertionError: "item.IsOk()" failed, and GetItemData(server_a) on a stale TreeItemId is undefined behavior on the native control.
The other three tests pass / skip cleanly on Windows, so this is a test-only issue — the production shim and login-dialog changes are fine to merge.
A follow-up branch with the one-line fix is ready: zarvox/fix-tree-selection-test-gate (commit). It just adds @pytest.mark.skipif(sys.platform != "darwin", ...) to that test, mirroring the existing gate on test_managed_tree_ctrl_rejects_unsupported_tree_event_binders. I'll open a follow-up PR after this one merges.
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
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.
the items in the treeview are also readable on mac now again in.