helium/ui: add "Copy link" action to downloads context menu#1511
helium/ui: add "Copy link" action to downloads context menu#1511saberoueslati wants to merge 8 commits into
Conversation
|
I had also errors in the CI/CD piepline, I'll fix them then make it ready for review |
It is possible to have clear/clear from list option similar to firefox? |
| + <message name="IDS_DOWNLOAD_MENU_COPY_LINK" | ||
| + desc="Download context menu: Copy the download source URL to the clipboard"> | ||
| + Copy link | ||
| + </message> |
There was a problem hiding this comment.
i think we can borrow one of these instead of retranslating it: https://source.chromium.org/search?q=case:y%20pcre:yes%20file:generated_resources.grd%20%22Copy%20link%22&ss=chromium%2Fchromium%2Fsrc
There was a problem hiding this comment.
I'll check it when I'm home later this evening
There was a problem hiding this comment.
@dumbmoron I replaced IDS_DOWNLOAD_MENU_COPY_LINK with preexisting IDS_DOWNLOAD_COPY_DOWNLOAD_LINK
There was a problem hiding this comment.
@dumbmoron Video of the manual test after the change
Screencast.from.2026-05-05.02-09-21.webm
I'm struggling to test on Windows, build is very slow there, test done on Ubuntu works perfectly. If testing on Windows is necessary, I'll keep working on it there; otherwise the PR is ready to be merged
|
Reviews (1): Last reviewed commit: "updated source.gen.json" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| + case DownloadCommands::COPY_DOWNLOAD_LINK: | ||
| + id = IDS_DOWNLOAD_COPY_DOWNLOAD_LINK; | ||
| + break; |
There was a problem hiding this comment.
The patch references IDS_DOWNLOAD_COPY_DOWNLOAD_LINK in download_ui_context_menu.cc, but no change to generated_resources.grd defining this string ID is included anywhere in the patch. The PR description mentions adding IDS_DOWNLOAD_MENU_COPY_LINK (a different name) to the GRD file, but that entry is also absent from the patch. Without the corresponding GRD entry, the build will fail with an undefined symbol error. The patch needs a hunk that adds the new string to generated_resources.grd, and the ID used there must exactly match IDS_DOWNLOAD_COPY_DOWNLOAD_LINK.
| // kReviewEnabled = 34, | ||
| // kReviewClicked = 35, | ||
| kNotReached = 36, // Should not be possible to hit | ||
| - kMaxValue = kNotReached | ||
| + kCopyDownloadLinkEnabled = 37, | ||
| + kCopyDownloadLinkClicked = 38, | ||
| + kMaxValue = kCopyDownloadLinkClicked | ||
| }; |
There was a problem hiding this comment.
kNotReached no longer semantically last in the stats enum
kCopyDownloadLinkEnabled = 37 and kCopyDownloadLinkClicked = 38 are appended after kNotReached = 36, which carries the comment "Should not be possible to hit". The intent of kNotReached is to act as a sentinel for an unreachable default branch; placing new valid values after it leaves it stranded in the middle of the enum, which is confusing and may cause future values to be mispositioned as well. The two new values should be inserted before kNotReached, keeping it last (or rename/remove it as the sentinel).
|
Reviews (2): Last reviewed commit: "updated source.gen.json" | Re-trigger Greptile |
| + case DownloadCommands::COPY_DOWNLOAD_LINK: | ||
| return true; |
There was a problem hiding this comment.
IsCommandEnabled returns true unconditionally for invalid/empty URLs
COPY_DOWNLOAD_LINK is always enabled regardless of whether GetURL() returns a valid URL. If the URL is empty (e.g. some offline items or edge-case downloads), GetURL().spec() returns an empty string, which silently overwrites the user's clipboard with nothing. The base IsCommandEnabled case should return GetURL().is_valid() instead of falling through to return true.
|
@dumbmoron Finally also tested on Windows, works as intended there too Enregistrement.2026-05-05.174531.mp4 |
Co-authored-by: jj <log@riseup.net>
3 participants
For your pull request to not get closed without review, please confirm that:
(an approved feature request, or confirmed bug).
otherwise I have marked my PR as draft.
organization if I lied by checking any of these checkboxes.
Tested on (check one or more):
I tested my PR only on Linux at the moment, leaving this as a draft until I test on Windows, I'm building helium there at this moment
Edit : Tested also on Windows
Video of manual test below :
Screencast.from.2026-04-29.00-09-03.webm
Closes #897
Adds a "Copy link" option to the download context menu that copies the original download source URL to the clipboard. Available on in-progress, paused, finished, and interrupted downloads.
Changes
COPY_DOWNLOAD_LINKcommand wired through all model/controller layersScopedClipboardWriterinDownloadUIModel::ExecuteCommanddownload_ui_context_menu.cckCopyDownloadLinkEnabled,kCopyDownloadLinkClicked)IDS_DOWNLOAD_MENU_COPY_LINKstring ("Copy link") ingenerated_resources.grd