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

Added --run-command option, obsoletes --wrapper #104

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

Hritik14
Copy link
Contributor

Wrapper was an idea exclusively useful for running the binary via i3-exec.
While that is of utmost importance, the current --run-command feature subsets wrapper and a lot more.

The current syntax is

           --run-command "/usr/bin/anybinary {}"

The --wrapper could be replaced with a similar structure

           --run-command "wrapper_binary '{}'"`

I strongly advocate for --run-command and obsolete --wrapper as it is way more awesome and flexible. The idea was directly inspired by rofi's -run-command

Hritik14 and others added 2 commits October 11, 2020 15:12
Wrapper was an idea exclusively useful for running the binary via
i3-exec. While that is of utmost importance, the current --run-command
feature subsets wrapper and a *lot more*.

The current syntax is
```
	--run-command "/usr/bin/anybinary {}"
```

The --wrapper *could* be replaced with a similar structure
```
	--run-command "wrapper_binary '{}'"
```

I strongly advocate for `--run-command` and obsolete `--wrapper` as it is
way more awesome. The idea was directly inspired by rofi's -run-command
@Hritik14
Copy link
Contributor Author

--wrapper is still present for backwards compatibility, though I intend to either

  1. completely remove --wrapper cmd
  2. make it an alias to --run-command "cmd ''"

// --run-command switch
if (this->run_command.length()){
command = replace(this->run_command, this->run_command_token, command);
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think command should be shell-quoted here, so that the command always gets the command line as a single argument? Not sure, really.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that totally depends on the end user. They can perfectly choose to shell quote the {}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One the other hand, if we do shell-quote by default and the user then - by standard practice - quotes again, it would be something like "{}" -> ""cmd"" which would completely defy the quotes. I guess the current approach should suffice. Let me know what you think.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm only familiar with the {} syntax from find(1) - there are likely others, which probably behave differently - which escapes the file name (e.g .find -exec strace -e execve echo {} \; with some files with spaces in their names will look like this: execve("/usr/bin/echo", ["echo", "./foo bar baz"], ..., so the {} becomes a single argument).

Copy link
Contributor Author

@Hritik14 Hritik14 Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new commit to resolve this issue. This is as far I could think which would maintain $SHELL usage and won't break with weird filenames (ones with quotes in them)

Similar to the `{}` syntax from `find(1)` which escapes the file name,
-run-command now escapes the input `command`.
The escaping is POSIX compliant, there may be issues if some weird shell
is used.
@Hritik14
Copy link
Contributor Author

Hi @enkore, is there any update on the PR ?

@meator
Copy link
Collaborator

meator commented May 5, 2024

Hey @Hritik14, are you still interested in adding this functionality to j4-dmenu-desktop? A lot has changed (partly thanks to me) since this pull request was made. I have added support for i3 IPC, which should replace the main usecase for --wrapper.

This change makes --wrapper pretty much useless in my eyes. I can't come up with any other valid usecase for this flag. Maybe other window managers provide similar functionality, that could be possible. I of course won't remove --wrapper. I want to make sure that existing configurations continue to work (and the fact that I can't imagine another usecase for it doesn't have to mean that there isn't one).

While we're talking about usecases: do you have any specific usecase for --run-command in mind? Why do you want to add it?

@Hritik14
Copy link
Contributor Author

Hritik14 commented May 7, 2024

Hey @meator, it's been a while and I've moved to dwm since. As far as I can recall, the usage was inspired from https://github.com/davatorium/rofi/blob/next/doc/rofi.1.markdown#run-settings

I cannot foresee working on i3 for a considerable amount of time right now (due to my day job).

@meator
Copy link
Collaborator

meator commented May 7, 2024

I cannot foresee working on i3 for a considerable amount of time right now (due to my day job).

I3 support is fully implemented. It is optional. It can be turned on with the -I flag with j4-dmenu-desktop built from develop. I have implemented it because it is more direct and because there were some escaping issues with --wrapper i3-msg.

Neither --wrapper nor the hypothetical --run-command flag shouldn't be used for I3 integration now, because there is support for I3 IPC. These two flags could be used for some other wrappers (I don't know what wrappers could be useful, that's why I'm asking whether you have some specific usecases in mind).

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.

3 participants