fix(gui): shrink the per-network connect button and name its network - #683

@LSalami I think this button does the same thing as connect and it can be removed too (if you didn't already)

Another improvement could be this. In ed2k tab we have this after the table

In the kad tab we could have a similar layout and use more space horizontal space for the graph. Also have the IP address in jut one field. It's easier to copy than 4 separated fields...

I think this button does the same thing as connect and it can be removed too

Confirmed and removed -- OnBnClickedBootstrapKnown called the exact same theApp->StartKad() the Connect toggle already calls when off, so it was a second control for the same action. Dropped the button, its event handler, and the ID; po/*.po regenerated to drop the now-unused strings.

Another improvement could be this [...] use more space horizontal space for the graph. Also have the IP address in jut one field.

Good idea, but I'd rather keep it out of this PR -- it's a real layout redesign (graph sizing, collapsing the 4-field IP entry into one, matching your WebUI reference) rather than a fix to what's already here, and this PR is getting long enough as review feedback accumulates. Happy to pick it up as a follow-up once this one's settled, or if you'd rather open a tracking issue for it that works too.

Rebuilt clean, clang-format v18 applied, po regenerated, pushed.

Reviewed and built this on three platforms at d8b247639 — macOS ARM64 (wx 3.3.2), Ubuntu ARM64 (wxGTK3 3.2.9) and Windows ARM64 (MSYS2 CLANGARM64), monolithic. All clean: Linux zero errors and six pre-existing wxGTK header warnings, Windows zero warnings.

Both removals verified. ID_BUTTONCONNECT really was inert — on master it appears only in the AddTool call and the #define, with no EVT_TOOL, no EVT_MENU, no dynamic Bind. One caution on the reasoning: "no EVT_TOOL binding" isn't sufficient on its own, since wxWidgets aliases wxEVT_TOOL to wxEVT_MENU and amuleDlg.cpp deliberately double-binds every other toolbar ID both ways — an EVT_MENU-only binding would have slipped past that check. ID_KNOWNNODECONNECT is a genuine alias as you found, and ID_NODECONNECT → BootstrapKad(ip, port) is correctly left alone.

Please fold the Bootstrap removal into the PR body. It's explained in the thread, but the body's "Dead code removal" section still covers only the toolbar button, and the body is what survives as the record for whoever reads this later.

The SetBitmapMargins() comment is wrong about Windows, which matters because it's the stated reason for the leading-space trick. DoSetBitmapMargins is overridden in wx/msw/anybutton.h as well as wx/osx/anybutton.h — wxGTK is the only port that inherits the base-class no-op. The visible consequence, comparing the built binaries side by side: the icon-to-label gap on Windows differs noticeably from Linux and macOS, because the spacing is the toolkit's own margin (native BUTTON_IMAGELIST placement on MSW, none at the wx level on GTK, wxOSX's own on Mac) plus a space glyph whose width follows the platform button font. The approach can't produce a consistent gap by construction. Suggest SetBitmapMargins() on the two ports that implement it, keeping the space or GTK-side padding only for GTK — which also drops the "space outside the translatable string" trick, which quietly depends on no translator trimming leading whitespace.

On the Kad tab layout: I'm with ngosang, and I'd like it done rather than deferred to a tracking issue. Concretely, KadDlg's item1 is a two-column wxFlexGridSizer with the graph in column 0 and the Bootstrap box in column 1. Dropping that and stacking vertically gives the graph the full width, with the Bootstrap / New node form as a full-width row underneath the legend labels — which is exactly the shape serverListDlgUp already has on the ED2K side (top row, full-width list, then the horizontal Name / IP / Port row). Worth doing the single IP field in the same pass: ED2K uses one IDC_IPADDRESS control, Kad splits it across ID_NODE_IP1..4, and collapsing it also simplifies OnBnClickedBootstrapClient, which currently re-joins the four fields with dots before StringIPtoUint32. If you'd rather not grow this PR further, a follow-up PR straight after is fine — but let's not park it as an issue.

Two more things to fix in this PR. ScaledConnButImg hard-codes 16x16 physical pixels, so a HiDPI display discards the 32x32 source that would otherwise serve as the 2x representation — please scale with FromDIP(16) instead. It would be the first use of FromDIP in the tree, which is fine; the icon is new code and shouldn't ship blurry on Retina and high-DPI Windows. And please cache the scaled bitmaps: the ConvertToImage + Rescale(wxIMAGE_QUALITY_HIGH) currently runs on every SetConnectButtonState call, i.e. on every connection-state update, and index 2 (connecting) is already 16x16 so it round-trips for nothing. Three statics covering the three states would do it.

All four addressed.

SetBitmapMargins(): you're right, the comment had it backwards. Now calls SetBitmapMargins() on wxMSW/wxOSX (both override DoSetBitmapMargins) and keeps the leading-space trick only for wxGTK, the actual no-op port.

Icons: ScaledConnButImg now scales via FromDIP and caches the three resulting bitmaps as statics -- computed once, not on every SetConnectButtonState call.

Kad tab redesign: done -- dropped the two-column layout for the same vertical shape serverListDlgUp uses (full-width graph, then a full-width bootstrap row below), and collapsed ID_NODE_IP1..4 into one field matching the eD2k tab's IDC_IPADDRESS.

Bootstrap removal: folded into the PR body now, alongside the toolbar button.

One thing worth flagging since it's exactly the kind of thing you asked me to watch for on the last round: the first version of the Kad redesign had a real crash bug. Switching to the Kad tab reproducibly killed amule.exe inside COMCTL32.dll (access violation) every single time. Caught it by actually running the build and clicking over to the tab rather than trusting a clean compile.

Root-caused by stashing the change and comparing: the pre-redesign build switches to Kad fine, the redesign crashed 100% of the time. The cause was sizer restructuring, not the single-IP-field change or the tooltip on it (both cleared as suspects first) -- replacing the wxFlexGridSizer that used to wrap the nodes-stats graph with a direct item0->Add() changed the wrapping depth the graph's OScopeCtrl gets its first layout pass through, and something in that first layout/paint sequence blew up inside comctl32. Fixed by keeping the original wrapping (a plain wxBoxSizer around the graph's wxStaticBoxSizer, same nesting as before) and only removing the second column, rather than flattening both levels at once. Re-verified clean afterward: switches tabs, populates, no crash, confirmed against Windows Event Log for Application Error entries before and after.

Rebuilt clean, clang-format v18 applied (the real CI image this time), po regenerated, rebased onto current master, force-pushed.

All four review points are addressed, and I re-tested the result on three platforms at ec13cc556 — macOS ARM64, Ubuntu ARM64 and Windows ARM64, monolithic, all clean (Windows with zero warnings). The visual check looks good: the graph now uses the full tab width, the bootstrap row beneath it mirrors the ED2K tab, and the single IP field is a clear improvement.

I checked the IP-parsing rewrite for behavioural equivalence rather than taking the comment's word: the old code read fields 1→4 and built d.c.b.a, and the new split rebuilds the same string from octets[3..0], so StringIPtoUint32 receives identical input. Malformed input leaves ip = 0 and falls into the existing "Invalid ip to bootstrap" branch.

This needs a rebase before it can go in. Master moved after your last one — #689 landed and touched src/muuli_wdr.cpp along with all the catalogs, so this now shows as conflicting. Could you rebase onto current master and re-run ./scripts/update-po.sh? CI was green on ec13cc556, so I'd expect it back green once the conflicts clear.

Since you'll be pushing again anyway, it'd be good to fold these three in rather than leaving them for a follow-up:

Drop the explicit SetBitmapMargins() on wxMSW and keep it wxOSX-only. The icon gap still reads differently on Windows than on macOS, because the same value means different things per port: wxOSX stores it as the margins around the bitmap, so 4 px lands on both sides including between icon and button edge, while wxMSW applies it between bitmap and label only. On top of that, the Windows default being overridden is font-derived — m_margin.x = GetCharWidth(); m_margin.y = GetCharHeight() / 2; in src/msw/anybutton.cpp — so (4, 0) both narrows the gap and drops the vertical margin to zero, shrinking the button's best height and moving it away from standard Windows button metrics rather than toward them. wx's default is already the native convention there, so the explicit call is only earning its keep on wxOSX. (I checked NeedsOwnerDrawnForImageLayout as well — it only forces owner-drawn mode for wxRIGHT/wxBOTTOM bitmap positions, so the default left-aligned bitmap keeps the native themed control either way.)

Key the bitmap cache by DPI. static wxBitmap cache[3] is filled from whichever button triggers it first, with no wxEVT_DPI_CHANGED invalidation, so a window dragged between differently-scaled monitors on Windows keeps the stale size. The comment documents this, but with per-monitor DPI v2 it's reachable in normal use.

Trim whitespace on the pasted IP. The single field's whole point is easier copy-paste, and a paste carrying a leading or trailing space currently fails with "Invalid ip to bootstrap"; Trim(true).Trim(false) before the split covers it. Minor related note: wxSplit's third parameter defaults to \\ as an escape character — harmless for IPs, but non-obvious to a future reader.

The rest looks good as it stands — ping me when it's rebased and I'll merge.

All three folded in, plus the rebase.

Rebased onto current master (both the #689 and #691 conflicts -- master moved again mid-review -- are resolved; no manual conflicts left in the source, only the usual po/ catalog churn from the regen).

SetBitmapMargins() is now wxOSX-only. Good catch on the per-port semantics -- wxMSW keeps its own native, font-derived default (GetCharWidth()/GetCharHeight()) completely untouched; only wxOSX gets the explicit (4, 0) override, since that's the port where it's actually a margin-around-the-bitmap fix rather than a mismatch with the native convention. wxGTK still gets the leading-space trick, unchanged.

Icon cache is now keyed by physical size, not just state. ScaledConnButImg's cache is a std::map<int, wxBitmap> per state, keyed by the FromDIP-resolved pixel size -- so a window dragged to a differently-scaled monitor gets a freshly-rescaled bitmap for that size instead of reusing whatever was cached from wherever it was first computed. No wxEVT_DPI_CHANGED handler needed; the size-keyed cache means there's nothing stale to invalidate.

Pasted IP is now trimmed. Trim(true).Trim(false) before the wxSplit, so a copy-paste carrying leading/trailing whitespace no longer trips "Invalid ip to bootstrap".

Rebuilt clean, clang-format v18 applied (the real CI image), po regenerated, force-pushed.

Merged as 09f1b0546 — thanks for the quick turnaround across four passes, and for digging into the wx internals on the margin question rather than just taking my word for it.

Screenshot of the new UI for @ngosang 's review

Thank you both. It looks muuuch better.