Skip to content

feat(gui): shared wxDataViewCtrl base, and port CServerListCtrl onto it (#180, #801) - #811

Merged
got3nks merged 2 commits into
amule-org:masterfrom
got3nks:feat/dataview-base
Aug 5, 2026
Merged

feat(gui): shared wxDataViewCtrl base, and port CServerListCtrl onto it (#180, #801)#811
got3nks merged 2 commits into
amule-org:masterfrom
got3nks:feat/dataview-base

Conversation

@got3nks

@got3nks got3nks commented Aug 5, 2026

Copy link
Copy Markdown

Answers the open question on #801 and ports the first list onto the result. Supersedes #807, whose port informed this one.

#801 asked whether the remaining lists should each be ported independently or share a base, and deferred deciding until the shape was clear from more than one list. #807 made it clear: it reimplemented type-to-select, select-all, the sort chain, the column-store adapter and the notification pattern from scratch, and — having been cut a few hours before #805 landed — arrived without the column show/hide menu, hidden-state tracking, the macOS trailing-column fix or the shifted navigation keys. A third port would have repeated it.

CMuleDataViewCtrl is the wxDataViewCtrl counterpart of CMuleListCtrl: column widths and 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, drag-resize detection. It owns no data — a list supplies rows via GetDisplayOrder(), a label via GetRowLabel(), and ordering via CompareByColumn().

CMuleVirtualDataViewCtrl is the counterpart of CMuleVirtualListCtrl, virtual in the same sense: rows addressed by index through a wxDataViewIndexListModel, nothing materialised per row. It carries the item-identity bookkeeping an identity-addressed model avoids — a wxDataViewItem from a row-addressed model encodes the row number, so a deletion silently retargets any item held across it. Verified with a standalone probe: selecting rows 1 and 3, deleting row 0, then re-reading the selection yields two different objects. Everything here therefore 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 interacts, bulk append, batch removal via RowsDeleted, and icon columns.

CSearchListCtrl moves onto the plain base and loses 499 lines.

CServerListCtrl is the first list on the virtual base. Its public API is unchanged, so ServerWnd, GuiEvents and the remote GUI needed nothing — the base offers the same AddItemData/RemoveItemData/RefreshItemData vocabulary as before. Removals notify the control before the core frees the server rather than deferring to idle: the row's identity is the object, so a repaint, a resort or a menu handler reading the selection in between would be reading freed memory (confirmed under ASAN on wxGTK 3.2.9 against the deferred variant). Live re-sort on Ping/Users/Files stays gated by thePrefs::LiveListSort(), and the TCP/UDP flag columns stay hidden by default in release builds — now through the header menu's hidden state rather than a zero width.

Testing

Interactive pass on macOS, Ubuntu 26.04 arm64 (wxGTK 3.2.9) and Windows 11 arm64: both lists' sorting and its persistence, live re-sort, context menus, multi-select, delete, flags, the connected-server highlight, the column menu, type-to-select and select-all. amule and amulegui built separately on each. clang-format and both clang-tidy tiers clean.

Three bugs found only by running it, none visible to any gate: the search list's event table still named wxDataViewCtrl as its parent, so wx skipped the base's table entirely and EVT_IDLE never ran — no results appeared at all; the virtual model inverted descending sorts twice, so only the first header click did anything; and WXK_DELETE (127) sits below WXK_START, so the delete key fell through to type-ahead.

Follow-up, deliberately not here

The country-flag icon cache is per-list, so Sources and Peers would each keep their own once ported. It belongs on CCountryFlags beside the existing wxImage cache — left out because it touches a shared class after the testing above was done.

got3nks added 2 commits August 5, 2026 23:56
amule-project#796 ported the search list to wxDataViewCtrl and amule-project#805 fixed what that
port left behind. amule-project#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.
…mule-project#801)

The first list onto CMuleVirtualDataViewCtrl, and what makes it more
than dead code. Supersedes the port in amule-project#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.
@got3nks
got3nks merged commit 86c84e1 into amule-org:master Aug 5, 2026
15 checks passed
@got3nks
got3nks deleted the feat/dataview-base branch August 5, 2026 22:09
got3nks pushed a commit that referenced this pull request Aug 7, 2026
…) (#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.
got3nks added a commit that referenced this pull request Aug 7, 2026
…#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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant