feat: include AICH in generated magnet links, parse it back on load - #651
Conversation
…mule-org#331) CreateMagnetLink() now adds an "xt=urn:aich:<hash>" field alongside the existing ed2k/ed2khash ones whenever the file has a proper AICH hash set -- same source and guard CreateED2kLink() already uses for the ed2k link's "h=" field. CMagnetED2KConverter::GetED2KLink() -- used both when a user pastes a magnet: link (CDownloadQueue::AddLink) and by the "ed2k" CLI tool -- now looks for that urn:aich: field alongside the ed2k one and, when present, embeds it as "h=<aich>|" in the ed2k link it builds. That string then flows through the *existing* ed2k link parser (ED2KLink.cpp already handles "h="), so the AICH hash reaches the download with no second parsing path to add or keep in sync. Added MagnetURITest (4 cases: field round-trips into the magnet link, AICH urn converts into the ed2k h= field, no AICH urn means no h= field, and field order in the magnet doesn't matter) since this is pure string logic with no GUI/network dependency -- a much more reliable check than exercising it by hand through the app. Verified both compile modes MagnetURI.cpp supports: the main app (wxString) and the standalone "ed2k" CLI tool (USE_STD_STRING).
got3nks
left a comment
There was a problem hiding this comment.
Both directions covered, and reusing the existing h= parser on import is clean. One guard I'd like before merge:
The AICH token is passed through opaquely, but the ed2k parser throws on a bad master-hash (ED2KLink.cpp: throw wxString("Invalid master-hash")). So a magnet with a junk/truncated urn:aich: converts to …|h=<garbage>|/ and gets rejected wholesale — even though its ed2k hash is valid. Previously the AICH urn was just ignored, so it couldn't break the add.
Could you only embed h= when the token is well-formed (32 chars, base32 [A-Z2-7]), and drop it otherwise? A plain string check works in both build modes (no CAICHHash dependency), plus a MalformedAichUrnIsDropped test to lock it in. Keeps the invariant "valid ed2k magnet ⇒ always addable, junk AICH ignored."
Rest LGTM.
ED2KLink.cpp's ed2k-link parser throws on a bad master-hash, so a magnet carrying a junk or truncated urn:aich: converted straight into a "...|h=<garbage>|/" ed2k link that got rejected wholesale on import -- even though its ed2k hash was perfectly valid and previously (before urn:aich: support) would have been ignored harmlessly. Validate the AICH token as 32 chars of base32 [A-Z2-7] before embedding it, and drop it silently otherwise, keeping the invariant that a valid ed2k magnet is always addable regardless of what a malformed AICH tagalong looks like. Plain char-range check, no CAICHHash dependency, so it compiles the same in both build modes. Addresses got3nks's review comment on amule-org#651.
|
Good catch. Fixed in 67d9366: Added |
got3nks
left a comment
There was a problem hiding this comment.
Guard's in and both failure shapes are covered by the new test — a malformed urn:aich: now drops cleanly and the valid ed2k hash always goes through. LGTM.
Mechanical rebase to resolve the po/ catalog conflicts against master's amule-org#648/amule-org#650/amule-org#651 (per got3nks's note on amule-org#643) -- no source changes here, scripts/update-po.sh output only, so the diff is just the tree back in sync with the current strings.
Mechanical rebase to resolve the po/ conflicts against master's amule-org#643/amule-org#646/amule-org#648/amule-org#650/amule-org#651 -- no source changes here. Confirms got3nks's prediction on the Alt+<letter>-outside-_() fix: the only genuinely new msgid in the diff is "Navigate" (the new macOS menu itself); every other hunk is line-number-comment churn from the rebase, not orphaned or duplicated translations.
Mechanical rebase to resolve the po/ catalog conflicts against master's amule-org#648/amule-org#650/amule-org#651 (per got3nks's note on amule-org#643) -- no source changes here, scripts/update-po.sh output only, so the diff is just the tree back in sync with the current strings.
* feat(search): persist search history across restarts (#641) The search field was a plain text control with no memory of past searches. Swap it for a wxComboBox (a wxTextEntry, like wxTextCtrl, so the existing GetValue()/Clear() call sites keep working via that shared base) and persist submitted terms to wxConfig, most-recent first, capped at 20 entries and deduplicated case-insensitively. Right-clicking the field opens a small menu ("Remember search history" checkbox + "Clear search history") so history can be paused or wiped without a dedicated Preferences page. i18n: ran scripts/update-po.sh to register the two new menu strings. * style(search): separate the destructive Clear item from the toggle above A separator between the checkbox and "Clear search history" keeps the one-shot destructive action visually distinct from the persistent on/off state, instead of reading as a second toggle in the same group. * style(search): use nullptr instead of NULL for the new combo box Fixes the modernize-use-nullptr clang-tidy finding on the changed line; matches the nullptr convention already used a couple of wxComboBox constructions below in the same file. * feat(search): rework query history per #643 review — file, prefs toggle, edit menu Addresses all four blocking points from got3nks's review, plus both optional suggestions: 1. Edit context menu restored. Overriding the field's context menu no longer drops Cut/Copy/Paste/Select All -- same custom-Paste-ID idiom as CMuleTextCtrl::OnRightDown (wxMenu over-permits wxID_PASTE, so it gets a manual clipboard-content check), plus a separator, plus the history's own Clear action. 2. Persisted to a dedicated searchhistory.dat (via CTextFile, one term per line) instead of amule.conf -- mirrors eMule's AC_SearchStrings.dat. Query terms no longer bloat the main config or travel with config backups / the --amule-config-file push to amuleweb/amuleapi. 3. "Remember search history" moved into Preferences > General as an ordinary Cfg_Bool checkbox (IDC_SEARCHHISTORYENABLED), same NewCfgItem wiring as every other GUI-behavior toggle on that page. It's a client-side-only setting, so the one checkbox works unchanged in both amule and amuleGUI. The context menu keeps only Clear (a one-shot action, not a setting to toggle from a hidden menu). 4. Cap raised from 20 to 30, matching eMule's CCustomAutoComplete default. Optional, done anyway: - wxComboBox::AutoComplete() wired up (eMule's ACO_AUTOSUGGEST equivalent), re-armed after every load/record/clear so it always reflects the current entry set. - Explicitly framed as *query* history throughout (comments, tooltip) to distinguish it from the separate, not-yet-implemented result persistence that's the rest of #641. The dedup/move-to-front/cap logic that used to live inline in CSearchDlg::RecordSearchHistory is extracted into a pure function, ApplySearchHistoryEntry() (SearchHistory.h/.cpp), decoupled from wxComboBox/wxConfig specifically so it's unit-testable without a wx event loop -- new SearchHistoryTest covers empty-list insert, reorder of an existing term, case-insensitive dedup, empty-term no-op, and capping (including locking in the actual 30-entry constant). Rebased onto current master; po/ catalogs regenerated as the final step so the diff is just the new/changed strings in sync with the tree. Verified for real: built and ran both amule and amulegui, plus the full unit test suite (27/27 passing, including the new SearchHistoryTest's 7 cases). * fix(tests): link Format.cpp + strerror_r.c into SearchHistoryTest Same Linux/mingw-only link failure as CMuleCollectionTest/MagnetURITest already work around: muleunit's MuleDebug.cpp needs CFormat for its glibc backtrace path, which isn't compiled in on macOS -- so the target linked fine locally but failed on the Ubuntu/mingw CI builds (and the clang-tidy jobs, which build the tree first). * i18n: regenerate po/ catalogs after rebasing onto current master Mechanical rebase to resolve the po/ catalog conflicts against master's #648/#650/#651 (per got3nks's note on #643) -- no source changes here, scripts/update-po.sh output only, so the diff is just the tree back in sync with the current strings. * fix(search): avoid an unnamed EReadTextFile bitmask cast in history load txtIgnoreEmptyLines|txtStripWhitespace has no single named enumerator to cast to -- clang-tidy Tier-1 flagged it (clang-analyzer-optin.core. EnumCastOutOfRange), correctly: EReadTextFile isn't a flag enum, so a synthesized OR'd value is genuinely out of its declared range even though CTextFile::ReadLines treats it as bitflags at runtime. txtReadDefault would dodge the cast but also drops '#'-led lines, silently eating a legitimate search term that happens to start with one. Read unfiltered (txtReadAll, a real enumerator) and do the trim/empty-line-drop by hand instead -- same behavior, no cast, no lost terms. * i18n: regenerate po/ catalogs after rebasing onto current master Mechanical rebase to resolve the po/ conflicts against master's #642 (just merged) -- no source changes here.
Summary
Fixes #331 — two related asks:
Implementation
CreateMagnetLink()now adds anxt=urn:aich:<hash>field alongside theexisting ed2k/ed2khash ones whenever the file has a proper AICH hash set —
same source and guard
CreateED2kLink()already uses for the ed2k link'sh=field.CMagnetED2KConverter::GetED2KLink()— used both when a user pastes amagnet:link (CDownloadQueue::AddLink) and by theed2kCLI tool —now looks for that
urn:aich:field alongside the ed2k one and, whenpresent, embeds it as
h=<aich>|in the ed2k link it builds. That stringthen flows through the existing ed2k link parser (
ED2KLink.cppalready handles
h=), so the AICH hash reaches the download with nosecond parsing path to add or keep in sync — the second half of the
request falls out of the first for free.
Test plan
Added
MagnetURITest(4 cases) since this is pure string logic with noGUI/network dependency — more reliable than exercising it by hand through
the app:
GetLinkIncludesAichField— field round-trips into the generated magnet linkConvertsAichUrnIntoEd2kHField— AICH urn converts into the ed2kh=field, correctly positioned before the closing/NoAichUrnMeansNoHField— no AICH urn means noh=field (regression check)AichUrnOrderBeforeEd2kUrnStillWorks— field order in the magnet doesn't matterVerified both compile modes
MagnetURI.cppsupports build cleanly: the mainamuleapp (wxString) and the standaloneed2kCLI tool (USE_STD_STRING)Not verified end-to-end through the GUI with a real peer serving an AICH-bearing file (impractical to simulate) — the conversion logic is unit-tested in isolation and the downstream parser is pre-existing/unchanged, so residual risk is low