feat(gui): port CServerListCtrl to wxDataViewCtrl (#180, #801) - #807
feat(gui): port CServerListCtrl to wxDataViewCtrl (#180, #801)#807LSalami wants to merge 2 commits into
Conversation
…le-org#801) CMuleVirtualListCtrl wraps the vendored wxGenericListCtrl, which is owner-drawn and invisible to VoiceOver/Orca -- the same root cause amule-org#180 and PR amule-org#796 (search results list) already fixed for CSearchListCtrl. This is the first port of a amule-org#801 remaining list: CServerListCtrl was chosen as the smallest *valid* proof-of-concept since it's purely text-rendered (no CBarShader bars, unlike Downloads/SharedFiles) and has the smallest external blast radius among the CMuleVirtualListCtrl candidates. New CServerListModel is a flat wxDataViewModel (not wx's built-in wxDataViewVirtualListModel, which is row-index addressed and would force a translation layer at every one of the control's existing CServer*-identity call sites), reusing the dirty-flag + idle-coalesced Cleared() notification pattern PR amule-org#796 converged on after its own GTK/MSW structural-notification regressions -- this sidesteps that bug class entirely rather than re-deriving it. Guards against every regression class amule-org#796's review already found once: wxDV_MULTIPLE explicit (control defaults to single-select), type-to-select and Cmd/Ctrl+A reimplemented in OnChar (no backend provides them), wxDATAVIEW_COL_SORTABLE on every column (otherwise no header caret), and all mutations funnel through MarkDirty()/OnIdle rather than incremental Item* notifications. Public API kept name/signature-stable for ServerWnd.cpp/GuiEvents.cpp/ amule-remote-gui.cpp; muuli_wdr.cpp's construction call updated for the new ctor (dropped the unused wxLC_* style parameter, matching how CSearchListCtrl's port already did). CListColumnStore/IColumnWidthProvider (PR amule-org#787) carries over unchanged via the same adapter pattern already proven by CSearchListCtrl. Verified: builds clean (monolithic macOS target); clang-format v18 (pinned Docker image) applied; clang-tidy Tier-1 (whole-tree) and Tier-2 (changed-lines, .clang-tidy-new-code) both clean via the ~/aMuleTest/ci-local replica -- caught and fixed one real bugrone-narrowing-conversions hit in OnChar along the way. Launched against an isolated ~/aMuleTest/monolithic instance (non-default ports): starts clean, loads server.met, connects and starts Kad with no crashes in the log. Interactive verification (sort persistence, multi-select, type-to-select, live updates, VoiceOver) is left for manual testing per project convention, same as PR amule-org#796.
CI's "App catalogs in sync with source" caught this: the translatable strings that used to live in CServerListCtrl::GetItemColumnText (now CServerListModel::GetValue, after the wxDataViewCtrl port) moved to a new source file that wasn't yet listed in po/POTFILES.in. Verified per project convention: msgid count unchanged (1935 before and after), and the diff is symmetric per-locale (same N msgid blocks removed/re-added, reordered by the new source-reference position) -- no strings actually added, removed, or retranslated, only source comments moved from ServerListCtrl.cpp to ServerListModel.cpp.
|
Reviewed at 1. Use-after-free when a server is removed. Confirmed under ASAN on wxGTK 3.2.9 with a standalone A repaint alone didn't trip it; This isn't a flaw in reusing the #796 pattern — it was safe there because the only removal path tore the whole tab down before freeing, and Fix: flush synchronously on removal — 2. #805 merged after this branch was cut, and every fix in it applies here.
3. The duplication is now measurable, and it argues for extracting the base before this lands rather than after. Against the merged
#801 said to defer generalizing until 2+ lists were ported and the shape was clear. That condition is met, and this PR shows the cost of waiting: it inherited the parts of the pattern that were public when it was cut and missed everything fixed in the following three hours. A third port repeats it. Concretely, Minor: |
|
Following up on the notification model, since this port will be the template for Downloads and Shared Files. The A flat list has no such transition, so it inherits the cost without the reason. Every value update discards the control's entire tree and rebuilds it, where Verified that targeted notifications behave correctly for a flat model on wxGTK 3.2.9 (standalone Suggested shape:
That also lets One thing worth deciding here rather than twice more: for Shared Files at 10k+ rows, the per-item nodes an identity-addressed model creates on GTK/MSW are themselves significant, and |
|
@LSalami — heads up before you spend more time on this: I've opened #811, which supersedes this one. That's my call as maintainer and not a reflection on the work here; your port is what made the shared-base case concrete, and the column handling and API-stability approach carried over into it. The short version of why: #801 deferred the shared-base question until the shape was clear from more than one list, and this PR made it clear. Rather than ask you to absorb a base-class extraction mid-review, we built it and ported the server list onto it, which also folds in the #805 fixes this branch predates and the use-after-free from the review above. If you want to keep going on the migration, Downloads and Shared Files are the next two on #801 and both would now start from the base rather than from scratch — happy to walk through the hooks. |
The first list onto CMuleVirtualDataViewCtrl, and what makes it more than dead code. Supersedes the port in #807, whose approach and column handling informed this one. The list keeps its public API, so ServerWnd, GuiEvents and the remote GUI are unchanged, and AddServer/RemoveServer/RefreshServer/ HighlightServer work as they did because the base offers the same AddItemData/RemoveItemData/RefreshItemData vocabulary CMuleVirtualList- Ctrl did. What is left here is the list's own business: cell text, country flags, the bold connected server, the comparator, the context menu and column auto-fit. Removals notify the control before the core frees the server, rather than deferring to idle: the row's identity is the object, so anything touching the list in between -- a repaint, a resort, a menu handler reading the selection -- would be reading freed memory. Ping, Users and Files still drive the live re-sort, gated by thePrefs::LiveListSort() as before; the TCP/UDP flag columns are still hidden by default in release builds, now through the header menu's hidden state rather than a zero width.
…) (#830) * feat(gui): port CFriendListCtrl to CMuleVirtualDataViewCtrl (#180, #801) Continues the wxListCtrl -> wxDataViewCtrl migration (#180/#801) onto the shared base PR #811 extracted after the Servers port (#807, now absorbed into #811). CFriendListCtrl was picked next after reading all the remaining candidates directly rather than going by line count alone: it has no CBarShader rendering (unlike Downloads/SharedFiles/ Sources-Peers, which additionally need a still-nonexistent wxDataViewCustomRenderer), it's already pointer-identity addressed (UpdateFriend(CFriend*)/RemoveFriend(CFriend*) map directly onto AddItemData/RefreshItemData/RemoveItemData), and its blast radius is tiny (ChatWnd.cpp, muuli_wdr.cpp construction only). CFileDetailListCtrl looked smaller by line count but its caller (FileDetailDialog.cpp) drives it with raw position-indexed CRUD (FindItem by name, SetItem by column index, DeleteItem) plus a pre-existing duplicate-type quirk (two unrelated SourcenameItem structs relying on compatible layout) -- porting it cleanly means refactoring the caller too, which is a separate, more invasive piece of work than this one. CServerListCtrl (current, post-#811) is the template mirrored here: same AppendTextColumn/AppendSpacerColumn/AssociateVirtualModel/ LoadColumnSettings/InitColumnState ctor sequence, same GetItemColumnText/ GetItemAttr/CompareItemData/OnListKey hook shape. Public API (UpdateFriend, RemoveFriend) and the constructor signature are unchanged, so ChatWnd.cpp and muuli_wdr.cpp needed no edits. Notable deltas from the pre-port behaviour: added a real CompareItemData so header-click sort now works (the old list never called SetSortFunc at all); GetItemAttr replaces the old SetItemTextColour call with the same visible result (blue for linked friends, default text colour otherwise). Verified: builds clean; clang-format v18 (pinned Docker image) applied; clang-tidy Tier-1 (whole-tree) and Tier-2 (changed-lines, .clang-tidy-new-code) both clean via the ~/aMuleTest/ci-local replica -- Tier-2 caught one real modernize-use-nullptr hit, fixed. Grepped ChatWnd.cpp/.h and muuli_wdr.cpp to confirm no other call sites exist. Interactive verification (sort, right-click menu states, Delete-key removal, chat-session activation, VoiceOver) left for manual testing per project convention, same as PR #796/#807. * fix(gui): resolve activated friend through selection, not row-as-pointer got3nks's review on #830 found a real crash: OnItemActivated() cast event.GetItem()'s ID directly to CFriend* on the assumption it was the item's data pointer, but CMuleVirtualDataViewCtrl's row-addressed model returns the row index (+1) as that ID -- the "item identity is not row identity" case MuleVirtualDataViewCtrl.h itself documents. Every double-click/Enter on a friend dereferenced a bogus pointer. Fixed by selecting the activated row and resolving it through GetSelectedItemData(), matching CServerListCtrl::OnItemActivated. Also from the same review: IsLiveSortColumn() now returns true, since the name (the only sortable column) can change after a friend is already listed and the UpdateFriend() comment claimed a re-sort that the base's default-false hook never actually triggered; and dropped a no-op static_cast<int>() around a call that already returns int. Verified: builds clean, clang-format v18 applied, clang-tidy Tier-2 (changed lines, .clang-tidy-new-code) clean via the local CI replica. * chore: retrigger CI The previous run hit a GitHub Actions infrastructure outage (job not acquired by any runner, "Failed to resolve action download info" / "Service Unavailable" on clang-format, Translation checks, mingw-w64 Debug and clang-tidy Tier-1) unrelated to this branch's code -- every job that did run passed. No admin rights to rerun the failed jobs directly, so retriggering with an empty commit instead.
…#801) (#839) * feat(gui): port CFileDetailListCtrl to CMuleVirtualDataViewCtrl (#180, #801) Continues the wxListCtrl -> wxDataViewCtrl migration (#180/#801) onto the shared base from #811, after Search (#796), Servers (#807/#811) and Friends (#830). CFileDetailListCtrl was next because it has no CBarShader rendering (unlike Downloads/SharedFiles/Sources-Peers, which additionally need a still-nonexistent wxDataViewCustomRenderer). Its caller, CFileDetailDialog::FillSourcenameList(), drove the old list with raw position-indexed CRUD (FindItem by name, SetItemPtrData, SetItem by column index, DeleteItem) rather than the pointer-identity style CServerListCtrl/CFriendListCtrl already use, so porting the control cleanly meant refactoring the caller too: - CFileDetailDialog gained a std::map<wxString, SourcenameItem *> m_sourcenames member, replacing "search the list widget by name" with a map lookup. FillSourcenameList()'s reset/update/prune shape is otherwise unchanged, just re-keyed to the map and driven through the control's new AddSource/RefreshSource/RemoveSource API. - OnBnClickedTakeOver()/OnListClickedTakeOver() resolve the selected row through GetSelectedItemData() (inherited from CMuleVirtualDataViewCtrl) instead of GetNextItem+GetItemText. - The dialog's EVT_LIST_ITEM_ACTIVATED binding on IDC_LISTCTRLFILENAMES becomes EVT_DATAVIEW_ITEM_ACTIVATED; wxDataViewEvent is a wxNotifyEvent/wxCommandEvent descendant so it still propagates from the child control to the dialog-level handler the same way. - Fixed a latent leak: nothing in ~CFileDetailDialog() freed the SourcenameItem objects still referenced by open rows -- only the "not a partfile" and "pruned to zero" paths in FillSourcenameList() ever deleted them. Now freed in the destructor via m_sourcenames. CMuleDataViewCtrl always ORs in wxDV_MULTIPLE (no single-selection mode exists on the shared base), but this list's "take over filename" actions assume exactly one selection, same as the old list did (it never actually requested wxLC_SINGLE_SEL either -- checked muuli_wdr.cpp -- it enforced single-selection itself via OnSelect() deselecting every other row). CFileDetailListCtrl::OnSelectionChanged() does the equivalent: collapses to the just-clicked row whenever more than one ends up selected. Also deleted a vestigial nested SourcenameItem struct in FileDetailListCtrl.h that duplicated (by accident of matching layout) the real one in PartFile.h, and dropped a per-row background-colour set that reproduced the default and carried its own "do we still need this?" comment. Verified: builds clean; clang-format v18 (pinned Docker image) applied; clang-tidy Tier-1 (whole-tree) and Tier-2 (changed-lines, .clang-tidy-new-code) both clean via the ~/aMuleTest/ci-local replica. Grepped for other CFileDetailListCtrl/IDC_LISTCTRLFILENAMES references to confirm muuli_wdr.cpp's construction call needed no changes. Interactive verification (source list populate/re-tally, take-over via button and double-click, single-selection enforcement, sort, VoiceOver) left for manual testing per project convention. * fix(gui): keep the source-name list sorted, and drop rows before freeing them Review follow-up to the port. FillSourcenameList() zeroes every count and then rewrites them in place, so while it runs the list is not ordered by the column it is sorted on -- and AddSource() places a new row with a binary search, which needs that ordering to hold. An insertion therefore leaves rows in arbitrary positions, and the repair has to happen here, exactly as the pre-port code did. Only on insertion, though. A count that merely changed is what the live-sort preference governs: RefreshSource() re-sorts when it is on, and when it is off the row is meant to stay put rather than move under the user. Sorting unconditionally would quietly override that setting for this list. The destructor and the not-a-partfile path also freed the SourcenameItem objects while the list control still held pointers to them. Nothing can paint or sort in either window today -- both dialog call sites are stack temporaries, so destruction is synchronous -- but that rests on wx's teardown order rather than on anything guaranteed, and the base states the rule plainly: it has to be told before the caller frees the item data. Both now clear the rows first, as the prune loop already did. * fix(gui): decide the source list's live re-sort per column IsLiveSortColumn() answered true for every column, so a refresh tick scheduled a re-sort even when the list was sorted by File Name -- a value that never changes, since the name is the key each row was created under. Answer per column instead, the shape CServerListCtrl already uses, so only a sources-sorted list re-sorts on its own. The header comment already described it this way; only the implementation did not. * chore: drop a duplicated paragraph from the file header FileDetailDialog.cpp carried the "Any parts of this program derived from the xMule, lMule or eMule project" paragraph twice. It is the only file in src/ that does, and the file is already being touched here. --------- Co-authored-by: got3nks <[email protected]>
2 participants
Context
#180 is the accessibility bug:
CMuleListCtrl/CMuleVirtualListCtrlwrap the vendoredwxGenericListCtrl, owner-drawn with no native backing, so VoiceOver/Orca can't see it. PR #796 fixed this for the search results list by porting towxDataViewCtrl. #801 tracks the remaining lists still on the old base; this is the first of those ports.CServerListCtrlwas chosen as the smallest valid proof-of-concept among theCMuleVirtualListCtrlcandidates: it's purely text-rendered (noCBarShaderprogress/availability bars, unlike Downloads/SharedFiles) and has the smallest external blast radius. No shared base class is extracted yet — per got3nks's own suggestion on #801, that generalization is deferred until 2+ of these lists are ported and the common shape is empirically clear.What changed
CServerListModel: a flatwxDataViewModel(not wx'swxDataViewVirtualListModel, which is row-index addressed and would force a translation layer at the control's existingCServer*-identity call sites likeRemoveServer(CServer*)/HighlightServer(const CServer*, bool)). Reuses the dirty-flag + idle-coalescedCleared()notification pattern feat(search): port CSearchListCtrl to wxDataViewCtrl #796 converged on after its own GTK/MSW structural-notification regressions.CServerListCtrlrewritten to inheritwxDataViewCtrldirectly. Public API (AddServer,RemoveServer,RefreshServer,HighlightServer,DeleteAllItems,ShowServerCount,FitColumnsToContent) kept name/signature-stable soServerWnd.cpp/GuiEvents.cpp/amule-remote-gui.cppneeded no changes beyond recompiling.RemoveAllServersdropped itswxLIST_STATE_*parameter for a plainbool selectedOnly(internal-only, no external callers).wxDV_MULTIPLEexplicit (the control defaults to single-select).OnChar— nowxDataViewCtrlbackend provides them the way the old list did.wxDATAVIEW_COL_SORTABLEon every column (otherwise sort works but no header caret).MarkDirty()/idle-flushedCleared(), never incrementalItemAdded/ItemDeleted/ItemChanged.IsLiveSortColumn()'s bespoke live-resort machinery is gone entirely:RefreshServer()'sMarkDirty()→ idleCleared()already re-sorts againstCompare()for free, a genuine simplification versus the base being replaced.CListColumnStore/IColumnWidthProvider(refactor(gui): extract column persistence out of CMuleListCtrl #787) carries over unchanged via the sameColumnWidthAdapterpatternCSearchListCtrlalready proved.muuli_wdr.cpp's construction call updated for the new ctor (dropped the unusedwxLC_*style parameter, same as Search's port).Verification
clang-formatv18 (pinned Docker image) applied..clang-tidy-new-code) both clean via a local CI replica — caught and fixed one realbugprone-narrowing-conversionshit inOnCharalong the way.server.met(9 servers), connects and starts Kad, no crashes in the log.