Skip to content

feat(gui): port the Preferences sidebar to wxDataViewCtrl (#180 phase 1) - #671

Merged
got3nks merged 2 commits into
amule-org:masterfrom
LSalami:prefs-sidebar-dataviewctrl
Jul 28, 2026
Merged

feat(gui): port the Preferences sidebar to wxDataViewCtrl (#180 phase 1)#671
got3nks merged 2 commits into
amule-org:masterfrom
LSalami:prefs-sidebar-dataviewctrl

Conversation

@LSalami

@LSalami LSalami commented Jul 28, 2026

Copy link
Copy Markdown

Phase 1 of #180 (Preferences sidebar).

What changed

m_PrefsIcons is now a wxDataViewListCtrl (single icon+text column, no tree mode — the sidebar is a flat list today; tree mode is future work for subcategories, not part of this port) instead of a plain wxListCtrl. wxDataViewCtrl is native on GTK and macOS (wxHAS_NATIVE_DATAVIEWCTRL), so VoiceOver/Orca can navigate it directly; on Windows it falls back to the generic wx-drawn control, i.e. the same as before per got3nks's correction on the #180 thread.

Bug fix along the way

OnPrefsPageChange used to derive both the current wxPanel* and the pages[] lookup from the row's live position in the sidebar (event.GetIndex()), which drifts whenever the Server or IP2Country row is hidden/re-shown — a comment already flagged this had bitten the reset-button-visibility check once. Each row's item data is now the page's stable pages[] array index, set once at insertion and never derived from position; a new m_pageWidgets vector (indexed by that same stable index) replaces the old SetItemPtrData(widget) lookup. EnableServerTab's insert-at-m_IndexServerTab positional logic is unchanged — it was already correct for that purpose, just fragile for the different purpose OnPrefsPageChange was reusing it for.

Sizing

Column width is computed by measuring actual (translated) label text extents rather than relying on wxDataViewColumn's own auto-size timing, which isn't reliably immediate across native vs generic backends.

Testing

Built and ran both amule and amuleGUI on macOS, full unit test suite passing (28/28). Visually confirmed (by a human, not just me — I can't reliably screenshot this myself in this environment) that:

  • Every page label renders in full (an early version clipped the longest ones — fixed by widening the padding budget)
  • Page switching between pages works correctly
  • The Server tab correctly disappears/reappears at the right position when toggling the ED2K network on and off

Still needed before merge-ready: Windows and Linux screenshots, per your stated requirement that visual parity is a merge gate for each phase. I only have macOS available directly, but should have Windows coverage shortly.

@got3nks

got3nks commented Jul 28, 2026

Copy link
Copy Markdown

Linux

immagine

Windows

immagine

MacOS

immagine

@got3nks

got3nks commented Jul 28, 2026

Copy link
Copy Markdown

Reviewed the code and built it on all three platforms — macOS (wx 3.3.3), Ubuntu ARM64 (wxGTK 3.2), Windows ARM64 (wx 3.2, portable install). Clean build, zero errors on each.

Visual parity: covered. Preferences renders correctly on all three, with no clipping or ellipsis on any label — including the longer Italian ones ("Controlli remoti", "Contatti/emoticons") and English "Online Signature" / "Remote Controls". So the hand-computed column width holds up on both native backends and on the generic one.

The three do look noticeably different from each other, which is expected rather than something to chase: dataview.h enables the native control only for __WXGTK20__ and __WXOSX__, so macOS gets NSTableView (inset, rounded selection), GTK gets GtkTreeView (full-width themed bar), and Windows falls back to wxHAS_GENERIC_DATAVIEWCTRL. Divergent per-platform look is the price of the native control, and the native control is the entire point of #180 — so this is the right trade.

The bug fix is correct. The old pages[event.GetIndex()] indexed the static array by a live row position, which genuinely breaks once a row is hidden. Keying off the stable pages[] index stored as item data fixes both that and the widget lookup.

I also checked the claim that EnableServerTab's positional insert is still sound, rather than taking it on trust: Servers precedes IP2Country in pages[], and the IP2Country row is deleted before EnableServerTab runs, so no removable row ever sits ahead of the server row and m_IndexServerTab stays valid as a row position. m_pageIcons / m_pageWidgets also stay index-aligned — every branch of the icon if/else pushes exactly once, including with GEOIP_GUI undefined.

Two small things worth changing:

1. Guard the invalid item in OnPrefsPageChange. The control swap quietly changed the event contract: EVT_LIST_ITEM_SELECTED only ever fired when something became selected, whereas EVT_DATAVIEW_SELECTION_CHANGED also fires when the selection is cleared, and event.GetItem() is then invalid. I confirmed with a standalone wx program that GetItemData() on an invalid item crashes outright (SIGBUS) rather than returning a harmless value.

Nothing in the dialog currently clears the selection — the only mutations are SelectRow, AppendItem, InsertItem, DeleteItem — so this is hardening rather than a live bug. Worth having anyway because it's one line and because the backends diverge here: deleting the selected row fired no event at all on macOS, while GTK commonly auto-selects the next row instead.

if (!event.GetItem().IsOk()) { return; }

2. Measure with the sidebar's font, not the dialog's. GetTextExtent(label) runs on the dialog, so it uses the dialog font, while the label is drawn by m_PrefsIcons — which on a native backend often uses a different one. m_PrefsIcons->GetTextExtent(label) is the correct measurement, and it's part of why the padding budget has to be as generous as it is. On GTK there's a visible band of dead space to the right of the labels; tightening the measurement would let kSidebarPadding come down from icon + 48 and be less of a magic number.

With those two in, this is good to merge from my side.

@LSalami

LSalami commented Jul 28, 2026

Copy link
Copy Markdown
Author

Visually confirmed on Windows (MSYS2/MinGW-w64 build, wxWidgets 3.2.10, monolithic amule.exe):

  • All sidebar labels (General, Connection, Directories, Servers, Files, Security, Interface, IP2Country, Statistics, Proxy, Filters, Remote Controls, Online Signature, Advanced, Events) display in full — no "..." truncation.
  • Clicking each sidebar item correctly swaps the right-hand panel.
  • Toggling ED2K off/on in the Connection tab's Networks section makes the "Servers" entry disappear/reappear in the correct position (between Directories and Files).

(Screenshot of the Preferences sidebar taken locally, showing the full item list rendered without truncation on the General panel — happy to attach it directly if useful, just let me know a preferred way to share it.)

LSalami and others added 2 commits July 28, 2026 17:41
 phase 1)

Phase 1 of the accessibility work in amule-org#180: the Preferences sidebar was
a plain wxListCtrl, invisible/unusable to screen readers on macOS
(VoiceOver) and Linux (Orca) -- wxDataViewCtrl is native on both those
platforms (wxHAS_NATIVE_DATAVIEWCTRL for __WXGTK__/__WXOSX__), falling
back to the generic wx-drawn control only on Windows, where the answer
was already "works the same as before" per got3nks's review.

m_PrefsIcons is now a wxDataViewListCtrl (single icon+text column, no
tree mode -- the sidebar is a flat list today, per the earlier
correction to the amule-org#180 thread; tree mode is future work for
subcategories, not part of this port).

Along the way, fixes the positional-index bug flagged in the original
amule-org#180 phasing proposal: OnPrefsPageChange used to derive both the
current wxPanel* and the pages[] lookup from the row's live *position*
in the sidebar (event.GetIndex()), which drifts whenever the Server or
IP2Country row is hidden/re-shown -- a comment already noted this had
bitten the reset-button-visibility check once. Each row's item data is
now the page's stable pages[] array index instead, set once at
insertion and never derived from position; m_pageWidgets (indexed by
that same stable index) replaces the old SetItemPtrData(widget)
lookup. EnableServerTab's insert-at-m_IndexServerTab positional logic
is otherwise unchanged -- it was already correct for that purpose,
just fragile for the *different* purpose OnPrefsPageChange was
(mis)reusing it for.

Column width is computed by measuring actual (translated) label text
extents rather than relying on wxDataViewColumn's own auto-size timing,
which isn't reliably immediate across native vs generic backends.

Verified: built and ran both amule and amuleGUI on macOS. Visually
confirmed (by a human, not just me, since I can't reliably screenshot
this myself) that every page label renders in full (an early version
clipped the longest ones -- fixed by widening the padding budget),
page switching works, and the Server tab correctly disappears/
reappears at the right position when toggling the ED2K network on and
off. Windows and Linux screenshots still needed before this is
merge-ready, per got3nks's stated requirement -- see PR description.
EVT_DATAVIEW_SELECTION_CHANGED also fires when the selection is
cleared, unlike the old EVT_LIST_ITEM_SELECTED, and GetItemData() on
an invalid item crashes outright. Also measure the sidebar label width
with m_PrefsIcons's own font instead of the dialog's, since the two
can differ on native backends.

Addresses review feedback from got3nks on amule-org#671.

Co-Authored-By: Claude Sonnet 5 <[email protected]>
@LSalami
LSalami force-pushed the prefs-sidebar-dataviewctrl branch from 4056c67 to 3ebc530 Compare July 28, 2026 15:47
@LSalami

LSalami commented Jul 28, 2026

Copy link
Copy Markdown
Author

Pushed both requested changes (3ebc530): guarded the invalid-item case in OnPrefsPageChange, and switched the label-width measurement to m_PrefsIcons->GetTextExtent(). Rebased on current master, rebuilt clean on Windows (MSYS2/MinGW-w64), Preferences still behaves as verified above.

@got3nks
got3nks merged commit 8eb54b3 into amule-org:master Jul 28, 2026
13 checks passed
@LSalami
LSalami deleted the prefs-sidebar-dataviewctrl branch July 28, 2026 16:28
LSalami added a commit to LSalami/amule that referenced this pull request Jul 28, 2026
Mechanical rebase to resolve conflicts against master's amule-org#642/amule-org#643 --
no source changes here. Resolved a real conflict in amuleDlg.cpp's
event table: amule-org#642 added the Alt+<letter> EVT_MENU block right where
this branch removed the EVT_TOOL(ID_BUTTONCONNECT, ...) line; kept
amule-org#642's block, dropped the connect-button line as intended.

Regenerated again via scripts/update-po.sh after later rebasing onto
master's amule-org#671 (Preferences sidebar) and amule-org#665/amule-org#662 (amuleapi
credentials, sparse part-file setting), which had added strings this
branch's catalogs didn't have yet.
LSalami added a commit to LSalami/amule that referenced this pull request Jul 28, 2026
Mechanical rebase to resolve conflicts against master's amule-org#642/amule-org#643 --
no source changes here. Resolved a real conflict in amuleDlg.cpp's
event table: amule-org#642 added the Alt+<letter> EVT_MENU block right where
this branch removed the EVT_TOOL(ID_BUTTONCONNECT, ...) line; kept
amule-org#642's block, dropped the connect-button line as intended.

Regenerated again via scripts/update-po.sh after later rebasing onto
master's amule-org#671 (Preferences sidebar) and amule-org#665/amule-org#662 (amuleapi
credentials, sparse part-file setting), which had added strings this
branch's catalogs didn't have yet.
got3nks pushed a commit that referenced this pull request Jul 28, 2026
* feat(gui): remove the global Connect/Disconnect toolbar button

Closes #402. Per-network controls already exist (Kad pane's Start/Stop,
Servers pane's ED2K connect/disconnect), the connection state is
already shown in the status bar, and after discussing relocating it
into the Networks view the agreed call (got3nks + ngosang) was simply
to remove it -- mirrors what #585 already did on the Web UI side.

Removed the toolbar tool, its three skin icons (Toolbar_Connect/
Disconnect/Connecting) and their bitmap wiring, the button-update block
in ShowConnectionState(), and the two EnableTool(ID_BUTTONCONNECT, ...)
call sites.

CamuleDlg::OnBnConnect() itself stays -- CMuleTrayIcon::DoConnectDisconnect()
still calls it for the tray icon's own connect/disconnect action, which
is unaffected by this change.

ShowConnectionState()'s skinChanged parameter was only ever read by the
now-removed block, so it's gone too, along with the one call site that
passed true and the forceUpdate plumbing that reached it through
GuiEvents.cpp's ShowConnState() free function.

Left ID_BUTTONCONNECT and muuli_wdr.cpp's muleToolbar() (which still
calls it) alone -- that function predates Apply_Toolbar_Skin, is never
called anywhere, and touching pre-existing dead code is out of scope
for this fix.

Verified: built and ran both amule and amulegui cleanly, full unit
test suite passing (26/26). Confirmed via the po/ diff that "Connect"/
"Disconnect"/"Cancel" remain in the catalog (still used by other
per-network buttons) -- only their now-orphaned toolbar-specific
tooltip strings were dropped. Could not get a real on-screen visual
confirmation of the toolbar layout -- same macOS Accessibility
automation limitations as #180 got in the way of screenshotting the
actual app window.

* i18n: regenerate po/ catalogs after rebasing onto current master

Mechanical rebase to resolve conflicts against master's #642/#643 --
no source changes here. Resolved a real conflict in amuleDlg.cpp's
event table: #642 added the Alt+<letter> EVT_MENU block right where
this branch removed the EVT_TOOL(ID_BUTTONCONNECT, ...) line; kept
#642's block, dropped the connect-button line as intended.

Regenerated again via scripts/update-po.sh after later rebasing onto
master's #671 (Preferences sidebar) and #665/#662 (amuleapi
credentials, sparse part-file setting), which had added strings this
branch's catalogs didn't have yet.

* feat(gui): turn the per-network Disconnect buttons into Connect/Cancel/Disconnect toggles

Relocates the removed global toolbar button's functionality into the
ED2K (IDC_ED2KDISCONNECT) and Kad (ID_KADDISCONNECT) panes instead of
just dropping it, per got3nks's proposal on #663: each button now
mirrors ed2kState/kadState (off/connecting/connected) using the
existing connButImg() bitmaps, and can independently connect or
cancel/disconnect its own network -- something the old OR-toggled
global button never allowed. The ED2K pane also gains a Connect
button it never had (previously Disconnect-only).

CServerWnd::UpdateED2KConnectButton() / CKadDlg::UpdateConnectButton()
are called from CamuleDlg::ShowConnectionState() alongside the
existing UpdateED2KInfo()/UpdateKadInfo() calls, and once from each
pane's own ctor/Init() for the initial paint. OnBnClickedED2KDisconnect
and OnBnClickedDisconnectKad gained the missing "connect when off"
branch; OnBnConnect and the tray icon are untouched.

Verified interactively on Windows: both buttons show the correct
label/icon for their live state, ED2K disconnect/reconnect works from
its own button, and disabling ED2K in Preferences while Kad stays
connected confirms the two toggle independently.

po/ regenerated via scripts/update-po.sh to match the new button
labels ("Disconnect Kad" is now unused and moves to the obsolete
section; no new translatable strings).

* fix(gui): drop the per-network connect-button state cache

got3nks flagged two bugs in review (PR #663): the static cache's
early return also skipped Enable(), so the button could get stuck
disabled once IsReady()/network-pref state changed without the
Connect/Cancel/Disconnect state itself changing (master had two now-
deleted escape hatches -- the skinChanged force-path and an
unconditional EnableTool call -- that used to paper over this). Worse,
the cache was function-scope static, surviving widget recreation, so
a fresh pane's Init() call could early-return and leave the wxDesigner
default label ("Disconnect Kad") on screen.

Dropping the cache removes both: SetLabel/SetBitmap/Enable are cheap
on a plain wxButton (unlike the old toolbar tool, which had real
native-image-list reasons to memoize, see amuleDlg.cpp:1054-ish), and
master's own second call site already did an unconditional EnableTool
every tick without issue.

Second review round (macOS + Ubuntu ARM64, monolithic + amuleGUI)
confirmed the toggle works functionally but flagged three cosmetic
issues, all fixed here:

- No gap between the connect-button icon and its label. The obvious
  fix, wxButton::SetBitmapMargins(), is NOT portable (wxOSX overrides
  it, wxGTK inherits the base no-op) -- prefixing the label with a
  literal space outside the _() call behaves identically on both and
  introduces no new msgid.
- The Kad button was stretched full-width by its sizer's .Expand()
  flag, while the ED2K one sizes naturally; GTK and macOS then filled
  the slack differently (centered vs. icon-left). Dropped .Expand()
  so the two buttons size the same way -- trades away lining up with
  the "Bootstrap from known clients" button above it, but the two
  connect buttons reading as the same control matters more.
- CServerWnd::UpdateED2KConnectButton() and CKadDlg::UpdateConnectButton()
  were near-identical (same 3-state enum, same switch, same three
  bitmaps). Factored into a shared SetConnectButtonState(button, state,
  enabled) helper in muuli_wdr.cpp/.h, next to connButImg() which it
  reuses -- so the icon-spacing fix above lives in one place instead
  of two.

Verified interactively on Windows: re-tested full disconnect/reconnect
cycle on both panes, toggling ED2K off and back on via Preferences
(existing behavior: needs an app restart to re-enable) with correct
state on the fresh post-restart paint, and confirmed by screenshot
that both buttons now show a visible icon/label gap and size the same
way (Kad no longer stretched). Also confirmed amulegui (CLIENT_GUI)
builds clean with these changes; did not perform full interactive
remote-daemon testing.

* fix(gui): first-paint layout bug + connect-button placement, per review

Two more issues from got3nks's Linux/GTK pass on #663.

1. Bug: the ED2K connect button painted with the icon overlapping the
   label on first show (Kad was fine, only by luck of timing).
   CServerWnd::UpdateED2KConnectButton() runs after sizer->Show(this,
   TRUE) in the ctor, so SetBitmap() grows the button's best size
   after the row was already measured against the text-only label,
   and nothing re-lays it out until a window resize. Fixed by calling
   Layout() on the button's parent at the end of the shared
   SetConnectButtonState() helper -- covers both panes, and stops Kad
   from relying on being laid out late (its notebook page is only
   measured once shown, after Init() already set its bitmap).

2. Layout: moved both connect/disconnect toggles to lead their pane
   instead of sitting among secondary controls, per got3nks's request:
   - ED2K (serverListDlgUp): toggle now on its own top row, right-
     aligned via a stretch spacer, directly above the server list.
     The "Add server manually" form (name/IP/port/Add) moved below
     the table instead of sharing a row with the toggle; the vertical
     separator that used to sit between them is gone (no longer
     needed once they're not adjacent).
   - Kad (KadDlg): toggle moved out of the "Bootstrap" static box
     (where it sat under "Bootstrap from known clients") to its own
     top row above the box, right-aligned the same way as ED2K's.
     item0 changed from a 1x1 FlexGridSizer (which only ever held the
     two-column area) to a plain vertical wxBoxSizer so it can hold
     the new top row plus the existing two-column layout.

Both panes now read "primary control on top, secondary/manual
controls below" -- matching shape on both tabs.

Verified interactively on Windows: screenshotted both panes, confirmed
no icon/label overlap on first paint (no window resize needed), and
re-ran the disconnect/reconnect toggle cycle on both after the layout
change.
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.

2 participants