Skip to content

StatTree: guard total ratio against div-by-zero on fresh installs (#288) - #585

Merged
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/stattree-ratio-divide-by-zero
May 11, 2026
Merged

StatTree: guard total ratio against div-by-zero on fresh installs (#288)#585
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/stattree-ratio-divide-by-zero

Conversation

@got3nks

@got3nks got3nks commented May 11, 2026

Copy link
Copy Markdown
Contributor

Why

CStatTreeItemRatio::GetString (src/StatTree.cpp) computes a session ratio v1/v2 guarded by v1 > 0 && v2 > 0, then appends an all-time ratio computed from the totals t1/t2.

Issue #288's 2022 fix (commit 9b8b60f, authored by the OP @SevC10) correctly moved the total-ratio block out of the session-only branch so it shows even when the session is "Not available" — but didn't carry the corresponding t1 > 0 && t2 > 0 guard over.

Result: on a brand-new install (or any state where the persisted totals are still zero), the total-ratio path divides by zero and the UI surfaces literal (1 : nan) or (1 : inf) text. The original issue body explicitly noted this requirement ("if DL>0 and UL>0").

What

Add the missing guard inside the existing if (m_totalfunc1 && m_totalfunc2) block:

if (t1 > 0 && t2 > 0) {
    if (t2 < t1) {
        ret += CFormat(" (%.2f : 1)") % (t1 / t2);
    } else {
        ret += CFormat(" (1 : %.2f)") % (t2 / t1);
    }
}

Inline rather than gating the whole block, so the m_totalfunc1 && m_totalfunc2 callability check stays at the outer level and a future caller with one zero side still hits the early-out cleanly.

Test

  • macOS Apple Silicon: amule + amuled compile clean.
  • Fresh ~/.aMule start (no upload/download history yet): statistics tree shows Not available for the U/D ratio with no NaN/inf appended.

Fixes the remaining edge case from #288.

CStatTreeItemRatio::GetString computes a session ratio (v1/v2 or v2/v1)
guarded by `v1 > 0 && v2 > 0`, then appends an all-time ratio computed
from the totals t1/t2. Issue amule-project#288's original 2022 fix (commit
9b8b60f) correctly moved the total-ratio block out of the
session-only branch so it shows even when the session is "Not
available", but it didn't carry the corresponding `t1 > 0 && t2 > 0`
guard over.

Result: on a brand-new install (or any case where the persisted
totals are still 0) the total-ratio path divides by zero and the UI
surfaces literal "(1 : nan)" or "(1 : inf)" text. The original issue
body explicitly noted this requirement ("if DL>0 and UL>0").

Add the missing guard. Inline rather than gating the whole block so a
future caller with one zero side still hits the early-out, and so the
m_totalfunc1 && m_totalfunc2 callability check stays at the outer
level.

Build: amule + amuled compile clean on macOS Apple Silicon.
@mrjimenez
mrjimenez merged commit d9eb5d1 into amule-project:master May 11, 2026
12 checks passed
@got3nks
got3nks deleted the fix/stattree-ratio-divide-by-zero branch May 12, 2026 08:15
ngosang added a commit to ngosang/amule that referenced this pull request Jul 24, 2026
…mule-project#585)

Relocate the global connect/disconnect toggle out of the app toolbar
(and its mobile drawer copy) and into the Networks view, right-aligned
on the ED2K/Kad tab strip (see amule-org#402). Behaviour is
unchanged: same both-networks toggle, coloured plug states, confirm
dialog, and admin-only gating.

- components.js: Tabs gains an optional right-aligned `extra` slot,
  rendered inside the tab strip so it shares the bottom border line.
- networks.js: owns ConnectButton now (reads status via useStore) and
  passes it as `extra` to the top tabs.
- app.js: remove both toolbar ConnectButton renders, the component
  itself, the now-unused status var and confirmDialog import; the Kad
  status-bar default now reads "Not connected" to match ED2K.
- app.css: drop the three dead toolbar/drawer .conn-btn rules; add
  .tabs-extra styling so the button reads as a compact button on the
  tab strip.
- i18n (en/es): uppercase app_ed2k label; relabel the per-network
  buttons to "Disconnect ED2K", "Connect from known clients" and
  "Disconnect Kad".
ngosang pushed a commit to ngosang/amule that referenced this pull request Jul 28, 2026
…-project#663)

* feat(gui): remove the global Connect/Disconnect toolbar button

Closes amule-project#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 amule-project#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 amule-project#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 amule-project#642/amule-project#643 --
no source changes here. Resolved a real conflict in amuleDlg.cpp's
event table: amule-project#642 added the Alt+<letter> EVT_MENU block right where
this branch removed the EVT_TOOL(ID_BUTTONCONNECT, ...) line; kept
amule-project#642's block, dropped the connect-button line as intended.

Regenerated again via scripts/update-po.sh after later rebasing onto
master's amule-project#671 (Preferences sidebar) and amule-project#665/amule-project#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 amule-project#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 amule-project#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 amule-project#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