fix(search): restore the column show/hide menu and shift+page selection - #805
Conversation
Two more behaviours the wxDataViewCtrl port left behind, both inherited for free from CMuleListCtrl by every list that is still on wxListCtrl. Right-clicking the header opened a check-menu of columns (CMuleListCtrl::OnColumnRClick/OnMenuSelected); the ported search list had no header right-click handler at all, so there was no way to hide a column. Restored on CSearchListCtrl with the same semantics: hiding sets the column to COL_SIZE_MIN and caches the previous width, so re-showing restores what the user had and the state persists through CListColumnStore's existing width handling rather than needing a second mechanism. COL_SIZE_MIN moves from a file-local constant in MuleListCtrl.cpp to ListColumnStore.h, next to the column-width interface, so both lists read the same threshold instead of keeping copies (it is 10 on wxGTK, which won't shrink a column past the header grip, and 0 elsewhere). Shift+page-up/down extends the selection on GTK and MSW, whose backends implement it, but does nothing on macOS: NSOutlineView pages the view without touching the selection. Handled by hand there so the three ports agree. Plain page-up/down is deliberately left to the platform -- macOS scrolls without moving the selection by convention, and that stays. The row ordering the type-ahead search already needed is factored into BuildDisplayOrder() and shared with the page handler rather than duplicated; both need the top-level rows in displayed order, which means running them through this list's own comparator since GetItemByRow()/GetRowByItem() exist only in wx's generic implementation.
Four problems from testing the first cut. wxDataViewCtrl won't shrink a column to nothing, so setting the width to COL_SIZE_MIN left a header stub behind instead of hiding the column, and GetWidth() never dropped to the threshold -- which meant the menu kept showing a hidden column as visible and toggling it again just re-cached the width, so it could not be brought back. Hiding now goes through wxDataViewColumn::SetHidden() and the menu reads IsHidden(). Persistence is unchanged: ColumnWidthAdapter reports a hidden column as zero-width and treats a stored width at or below COL_SIZE_MIN as hidden, so CListColumnStore still round-trips the state through the width it already saves. Hiding the expander column would have taken the group triangles with it and left children unreachable, so the expander moves to the first column still visible. Shift+page-up/down moved a single row because the page size came from GetItemRect(), which returns an empty rect for rows that aren't on screen; the zero height collapsed a page to one row. GetCountPerPage() is implemented by the native macOS backend and is used instead. Shift+home/end weren't handled at all -- the view jumped but nothing was selected -- so they now extend the selection to the start or end of the list. The unshifted forms still belong to the platform.
Hiding the column that owned the expander moved it to the next visible one, but re-showing a column to its left didn't take it back, so after hiding and restoring the file name column the group triangles stayed out in the middle of the row. The rule is simply that the expander belongs on the leftmost visible column, so both directions go through one helper, which also runs after LoadColumnSettings(): a restored width can leave the default expander column hidden, which would strand the triangles on a column nobody can see.
|
Interactive pass done on macOS, Ubuntu 26.04 arm64 (wxGTK 3.2.9) and Windows 11 arm64. Four things the first cut got wrong, all fixed in the commits above:
Verified after the fixes: hiding and restoring any column including the file name one, hidden columns surviving a restart, the expander following the leftmost visible column both ways, shift+page/home/end extending the selection on macOS, and the unshifted keys plus GTK/MSW selection behaviour unchanged. |
SyncLists() mirrors column state between open search tabs, but only copied widths. A hidden column reports a width of zero, so hiding one in any tab pushed that zero to the others without hiding them -- leaving a column that is invisible in the list yet still ticked in the header menu, and so impossible to restore from there. Toggling it there just cached the zero as its "previous" width. Hidden state now travels with the width, and only a visible column's width is copied, so an empty width never reaches another tab. Hide/show also syncs immediately rather than waiting for the idle width-comparison to notice, and the receiving tab re-derives its expander column, since the sync can hide the column that was carrying it.
Observed on macOS: after hiding Codec and Directory the columns were gone from the list, but the header menu still ticked both and toggling them did nothing useful. wxDataViewColumn::SetHidden() had taken effect while IsHidden() went on reporting the column as shown, so every decision made by reading the state back was wrong -- the menu's check marks, the expander's "leftmost visible" search, the width the adapter reported to CListColumnStore, and what SyncLists() copied to the other tabs. CSearchListCtrl now owns the hidden state and only ever writes to the control, so nothing depends on how faithfully a port reports it. The persisted form is unchanged: a hidden column is still saved as a zero width, which CListColumnStore already stores as a negative entry carrying the width to restore.
The previous commit justified tracking the state by claiming macOS disagreed with itself -- that SetHidden() took effect while IsHidden() still reported the column as shown. That was wrong. A probe against wx 3.3.3 on macOS shows SetHidden(true) is followed by IsHidden() returning true, as expected. What is true, and what the code actually has to account for, is that a hidden column keeps reporting its previous width rather than zero. The width is what CListColumnStore persists, so the adapter has to answer "hidden" from somewhere other than the width -- which is what the tracked state is for. Same code, honest reason.
macOS sizes the trailing column to whatever space is left over, so once the columns are collectively wider than the control it reports a width of zero -- while remaining a perfectly visible column the user can scroll to. Confirmed with a standalone wxDataViewCtrl carrying this list's eleven columns: in a window too narrow for them the last column reported zero while every other column, including ones entirely off screen, reported its real width; widening the window until they all fit made it report a real width again. CListColumnStore::SaveSettings() treats a width <= 0 as hidden and writes it as a negative entry, so each shutdown marked the last column hidden; the next launch hid it and then did the same to whichever column had become last, losing one per restart. This predates the header menu -- it is why configs from the port already carry entries like "C:-80,D:-258" -- but it only became visible once a negative width started hiding the column outright instead of leaving a zero-width stub. The width adapter no longer lets a zero reach the store for a column that isn't hidden: hidden state is tracked explicitly now, so a reported zero falls back to the column's remembered or default width.
95ba314 to
a2914e5
Compare
macOS hands the leftover space to the last *resizable* column, so with eleven columns wider than the control the trailing one is sized to nothing: invisible in the list, and reported by GetWidth() as zero. CListColumnStore reads a width <= 0 as hidden and persists it as a negative entry, so each shutdown marked that column hidden and the next launch did the same to whichever column had become last -- one lost per restart, which is why configs from the port already carry entries like "C:-80,D:-258". This predates the header menu; it only became obvious once a negative width started hiding the column outright instead of leaving a stub. An empty spacer column now takes that position, so no column the user cares about is ever the last resizable one. It has to be resizable itself -- a fixed spacer cannot shrink and the collapse simply falls through to the last real column. RealColumnCount() keeps it out of the header menu, the persisted widths, the hidden-state tracking and the cross-tab sync, and the width adapter refuses to pass a zero for a column that isn't hidden, so a stray zero can no longer eat a column even if the spacer is ever not last. SetMinWidth() was the cheaper alternative but only stops the width reaching zero -- the column still shrinks to its minimum rather than keeping its configured width. Separately, BuildDisplayOrder() now includes the children of expanded groups. It is measured against what is on screen -- a page is GetCountPerPage() rows, children included -- so leaving them out made shift+page overshoot by however many children were showing and skip every child inside the range. Type-to-select shares the helper and can now land on a child row too.
|
Pushed the fixes from testing on all three platforms. Two things worth calling out for review. The trailing column collapsing is a pre-existing bug from #796, not something this PR introduced, and it is specific to the The lists still on The fix is an empty spacer column that occupies the position macOS insists on shrinking. It has to be resizable itself — a fixed spacer cannot shrink and the collapse simply falls through to the last real column.
One caveat if you read this commit-by-commit: Verified interactively on macOS, Ubuntu 26.04 arm64 (wxGTK 3.2.9) and Windows 11 arm64: the column menu, hidden columns surviving restarts, the expander following the leftmost visible column, cross-tab sync, and shift+page/home/end on macOS. Roughly 110 of the ~340 added lines are |
RealColumnCount() returns unsigned while the hidden-state helpers take an int column index, so the loop variable narrowed implicitly on three lines -- caught by the Tier-2 clang-tidy gate.
#796 ported the search list to wxDataViewCtrl and #805 fixed what that port left behind. #801 asked whether the remaining lists should each be ported independently or share a base, and deferred the answer until the shape was clear from more than one list. It now is. CMuleDataViewCtrl is the wxDataViewCtrl counterpart of CMuleListCtrl: column widths and their persistence, the header show/hide menu, hidden state, the multi-column sort chain, type-to-select, Cmd/Ctrl+A, the macOS shifted page/home/end keys, the trailing-column spacer, and drag-resize detection. It owns no data; a list supplies its rows through GetDisplayOrder(), their label through GetRowLabel(), and how two of them compare through CompareByColumn(). CMuleVirtualDataViewCtrl is the counterpart of CMuleVirtualListCtrl, and virtual in the same sense: rows are addressed by index through a wxDataViewIndexListModel and nothing is materialised per row. It carries the item-identity bookkeeping an identity-addressed port avoids -- a wxDataViewItem from a row-addressed model encodes the row number, so a deletion silently retargets any item held across it, which is why everything here speaks in wxUIntPtr and re-resolves selection after each mutation. Also the legacy filter API, live re-sort coalesced through one CallAfter and deferred while the user is interacting, bulk append and batch removal, and icon columns. CSearchListCtrl moves onto the plain base, losing 499 lines.
1 participant
Two more behaviours the
wxDataViewCtrlport left behind, both of which every list still onwxListCtrlinherits for free fromCMuleListCtrl. Same class as the three restored in #796 (multi-selection, type-to-select, select-all) and the reason #801 argues for a shared base before the next list is ported.Column show/hide menu
Right-clicking a column header opened a check-menu of columns (
CMuleListCtrl::OnColumnRClick/OnMenuSelected). The ported search list bound no header right-click handler at all, so there was no way to hide a column.Restored on
CSearchListCtrlwith the original semantics: hiding sets the column toCOL_SIZE_MINand caches the previous width, so re-showing restores what the user had, and the state persists throughCListColumnStore's existing width handling instead of needing a second mechanism.COL_SIZE_MINmoves from a file-local constant inMuleListCtrl.cpptoListColumnStore.h, beside the column-width interface, so both lists read one threshold rather than keeping copies. It is 10 on wxGTK — which won't shrink a column past the header grip — and 0 elsewhere, so this touches the GTK build in particular.Shift+page-up/down on macOS
Shift+page-up/down extends the selection on GTK and MSW, whose backends implement it. On macOS it does nothing:
NSOutlineViewpages the view without touching the selection. Handled by hand under__WXOSX__so the three ports agree; repeated presses keep extending, since it grows the existing selection between the cursor's old and new position.Plain page-up/down is deliberately left to the platform — macOS scrolls without moving the selection by convention (Finder, Mail) — and that behaviour is unchanged.
Shared ordering
Both the existing type-ahead search and the new page handler need the top-level rows in displayed order, so that is factored into
BuildDisplayOrder()and shared rather than written twice. It runs the rows through this list's own comparator becauseGetItemByRow()/GetRowByItem()exist only in wx's generic implementation, not on GTK or macOS.Testing
Built clean on macOS, Ubuntu 26.04 arm64 (wxGTK 3.2.9) and Windows 11 arm64;
clang-formatand bothclang-tidytiers clean on the diff.Draft until the interactive pass on all three is done — specifically that a hidden column stays hidden across a restart, and that the macOS page size feels right.