-
-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds support for custom commands #265
Conversation
Supports up to 10 custom commands, defined in the configuration file. The current or selected URL can be passed to the command by placing ${url} in the command string. Default hotkeys are Alt-1 through Alt-0 for triggering the command with the selected link URL as a parameter, or Alt-Shift-1 through Alt-Shift-0 for triggering the command with the current page URL as a parameter. This follows the convention set by the copy page URL (C) and copy selected URL (c) defaults. Custom commands must be available in the $PATH or use absolute paths. Relative paths are not supported, neither are pipes or shell redirections.
Thanks for your PR. I'll try and take a look at this when I have time, but it's not a priority for v1.9.0. Also note there's now a conflict, let me know if you need any help resolving that. |
Thanks, I had not seen the conflict. It is resolved now. Let me know if there's any improvements I need to make when you get a chance to review it. |
One of the changes has broken this, need to figure out what happened. |
Ok, let me know if you need any help or have questions. This is a cool feature for sure, but not top of my list, so I'm not sure if it'll make it into the v1.10.0 release. It's gonna be a while before I get there though, so who knows :) Thanks for making contributions to Amfora! |
The issue I was encountering seems to be a recently introduced bug, see #281. Once that is fixed, this should be ready. |
Todo: use goroutines to prevent deadlock per #281 |
Merged up to master, and I made the calls to Error and Info use goroutines as in #284. I think the risk of a race condition here with other parts of the UI is minimal, as it looks like it shouldn't be possible to invoke one of these commands during other operations that also pops up a dialog. The commands are forked when run so long-running commands also won't be an issue. I did some light testing and couldn't intentionally break it. |
Sorry I haven't had a lot of time to review this PR. I'm going to add it to the milestone so that I won't forget to come back to it when I make time for Amfora in the future. |
// RunCommand runs `command`, replacing the string "${url}" with `url`. | ||
func RunCommand(command string, url string) (string, error) { | ||
cmdWithURL := strings.ReplaceAll(command, "${url}", url) | ||
cmdSplit := strings.SplitN(cmdWithURL, " ", 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using a simple split like this will fail on commands with quoted strings. It might be better to use go-shellquote or something else.
I'll merge this now, I appreciate your work. If you have time to address the issue I mentioned above in a new PR that would be awesome. |
For #262
Supports up to 10 custom commands, defined in the configuration file. The current or selected URL can be passed to the command by placing ${url} in the command string.
Default hotkeys are Alt-1 through Alt-0 for triggering the command with the selected link URL as a parameter, or Alt-Shift-1 through Alt-Shift-0 for triggering the command with the current page URL as a parameter. This follows the convention set by the copy page URL (C) and copy selected URL (c) defaults.
Custom commands must be available in the $PATH or use absolute paths. Relative paths are not supported, neither are pipes or shell redirections.