I've added additional keybindings so that one can have up to 9 custom commands.
Also added support for passing the document file name as a command line argument by substituting "%s" (as with the context action). You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/792
-- Commit Summary --
* Added keybindings for custom commands 4-9 and support for passing the document file name (%s) as command line argument
-- File Changes --
M src/keybindings.c (36) M src/keybindings.h (6) M src/tools.c (17)
-- Patch Links --
https://github.com/geany/geany/pull/792.patch https://github.com/geany/geany/pull/792.diff
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792
Please make these two separate pull requests, don't mix more than one thing even if they relate to the same area of functionality.
Please update the manual to reflect the changes.
I would suggest use %f to substitute the filename to be consistent with build commands and because its inconsistent with the context action command which uses %s to substitute the selection.
Otherwise looks ok on first inspection.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-163033960
I will look into how to update the manual and also split the request. Good point about %f.
command_line is freed on line 255 of tools.c
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-163547742
Hard-coding more keybindings is probably not the best way. IIRC we limited the custom commands to be available as keybinding to 1-3 to not "waste" all the other Ctrl-<number> combinations for this particular feature.
Maybe it's better to remove the currently hard-coded keybindungs for custom commands and instead try to adapt the custom commands as normal keybindings, so the user can fully manage them herself. This is more work but IMO the better way to go.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-164251548
Not sure what you mean by hard coded. The new keybindings can be changed under Preferences like any other - they are simply defaults.
Perhaps they could be unbound by default so that one would manually have to bind them under Preferences. The problem with the current release is that it's impossible to set keybindings on them.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-164392662
Oops, I'm not up2date with the relevant code and didn't check the diff closely enough. Sorry for the confusion, long time ago the Ctrl-<1-3> keybindings were hard-coded but luckily, this has changed in the time between. So it's all fine, unbound by default for the new keybindings sounds good to me. Let the user choose what to bind (e.g. I'm already using Ctrl-<4-5> for other keybindings).
And I agree with Lex to split the %s/%f feature from this PR.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-164781012
@tsahlin the changes itself are good, but please split them up into two commits
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-169829244
Separate request for keybindings - https://github.com/geany/geany/pull/858
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-169955505
Uh you added merge commits. Can you please cleanup the history?
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-182470514
Nice-ish, but as usual with placeholders like this in commands, it is subject to unexpected behavior if it expands to something with control characters -- as the command is then parsed by some other logic. We already have this problem with almost every placeholder, but maybe it'd be worth not adding new ones… Here maybe it can be as simple as `g_shell_escape()` as we don't implicitly run in a shell but parse with `g_shell_parse_argv()` (on *NIX only?). Yet it can cause other issues if the user uses `sh -c "..."`, but at least it'd be consistent from the start, we wouldn't break existing commands further.
BTW, this might (unlikely) break existing custom commands if they contain a bare `%s`. There should be a way to easily enter bare `%s`. On *NIX, using `%''s` would be fine, not sure if it'd work on Windows, someone would have to check (at least the code path).
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-185281231
Needs rebase too.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-185461521
@b4n, since we don't get (any/many) complaints about that problem in all the other places it exists why not allow it here. And we can only be smart enough about quoting to be stupid, we don't know what the substitution is being used for, the `%f` need not be a separate standalone argument, and win and lin quote different.
Leave it up to the user to quote it in the command they substitute into.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-185463437
@elextr okay it might be hard-ish to quote properly, *but* it's impossible for the user to escape properly. Just plain impossible. If the `%f` expanded to i.e. `foo"bar'baz` or worse, `'foo $(rm -rf ~ 2>/dev/null) bar'` (or without the quotes that are meant to create the injection in case it's surrounded by `'` already). You can `s/quote/escape/` in my comment if you prefer, but that's the same deal.
And yes, we could just not care and hope it's all fine. Not sure if it's very sensible though.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-185729783
Restricting the user's user cases / abilities just because one can make up awkward file names like this sounds out of proportion to me.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-185733288
@b4n yes its possible to break fixed user quoting with a filename, but where are ya gonna quote `my_script --out=saved/%f.backup` ? Quoting the substituted filename is wrong.
Or `python -c "blah blah '%f.bz2' blah"`? Touching the quoting is wrong.
--- Reply to this email directly or view it on GitHub: https://github.com/geany/geany/pull/792#issuecomment-185963872
github-comments@lists.geany.org