chore(gui): delete unreachable bitmap functions/entries from muuli_wdr.cpp (#675) - #725
Conversation
5c5fc30 to
6819ce4
Compare
…r.cpp First slice of the icon-system cleanup scoped in amule-org#675: remove code with zero call sites anywhere in the tree, before any wxArtProvider migration work starts. - muleToolbar(): whole function unused -- superseded by the main wxToolBar setup in amuleDlg.cpp; nothing calls it. - moreImages(): whole function unused, both of its two icon entries. - amuleDlgImages(): 21 of 35 index blocks have no caller anywhere (0-13, 16, 17, 19, 27, 28, 31, 34). The 14 live ones are untouched -- 10 of those (20-26, 29, 32, 33) are already the fallback path inside amuleDlg.cpp's Add_Skin_Icon, which prefers a wxArtProvider/SVG lookup first; the other 4 (14, 15, 18, 30) are still called directly. - amuleSpecial(): 6 of 26 index blocks have no caller (6, 7, 8, 9, 18, 20) -- checked both direct call sites and the PrefsUnifiedDlg.cpp fallback table (pages[].m_imageidx), which uses neither. convert_xpm in PartFileConvertDlg.cpp was on the same "no literal grep hits" list initially but is not actually dead -- SetIcon(wxICON (convert)) reaches it via the wxICON macro's token-pasting (X##_xpm), invisible to a plain identifier search. Caught by a full build failing on the undeclared identifier, not by inspection; left untouched. Deletes 1373 lines (~16% of the file). No behavior change: every touched entry was unreachable code. clang-format v18 clean; full amule build verified (macOS, CLIENT_GUI unaffected since neither touched symbol is CLIENT_GUI-only).
muleToolbar() duplicated several msgid source-location references (Networks, Searches, Downloads Window, etc.) already present via amuleDlg.cpp's own toolbar setup. Deleting it drops those now-stale #: comments; no msgid added, removed, or retranslated -- verified via unchanged msgid count (1903 before/after) and a diff limited to source-location comments and POT-Creation-Date. Regenerated after rebasing onto upstream/master to pick up po/ changes from amule-org#723/amule-org#724/amule-org#726/amule-org#728/amule-org#730/amule-org#731, which had drifted our prior regeneration out of sync.
6819ce4 to
049c655
Compare
|
Verified the deletions against the base before merging: index sets match the description exactly, no surviving index is renumbered (each block is its own One candidate for the next slice, @LSalami: Also worth carrying forward from your |
|
Good catch to double-check, but this one's live: Left it in deliberately for that reason — happy to add a one-line comment next to Thanks for the |
|
You're right, and thanks for checking rather than just deleting it. My extraction was the bug. I pulled the indices out of Worth spelling out how badly that would have gone: deleting index 25 would not have failed any build. So yes please to the one-line comment next to the block noting the |
PR amule-org#725 review: this index looked dead by the same criterion used for the ones actually removed, but it's reachable via PrefsUnifiedDlg.cpp's pages[] table -- one entry ("Debugging", PreferencesDebug) is guarded by #ifdef __DEBUG__ and breaks the Preferences<Something>Tab naming convention every other entry follows, which is exactly what made it easy to miss with a naming-pattern-based grep or a release-only build. Deleting it wouldn't have failed any build (falls through to wxNullBitmap), so the only symptom would have been a blank icon on a debug-only preferences page -- worth spelling out for the next pass over this file.
|
Added: f82285d |
PR amule-org#725 review: this index looked dead by the same criterion used for the ones actually removed, but it's reachable via PrefsUnifiedDlg.cpp's pages[] table -- one entry ("Debugging", PreferencesDebug) is guarded by #ifdef __DEBUG__ and breaks the Preferences<Something>Tab naming convention every other entry follows, which is exactly what made it easy to miss with a naming-pattern-based grep or a release-only build. Deleting it wouldn't have failed any build (falls through to wxNullBitmap), so the only symptom would have been a blank icon on a debug-only preferences page -- worth spelling out for the next pass over this file.
* feat(gui): wxFAIL_MSG on unexpected icon-bank index (#675) got3nks (PR #725 review): every raw-bitmap bank in this file (clientImages, amuleSpecial, connButImg, amuleDlgImages) fails silently -- an index with no matching block falls through to wxNullBitmap, i.e. a blank icon, with no compiler error, no link error, and nothing for CI to catch. That's exactly how an incorrectly deleted amuleSpecial(25) almost shipped in #725: it would have compiled clean in every configuration and only shown up to a human who happened to open the right dialog in a debug build. wxFAIL_MSG matches this codebase's existing assert idiom (see GenericClientListCtrl.cpp, MuleListCtrl.cpp) and is a no-op outside debug builds, so this is a debug-only safety net with no release behaviour change. Doing it now, ahead of the riskier remaining slices, matters most for clientImages: it's consumed with index arithmetic across four call sites (Client_InvalidRating_Smiley + rating - 1), so an off-by-one there is exactly this failure mode -- one blank smiley, nothing failing anywhere. The assert turns that into an immediate stop on the first debug run instead of a maybe-someday-noticed bug. Verified via a full Debug-config build (macOS, wxDEBUG_LEVEL default, not the Win32-only wxDEBUG_LEVEL=0 override) and a visual pass through every tab/dialog that reaches one of these banks (Reti, Messaggi, File condivisi, Preferenze) -- no assertion fired, confirming every index currently in use is legitimate. * docs(gui): note amuleSpecial(25)'s __DEBUG__-only consumer PR #725 review: this index looked dead by the same criterion used for the ones actually removed, but it's reachable via PrefsUnifiedDlg.cpp's pages[] table -- one entry ("Debugging", PreferencesDebug) is guarded by #ifdef __DEBUG__ and breaks the Preferences<Something>Tab naming convention every other entry follows, which is exactly what made it easy to miss with a naming-pattern-based grep or a release-only build. Deleting it wouldn't have failed any build (falls through to wxNullBitmap), so the only symptom would have been a blank icon on a debug-only preferences page -- worth spelling out for the next pass over this file. --------- Co-authored-by: got3nks <[email protected]>
…ree, and tab-close icons to CamuleArtProvider Continues the icon-system cleanup scoped in amule-org#675, independent of amule-org#732/amule-org#733 (different files, no overlap). - amuleDlgImages(14)/(15): the small header icons in the Friends and Messages tabs. Friends gets a new "amule:friends" SVG; Messages reuses the existing "amule:toolbar_messages" art, requested at an explicit 16x16 so it doesn't inherit the toolbar's 32x32 natural size. - amuleDlgImages(18)/(30): the "reload list" buttons (shared files, ED2K server list, Kad node list) -- three call sites, one new "amule:reload" SVG (see below for its own history). - amuleSpecial(1)/(2): the shared-directory tree's folder icons get new "amule:folder"/"amule:folder_shared" SVGs -- same shape, tinted orange vs. red, matching how the original raw bitmaps only differed by colour. - amuleSpecial(3)/(4): the chat/search notebook tabs' close-on-hover icon, replaced with wx's own stock wxART_CLOSE for both states instead of a second bespoke asset (both were the same "X in a box" bitmap, differing only by a hover-highlight border colour). reload.svg's own history, per PR review: the first hand-drawn attempt (thin blue single arc) didn't match the original at all -- got3nks caught it, I reconstructed the original amuleDlgImages(18) raster to check (a thicker green double-arrow circle) and redrew closer to that, which still wasn't good enough. Final version is AI-vectorized via Recraft (through the Higgsfield MCP), which got3nks preferred over further hand-drawn iterations. src/icons/icon_data.c (the checked-in fallback used when Python3 is absent at configure time, per amule-org#487) regenerated via embed_icons.py to match. Rebased onto current master (picking up amule-org#732/amule-org#733/amule-org#739, which all touch the same amuleSpecial/amuleDlgImages functions) -- conflicts resolved by redoing the index deletions against the current tree rather than replaying the stale patch, since amule-org#725/amule-org#733 already moved the surrounding line numbers. Verified via a full amule build (macOS) and a visual check of every call site: Friends/Messages tab headers, the ED2K server-list and Kad node-list reload buttons, the shared-files reload button, and the Preferences > Directory shared-folder tree.
…or tab-close (#675) (#735) * feat(gui): migrate Friends/Messages headers, reload buttons, folder tree, and tab-close icons to CamuleArtProvider Continues the icon-system cleanup scoped in #675, independent of #732/#733 (different files, no overlap). - amuleDlgImages(14)/(15): the small header icons in the Friends and Messages tabs. Friends gets a new "amule:friends" SVG; Messages reuses the existing "amule:toolbar_messages" art, requested at an explicit 16x16 so it doesn't inherit the toolbar's 32x32 natural size. - amuleDlgImages(18)/(30): the "reload list" buttons (shared files, ED2K server list, Kad node list) -- three call sites, one new "amule:reload" SVG (see below for its own history). - amuleSpecial(1)/(2): the shared-directory tree's folder icons get new "amule:folder"/"amule:folder_shared" SVGs -- same shape, tinted orange vs. red, matching how the original raw bitmaps only differed by colour. - amuleSpecial(3)/(4): the chat/search notebook tabs' close-on-hover icon, replaced with wx's own stock wxART_CLOSE for both states instead of a second bespoke asset (both were the same "X in a box" bitmap, differing only by a hover-highlight border colour). reload.svg's own history, per PR review: the first hand-drawn attempt (thin blue single arc) didn't match the original at all -- got3nks caught it, I reconstructed the original amuleDlgImages(18) raster to check (a thicker green double-arrow circle) and redrew closer to that, which still wasn't good enough. Final version is AI-vectorized via Recraft (through the Higgsfield MCP), which got3nks preferred over further hand-drawn iterations. src/icons/icon_data.c (the checked-in fallback used when Python3 is absent at configure time, per #487) regenerated via embed_icons.py to match. Rebased onto current master (picking up #732/#733/#739, which all touch the same amuleSpecial/amuleDlgImages functions) -- conflicts resolved by redoing the index deletions against the current tree rather than replaying the stale patch, since #725/#733 already moved the surrounding line numbers. Verified via a full amule build (macOS) and a visual check of every call site: Friends/Messages tab headers, the ED2K server-list and Kad node-list reload buttons, the shared-files reload button, and the Preferences > Directory shared-folder tree. * fix(gui): thicker reload stroke, dedicated message-bubble icon (#735 review) got3nks, testing #735: - reload: arrow bodies read too thin at 16px. Regenerated via Recraft with an explicit thicker/bolder-stroke prompt. - Messages panel header: reusing "amule:toolbar_messages" (the main toolbar's detailed gradient mascot bust) at 16x16 doesn't read as "messages" once shrunk that far from its 32x32 native size. Added a dedicated "amule:message" chat-bubble glyph instead, sized for legibility at 16x16 specifically, and pointed the Messages panel header at it instead of the toolbar art. Folder/folder_shared and the tab-close X were already approved as-is. Verified via a full amule build (macOS) and a visual check of both fixes: the shared-files reload button and the Messages panel header.
…org#747 review) got3nks caught a real functional regression I'd missed (a second, earlier review comment I hadn't seen when I replied to the later one about colour): 6 of these icons aren't standalone -- GenericClientListCtrl.cpp draws them as overlays on top of the base client icon at the same point.x/realY. The original raw art kept each in a corner so the base icon stayed visible underneath; my replacements were designed as standalone centered pictograms, so they covered the icon they were meant to annotate (creditsyellow at 14x13 was larger than the 12x12 base smiley it sat on). Recomputed each as a corner badge, scaled/positioned to roughly the original's measured footprint and anchor (his measurements, converted from 16x16 pixel terms to the 2048x2048 viewBox): - extendedprotocol: top-right, ~8x4 - secident: bottom-left, ~7x6 - creditsgrey/creditsyellow: top-right, ~9x9 - encryption: top-left, ~6x8 badguy is the one exception, and deliberately so: the original struck through the *entire* icon rather than badging a corner, which reads as intentional (a bad-guy client's status is fully overridden, not just annotated) -- kept that full-cover behaviour rather than shrinking it to a corner mark. Also added the null-bitmap guard he flagged: wxArtProvider::GetBitmap() returning wxNullBitmap for an unresolvable id was returning straight through the new early-return path, never reaching the wxFAIL_MSG that amule-org#739 added for exactly this failure mode. Added a wxASSERT_MSG on the early-return path so a future icon rename can't silently regress back to the "fails into a blank icon" state amule-org#725/amule-org#739 fixed. Verified via a full amule build (macOS) and a visual check of all 6 badges at both 96px and actual 16x16, confirmed each sits in its corner without covering the centre of the base icon.
…rovider (#675) (#747) * feat(gui): migrate 22 of 27 clientImages() status icons to CamuleArtProvider (#675) Continues the icon-system cleanup scoped in #675. clientImages() is the last of the raw-bitmap banks, and the riskiest: it's consumed through a wxImageList built once at startup (Apply_Clients_Skin, amuleDlg.cpp) by iterating index 0..26 in ClientSkinEnum order, and four call sites then index that list with arithmetic (Client_InvalidRating_Smiley + rating - 1) rather than calling clientImages() directly. The enum order is load-bearing; this PR does not touch it, the calling loop, or the arithmetic call sites -- only what clientImages(index) returns for each index. 22 of the 27 icons are generic pictograms (smileys, a checkmark, an X, stars, an arrow, a key, ...) with no external branding, migrated to new "amule:client_<name>" SVG/PNG art. The remaining 5 -- indices 12, 15, 16, 17, 18 (eMule/aMule/lphant/Shareaza/xMule) -- are specific client-software mascots and are deliberately left as the original raw artwork: a generated pictogram is one thing, but redrawing someone else's brand mascot risks not matching the real logo, which is worse than not touching it. #675 discussion flagged this distinction explicitly. New art, per icon: - Green/Red/Yellow/Grey/White: the 5 base transfer-state smileys. - ExtendedProtocol/SecIdent/BadGuy/Encryption: badge overlays drawn per GenericClientListCtrl.cpp's own comments ("the '-'", "the 'v'", "the 'X'", "the '\xc2\xbf' except it's a key"). - CreditsGrey/CreditsYellow: the two credit-system stars. - Upload: the upload-active arrow. - Friend: the friend-list badge. - mlDonkey/eDonkeyHybrid: kept generic/abstract (a checkmark badge, a simple ghost) rather than attempting their real logos -- same reasoning as the 5 mascots, lower bar since these were already more abstract marks than full character art in the original raster. - Unknown: the unresolved-client-type question mark. - InvalidRating/PoorRating/FairRating/GoodRating/ExcellentRating: one potion-bottle SVG re-tinted per rating (red/red/orange/green/green), with Invalid and Excellent composited as two bottles side by side in a single 16x16 square canvas -- matching the original art's single-vs-double-bottle distinction for the two rating extremes. - CommentOnly: the comment-icon badge. Implementation: a static art-id lookup table indexed by ClientSkinEnum value, checked first; a null entry falls through to the corresponding raw-bitmap `if (index == N)` block for the 5 mascot indices, which are otherwise untouched. src/icons/icon_data.c regenerated via embed_icons.py to match. Testing: full build verified (macOS), both Release and Debug config. Debug build launched with a live window (Apply_Clients_Skin runs at startup, touching all 27 indices) and produced no wxFAIL_MSG assertion (#739) -- confirms every index resolves. Each new icon reviewed individually via rasterized contact sheets at both ~96px and actual 16x16 before being wired in. NOT independently verified: seeing these icons rendered in an actual populated client list (Downloads sources, Shared Files clients, Search results, Friends) requires live eD2k/Kad network data, which a from-scratch test config doesn't have. Same limitation already flagged for fileDetails/clientDetails/CategoriesEditWindow in #744. * fix(gui): recentre double-bottle rating icons, add ClientSkinEnum static_assert (#747 review) got3nks, reviewing #747: 1. client_invalidrating.svg / client_excellentrating.svg didn't fill their 2048x2048 viewport like their poor/fair/good siblings (53%x73% fill for those vs. 83%x44% for these, with the left bottle clipped at x=-47). Recentred both bottle <g> transforms to translate(-110,452)/translate(940,452) with scale raised to 0.8 (from 0.6), matching the visual weight of the single-bottle ratings next to them in the same column. Both files are otherwise identical apart from fill colour, so the one fix applies to both. 2. Nothing tied artIds[] to ClientSkinEnum, and wxFAIL_MSG (#739) compiles out in release builds -- so a future enum member could silently push a wxNullBitmap into the 16x16 image list instead of failing loudly anywhere. Added static_assert(WXSIZEOF(artIds) == CLIENT_SKIN_SIZE, ...), which catches it at compile time in every build config. Needed "amuleDlg.h" (CLIENT_SKIN_SIZE's home) added to muuli_wdr.cpp's includes -- checked first that amuleDlg.h doesn't include muuli_wdr.h back, no circular include. Verified via a full amule build (macOS) -- the static_assert compiling clean confirms artIds[] and ClientSkinEnum agree on 27 entries, and a visual check that the two rating icons now match their siblings' size and no longer clip. * fix(gui): recompute double-bottle composition, verified numerically (#747 review) My previous fix (eedcac3) just moved the problem: applying got3nks's suggested translate values verbatim together with the also-suggested scale=0.8 pushed the right bottle past the viewBox edge (measured: x range 13%-107%, i.e. clipped on the right instead of the left). The two numbers were each individually reasonable but hadn't been re-verified together -- exactly the mistake the last commit's message claimed to have checked and hadn't. Recomputed from the natural bounding box instead of iterating on coordinates by eye: two bottles at scale 0.68 with a 40-unit gap, each positioned so both are fully inside the 0-2048 viewBox with equal margins. Verified numerically (not just visually) before committing: both groups' bboxes land at x 12.9%-49.0% and 51.0%-87.1%, y 25.2%-74.8% -- fully in bounds, no clipping on either side. Worth being upfront about the height: overall fill is 74%x50%, not poor/fair/good's 53%x73%. That's not a leftover bug -- fitting two full-height copies of the same bottle side by side in a square without overlap is not geometrically possible; matching the single bottles' 73% height would require the pair to be over twice as wide as the viewBox. 50% is the tradeoff of choosing "fits, no clipping, similar per-bottle proportions" over an unreachable exact size match. Verified via a full amule build (macOS) and the bounding-box computation above, re-run against the actual committed SVG files rather than the standalone drafts, to catch exactly the kind of last-mile mismatch that slipped through last time. * fix(gui): unify bottle height across all 5 rating icons (#747 review) got3nks: rather than treat the double-bottle pair as a special case with its own (necessarily shorter) height, make all five ratings share one consistent bottle height, maximizing what the double-bottle pair allows without clipping and bringing poor/fair/good down to match -- they sit in the same column, so consistency across the set matters more than any one icon being as large as it could be alone. Recomputed from the natural bottle bounding box: scale 0.909 is the largest that fits two bottles side by side (24-unit gap) inside the 2048x2048 viewBox with no clipping. Applied that same scale to the single-bottle ratings, centred. All five now render their bottle at the same height. Verified numerically against the actual committed files: all five land at y 16.8%-83.2% (66.3% height) and are within the 0-100% x range -- identical height across poor/fair/good/invalid/excellent, zero clipping. Verified via a full amule build (macOS). * fix(gui): brighten client_green to match the rest of the set's saturation (#747 review) got3nks measured across every opaque pixel: red and yellow gained brightness/saturation moving to the new art, but green lost both, landing dimmer (53% mean brightness) than red (64%) and yellow (72%) -- inverting the original's balance, where green and red were level at 57%/55%. Green carries the most meaning of the three (actively transferring), so it being the dimmest state undersells it. Replaced Material Green 500 (#4CAF50, 57% sat / 69% val) with #22CC1A (87% sat / 80% val), inside the ~#1BD816-#2ECC1F range got3nks suggested to restore the original's emphasis. Red and yellow untouched, per the review ("read fine as they are"). Verified via a full amule build (macOS) and a visual check of the icon at 96px. * fix(gui): corner-anchor badge overlays, add null-bitmap guard (#747 review) got3nks caught a real functional regression I'd missed (a second, earlier review comment I hadn't seen when I replied to the later one about colour): 6 of these icons aren't standalone -- GenericClientListCtrl.cpp draws them as overlays on top of the base client icon at the same point.x/realY. The original raw art kept each in a corner so the base icon stayed visible underneath; my replacements were designed as standalone centered pictograms, so they covered the icon they were meant to annotate (creditsyellow at 14x13 was larger than the 12x12 base smiley it sat on). Recomputed each as a corner badge, scaled/positioned to roughly the original's measured footprint and anchor (his measurements, converted from 16x16 pixel terms to the 2048x2048 viewBox): - extendedprotocol: top-right, ~8x4 - secident: bottom-left, ~7x6 - creditsgrey/creditsyellow: top-right, ~9x9 - encryption: top-left, ~6x8 badguy is the one exception, and deliberately so: the original struck through the *entire* icon rather than badging a corner, which reads as intentional (a bad-guy client's status is fully overridden, not just annotated) -- kept that full-cover behaviour rather than shrinking it to a corner mark. Also added the null-bitmap guard he flagged: wxArtProvider::GetBitmap() returning wxNullBitmap for an unresolvable id was returning straight through the new early-return path, never reaching the wxFAIL_MSG that #739 added for exactly this failure mode. Added a wxASSERT_MSG on the early-return path so a future icon rename can't silently regress back to the "fails into a blank icon" state #725/#739 fixed. Verified via a full amule build (macOS) and a visual check of all 6 badges at both 96px and actual 16x16, confirmed each sits in its corner without covering the centre of the base icon.

First slice of the icon-system cleanup scoped in #675 — delete code with zero call sites anywhere in the tree, before any
wxArtProvidermigration work starts on what's actually reachable.What's removed
muleToolbar()— whole function, unused (superseded by the toolbar setup inamuleDlg.cpp).moreImages()— whole function, unused (both of its 2 icon entries).amuleDlgImages()— 21 of 35 index blocks have no caller anywhere (0-13,16,17,19,27,28,31,34). The 14 live ones are untouched: 10 of those (20-26,29,32,33) are already the fallback path insideamuleDlg.cpp'sAdd_Skin_Icon, which tries awxArtProvider/SVG lookup first; the other 4 (14,15,18,30) are still called directly.amuleSpecial()— 6 of 26 index blocks have no caller (6,7,8,9,18,20) — checked both direct call sites and thePrefsUnifiedDlg.cppfallback table (pages[].m_imageidx), which uses neither.convert_xpminPartFileConvertDlg.cppwas on the same "no literal grep hits" list at first, but isn't actually dead:SetIcon(wxICON(convert))reaches it through thewxICONmacro's token-pasting (X##_xpm), invisible to a plain identifier search. Caught by a full build failing on the undeclared identifier, not by inspection — left untouched, worth flagging since the same macro pattern could hide other false positives in a future pass over this file.Result
1373 lines deleted (~16% of
muuli_wdr.cpp). No behavior change — every touched entry was unreachable.Testing
clang-formatv18 clean. Fullamulebuild verified on macOS (both before and after, diffing that only the intended symbols moved). Not aCLIENT_GUI-relevant change (neither touched function/index is behind#ifdef CLIENT_GUI).