feat(gui): open a file and show it in the file manager, from both lists - #831
Merged
Conversation
Supersedes amule-project#278 by tbo47, whose Shared-tab "play" item this grew out of. Availability no longer depends on which binary is running. The old check was build-variant plus connection locality: always allowed in the monolithic app, allowed in amulegui only over a loopback connection. That refused every legitimate shared-filesystem setup, and allowed a local file that had since been moved or deleted. It is now simply whether the resolved path exists on this host, tested when the context menu is built -- one stat() per menu open, no polling. Two entries, on the Downloads list and the Shared Files list: Open the desktop's handler for the file's type. Media still goes to the configured video player, including the in-progress preview, but everything else goes to the platform opener regardless of that setting -- opening an archive no longer launches the video player. Show in file the containing folder, with the file selected on macOS and manager Windows. Suppressed for an unfinished download, whose path is a .part in the temp directory. Both live in a new FileLaunch, shared by the two lists so they cannot drift. Because the gate is now path existence rather than #ifdef CLIENT_GUI, the new file has no build-variant branches at all. An unfinished download is "NNNN.part" on disk, and no desktop registers a handler for that extension, so the platform opener cannot do anything with it. With no player configured, that case now says so instead of handing the file to an opener that can only fail. macOS reveals through NSWorkspace rather than spawning `open -R`: it selects the file, and needs no subprocess. Elsewhere the launch keeps the AppImage-safe environment from amule-project#334, and passes an argument vector rather than a command string so a name with spaces or quotes needs no escaping. Also fixes two things next door: the Downloads label fell through to an empty string if a file was neither part nor complete, and the enable test used PreviewAvailable(), which is media-only -- so a finished .zip or .pdf could not be opened at all. From the amule-project#278 review: the CPartFile downcast is guarded by IsPartFile(), and there is no reinterpret_cast.
Argument injection (the important one). LaunchWithPlayer had reproduced the old PreviewFile() string-building without the quote escaping that used to sit beside it, so a remote-supplied eD2k filename could inject arguments into the player command: "x' --script=/tmp/evil.lua '.avi" arrives as a separate --script argument, and mpv and vlc both execute scripts given that way. Restoring the old escaping would not have been enough either -- a backslash defeats it. The template is now split once with wxCmdLineParser::ConvertStringToArgs and the placeholders substituted inside the resulting arguments, so the path and the bare name each cross as a single argv entry and there is no string left to inject into. This also removes the double substitution that appended the path and then replaced %PARTFILE inside it, which mangled any filename containing the placeholder. The shared list shares PS_READY part files, so it needed the same media gate the Downloads list has: an unfinished entry is only openable when enough of the media is on disk, and it is labelled Preview rather than Open. Without it an in-progress .zip was handed to the video player -- the exact behaviour this work claimed to remove. Double-click stays media-only, as it was. Widening it to any completed type would have handed a .exe or .desktop to the platform opener on a gesture that is easy to trigger by accident; the menu's Open is the broad one. Both lists now act on the row the menu was built for. They previously took the first selected row, which diverges as soon as more than one is selected: CheckSelection only re-selects when the clicked row was not already selected, so the entry could enable against one file and act on another. Downloads also dropped its files.size() == 1 bail, which left the entry enabled and the click a silent no-op. Also: Windows reveal now reports a failure to spawn Explorer (its exit status stays ignored, being non-zero even on success); Reveal logs when the file disappeared between the menu being built and the click; the player preference is taken by reference, which the whole-tree clang-tidy gate requires; and the QUOTE macro, the TerminationProcess include and a stale comment block are gone from DownloadListCtrl now that nothing there launches anything. No string changes.
ConvertStringToArgs defaults to wxCMD_LINE_SPLIT_DOS, and the previous commit passed no type -- so on Linux and macOS a template that had always worked suddenly parsed by DOS rules. Single quotes stayed literal, backslash escapes were not honoured, and `mpv -fs '%PARTFILE'` or a player path with an escaped space silently stopped launching. Verified against wx 3.3: DOS splits `'/usr/local/my player/mpv' --fullscreen` into three broken pieces where UNIX yields the intended two. Windows genuinely needs DOS, since UNIX rules would eat the backslashes in C:\Program Files\..., so the type is chosen per platform. Placeholder substitution is now a single pass. Running three Replace() calls in sequence meant a value inserted by an earlier one was rescanned by a later one, so a file named "x%PARTNAME.avi" had that placeholder expanded inside its own path -- the same defect the previous commit set out to remove, in a narrower form. Both lists re-check that m_menuRow still names a live row. PopupMenu runs a nested event loop, so timers and EC updates keep firing while the menu is open: a completed download being cleared, or the shared-dir watcher's rescan, can shrink the list underneath it. ItemAt() and FileAtRow() only wxASSERT their range, which compiles out in the Release builds CI ships, so the stale index would have read out of bounds and dereferenced whatever it found. The previous code took the first selected row, which was always live, so this hazard came in with the switch to the clicked row. Double-click's comment now says what it does. It is still media-only, but it covers an in-progress download once enough of the media is on disk, where the old condition also required completion -- the classic eMule gesture, and the same thing the menu's Preview entry offers for that row. No string changes.
A bounds-checked row index stops the crash but not the mix-up: PopupMenu runs a nested event loop, so removing a row *above* the clicked one shifts every index below it, and the handler would then act on the neighbouring file -- opening or revealing something the user did not click. Both lists now remember the item data rather than the position, and re-validate it with CMuleVirtualListCtrl::HasItemData() before use. That pins identity instead of location, so a list mutated while the menu is open either still holds the item or does nothing. Also swaps the placeholder scanner's Mid() for compare(), which does not allocate a temporary string at every character position.
CanReveal() is CanOpen() plus an in-memory test, so building a menu asked the filesystem the same question twice for every completed file. The check has no upper bound -- on a network mount that has gone away it blocks the GUI thread indefinitely -- so halving the exposure is worth a small API. GetAvailability() answers both questions from a single stat(). CanOpen() and CanReveal() stay for the callers that genuinely need one alone: double-click tests only openability, and Reveal() re-checks on click because the file can disappear between the menu being built and the entry being chosen. Keeping the "revealing a .part is meaningless" rule inside FileLaunch also stops it being duplicated into both lists.
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #278 by @tbo47, whose Shared-tab "play" item this grew out of — the review points there (guard the
CPartFiledowncast, drop thereinterpret_cast, label it "Open" and route by file type) are all carried over.Adds two context-menu entries to both the Downloads and Shared Files lists:
Availability is now about the file, not the binary
The old check was build variant plus connection locality: always allowed in the monolithic app, allowed in amulegui only over a loopback connection (#657). That refused every legitimate shared-filesystem setup, and allowed a local file that had since been moved or deleted.
It is now simply whether the resolved path exists on this host, evaluated when the context menu is built. Both menus are constructed fresh per right-click, so the filesystem is touched only on that gesture — no polling, no background work, and no per-item loop, so the cost does not scale with the selection.
Concretely that is one
stat()call per right-click and one on double-click. Both menu entries need the answer, and asking for them separately stated the same question twice, soGetAvailability()answers both from a single check. The pre-PR menu build touched the filesystem zero times, so this is a real change of character rather than a free check, and it is worth being precise about: astat()has no upper bound. On a hung NFS hard mount it blocks uninterruptibly and takes the whole wx main loop with it, EC socket included; a soft mount bounds it at 60 s and up, dead SMB is typically 60–180 s. That is accepted deliberately — the alternatives (a cached result refreshed off the GUI thread, or gating on the filesystem being local, which needs astatfsthat can block just as badly) cost more than the exposure is worth for how few users run aMule against a network mount that can vanish.The one case it cannot distinguish is a remote daemon whose path happens to exist locally as a different file. That is accepted deliberately: the alternative refuses every shared-filesystem setup, which is the common case this unblocks.
A side effect worth noting: because the gate is no longer
#ifdef CLIENT_GUI, the new code has no build-variant branches at all.Opening
Double-click stays media-only, as it has always been — it is easy to trigger by accident, and a completed
.exeor.desktopshould not reach the platform opener that way. The menu's Open is the broad one.Media goes to the configured video player, including the in-progress preview. Everything else goes to the platform opener regardless of that setting — so "Open" on an archive no longer launches the video player, which is what #278's review asked for.
An unfinished download is
NNNN.parton disk, and no desktop registers a handler for that extension. With no player configured, that case now says so instead of handing the file to an opener that can only fail with its own "no application set to open this document" dialog."Show in file manager" is suppressed for unfinished downloads: revealing a
.partin the temp directory is never what was meant.Platform specifics
macOS reveals through
NSWorkspace selectFile:rather than spawningopen -R— it selects the file, and needs no subprocess. Windows usesexplorer /select,, which also selects; note Explorer exits non-zero even on success, so its status is deliberately ignored. Linux opens the containing folder viaxdg-openwithout selecting: doing better would mean theorg.freedesktop.FileManager1D-Bus interface, which not every file manager implements.The player command is split into arguments once, with
wxCmdLineParser::ConvertStringToArgs, and the placeholders are substituted inside the resulting arguments — so the path and the bare name each cross as a single argv entry. Building a command string instead would let a remote-supplied eD2k filename inject arguments into the player (--script=…is executable on both mpv and vlc); escaping the quotes, which is what the old code did, is bypassable with a backslash.Launches keep the AppImage-safe environment from #334 —
AppRunprepends the bundle's lib and bin directories to a child's search paths, and a host program that inherits them loads our older bundled libraries and dies on an undefined symbol. Our own launches pass an argument vector rather than a command string, so a name containing spaces or quotes needs no escaping; the user's player command necessarily stays a string, since it is a template with placeholders.Also fixed next door
IsPartFile()isstatus != PS_COMPLETE), but it was a blank menu entry waiting for a third status.PreviewAvailable(), which is media-only, so a finished.zipor.pdfcould not be opened at all. Preview and open are now separate questions.Testing
Built and exercised on macOS 15 (arm64), Ubuntu 26.04 (arm64, wxGTK), and Windows 11 (arm64, CLANGARM64) — all three from this commit. General interactive passes on each: opening media and non-media files from both tabs, revealing, and the disabled states.
Verified by construction rather than by clicking every cell: the full case matrix (all eight combinations of tab × file state × player-configured) was not walked on every platform.
ctest33/33, clang-format 18 clean, Tier-2 clang-tidy clean on the diff with 0 compiler errors. Note.mmfiles are not covered by the format gate — clang-format refuses Objective-C under this project's.clang-format, which is whyMacAppHelper.mmis hand-kept.AppImage and Flatpak are checked after merge, from the packaging artifacts.
packaging.ymlfires on push to master and its path filter coverssrc/**, so this lands there on merge. Both are places this can behave differently from a plain build — AppImage through the$APPDIRenvironment, Flatpak through--filesystem=homeand the portal shim standing in forxdg-open. A file outside$HOMEunder Flatpak should fail the existence check and grey the entries out, which is the correct answer but unverified until then.Strings
Five new msgids, all untranslated at merge, so these messages read English in every locale until the next Weblate round.
po/POTFILES.ingainssrc/FileLaunch.cpp— without it the new strings would not be extracted at all, and two existing ones would have been silently dropped as they moved out ofDownloadListCtrl.cpp.