Skip to content
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

Merged
merged 14 commits into from
Dec 19, 2024
Merged

Conversation

mntn-xyz
Copy link
Contributor

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.

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.
@makew0rld
Copy link
Owner

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.

@mntn-xyz
Copy link
Contributor Author

mntn-xyz commented Dec 8, 2021

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.

@mntn-xyz mntn-xyz closed this Dec 8, 2021
@mntn-xyz mntn-xyz reopened this Dec 8, 2021
@mntn-xyz
Copy link
Contributor Author

One of the changes has broken this, need to figure out what happened.

@mntn-xyz mntn-xyz marked this pull request as draft December 11, 2021 03:42
@makew0rld
Copy link
Owner

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!

@mntn-xyz
Copy link
Contributor Author

The issue I was encountering seems to be a recently introduced bug, see #281. Once that is fixed, this should be ready.

@mntn-xyz mntn-xyz marked this pull request as draft December 11, 2021 12:51
@mntn-xyz
Copy link
Contributor Author

Todo: use goroutines to prevent deadlock per #281

@makew0rld
Copy link
Owner

Please see changes made in #284 (once merged) for a guide on how to fix issues like #281.

@makew0rld
Copy link
Owner

@mntn-xyz #284 has been merged and #281 has been closed, so feel free to continue working on this whenever you can. You'll have to merge master back into this PR to get those updates.

@mntn-xyz
Copy link
Contributor Author

mntn-xyz commented Jan 2, 2022

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.

@mntn-xyz mntn-xyz marked this pull request as ready for review January 2, 2022 01:40
@makew0rld
Copy link
Owner

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.

@makew0rld makew0rld added this to the v1.10.0 milestone Feb 2, 2022
@makew0rld makew0rld modified the milestones: v1.10.0, v1.11.0 Feb 4, 2024
@makew0rld makew0rld removed this from the v1.11.0 milestone Dec 19, 2024
// 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)
Copy link
Owner

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.

@makew0rld
Copy link
Owner

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.

@makew0rld makew0rld merged commit fd8f122 into makew0rld:master Dec 19, 2024
3 checks passed
makew0rld added a commit that referenced this pull request Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants