Fix vendor catalog Search regression after #6073 multi-select#6253
Fix vendor catalog Search regression after #6073 multi-select#6253heffneil wants to merge 6 commits into
Conversation
…anges Two regressions from PR xLightsSequencer#6073 (multi-select catalog) that surfaced together when using the Search box: 1. Search button perma-disabled on Windows. ValidateWindow gated the button on GetFocusedItem().IsOk(). On the native Windows wxTreeCtrl, GetFocusedItem() returns invalid whenever keyboard focus is outside the tree — which is exactly the case while the user is typing in the search textbox. Mac's generic implementation tracks focused item independently of keyboard focus so the bug only showed up on Windows. Drop the focused-item gate, only require non-empty search text and a populated tree. 2. OnButton_SearchClick bailed when GetFocusedItem() was invalid. Even if the user could click Search (post fix xLightsSequencer#1), the loop would immediately return. Fall back to the first selected item, then to the root's first child, so the user can always kick off a search from the top of the tree. 3. Stepping through matches accumulated selection instead of replacing it. The tree was switched to wxTR_MULTIPLE in xLightsSequencer#6073, where SelectItem() *adds* to the selection set rather than replacing. Searching for 'pumpkin' and clicking Search repeatedly piled up every pumpkin into one giant multi-selection. UnselectAll() before SelectItem() so each Search step shows exactly one match. This intentionally clears any pre-existing user multi-selection — Search is a navigation tool, not a multi-select builder; user can shift- click after to build a multi-selection from search hits.
There was a problem hiding this comment.
Pull request overview
Fixes regressions in the Vendor Model Catalog search behavior introduced with multi-select tree support (#6073), especially on Windows where focused-item semantics differ.
Changes:
- Updates search button enablement logic to avoid depending on
GetFocusedItem()(Windows-nativewxTreeCtrlfocus behavior). - Adds fallback start node selection for search when no focused item exists.
- Clears multi-selection before selecting the next search match to avoid accumulating selections with
wxTR_MULTIPLE.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src-ui-wx/import_export/VendorModelDialog.cpp |
Adjusts search enablement and search traversal/selection behavior to work correctly with Windows focus rules and multi-select trees. |
README.txt |
Adds release note entry describing the vendor catalog search regression fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Four points raised, all valid:
1+2. Brace style. The two fallback if blocks I added in
OnButton_SearchClick were K&R while the surrounding function uses
Allman — reformatted to match.
3. README grammar. 'each step UnselectAll's before SelectItem' was
ungrammatical; rewrote as 'each step calls UnselectAll before
SelectItem so only the current match is highlighted'.
4. ValidateWindow's hasItems check was too lax. GetCount() > 0 is
true whenever the (hidden) root exists, even with no actual
model entries to search. The click handler already requires
GetChildrenCount(GetRootItem()) > 0 — match that here so the
Search button can't enable when there's nothing to find.
|
Windows .. still cant select the next item. I enter spinner, hit search. It finds the first one. Hit Search again .. still on the first item. |
Windows tester reports that the second press of Search after a match doesn't advance to the next occurrence. Root cause: after we call UnselectAll() + SelectItem() and return, focus on the Windows native wxTreeCtrl is on the Search button, not the tree. The next click then hits OnButton_SearchClick with: - GetFocusedItem() returning invalid (focus is outside the tree), and - GetSelections() not always reflecting the programmatically-selected item across platforms / wxTR_MULTIPLE. Result: the fallback chain ended up at root's first child, and the loop's first advance landed on the same first match again. Cache the last hit ourselves in _lastSearchHit (plus _lastSearchText so we reset the cache when the user types a new query). The Search loop now resumes from this remembered item and reliably steps forward on every click. Falls back to GetFocusedItem -> selection -> root's first child only when the cache is empty / stale (initial click, or after the search text changed).
|
@derwin12 — pushed a follow-up that should fix the second-press behaviour you reported. Root cause: after Fix caches the last hit in a member ( Verified on macOS — second/third/Nth press now advances. Windows artifact should be on the next CI run if you want to grab it: scroll to the latest "Windows xLights CI" check on this PR → Details → Artifacts → |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- ValidateWindow's hasItems check now passes recursive=false to GetChildrenCount. The default is recursive, which would walk the entire vendor tree on every keystroke just to determine 'is there anything to search?'. Direct-children-only is O(1) and semantically the same answer. - OnButton_SearchClick now lowercases TextCtrl_Search->GetValue() once before the traversal loop instead of re-lowering it on every iteration. The traversal can visit hundreds of items per click on big catalogs.
|
Now it goes to the next entry but it doesnt actually select it and hence show the prop on the right side. Confirmed it did that in the .01 release. |
|
PS .. tell the intern to skip adding new comments to the code. It is adding more comments then code. |
GetFocusedItem() returns invalid on Windows when keyboard focus is outside the tree (e.g. user clicked the Search button), so the detail-pane handler bailed and never updated. Take the item from the event itself, with GetFocusedItem as a fallback.
|
@derwin12 Pushed a fix. Also — apologies on the verbose comments, will trim going forward. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Trim search text before use; ValidateWindow already trims when enabling the Search button so the loop should match. - Honour user's manual selection: if they picked a node since the last cached hit, start from that node instead of resuming from cache. - GetChildrenCount(root) defaulted to recursive; pass false for the cheap direct-children-only check.
|
@derwin12 — pushed three more rounds of fixes since your last report (commits
Latest CI Windows artifact at https://github.com/xLightsSequencer/xLights/actions/runs/25123707052 if you have a moment to retest. Appreciate the patience. |
|
Update — opened PR #6256 which adds a live whitespace-tokenized filter above the catalog tree. Tested on Windows: it both fixes the search-step issues this PR was chasing AND covers the wider 'huge catalog hard to navigate' UX in one shot. Filter narrows the tree as you type, hierarchy-aware so typing 'tree EFL' or 'boscoyo halloween' just works. Suggest considering #6256 as the path forward and closing this one once #6256 lands. Happy to close this PR myself if maintainers prefer; leaving it open for now in case you want to use the small targeted fixes here as a stop-gap before #6256 merges. |
|
Close this .. if you have a better one ... |
|
This is replaced with a better filter coming in another PR |
Summary
Fixes two regressions in the vendor model catalog Search after PR #6073 (multi-select catalog).
Bugs
1. Search button perma-disabled on Windows
ValidateWindowwas gating the Search button onGetFocusedItem().IsOk(). On the native WindowswxTreeCtrl,GetFocusedItem()returns invalid whenever keyboard focus is outside the tree — i.e. while the user is typing in the search textbox itself. Result: button stayed greyed out, search was effectively dead on Windows. macOS's generic implementation tracks focused item independently of keyboard focus, so the bug never surfaced there.Fix: drop the focused-item gate. Only require non-empty search text and a populated tree.
2.
OnButton_SearchClickbailed when no focused itemEven if Search was clickable, the search loop returned immediately when
GetFocusedItem()was invalid (same Windows root cause).Fix: when
GetFocusedItem()is invalid, fall back to the first selected item, then to the root's first child, so the user can always kick off a search from the top of the tree.3. Stepping through matches accumulated selection
PR #6073 switched the tree to
wxTR_MULTIPLE. In that mode,SelectItem()adds to the selection set rather than replacing. Each Search step was silently piling matches into the multi-selection — searching for "pumpkin" and clicking Search repeatedly produced this:[user's screenshot showing 6 highlighted pumpkin entries]
Fix:
UnselectAll()beforeSelectItem()so each Search step shows exactly one match. This intentionally clears pre-existing user selection — Search is a navigation tool, not a multi-select builder. User can shift-click afterwards to build a download list from the search results.Test plan
Out of scope
The "Search ↔ multi-select for download" interaction has a residual mild UX wart: shift-clicking to build a multi-download from search hits requires the user to keep search results in mind manually. Could be worth a separate enhancement (e.g. "add all matches to selection" button) but that's a feature, not a regression.