On Fri, Feb 26, 2016 at 02:01:48AM -0800, Michael Stapelberg
<[email protected]> wrote:
> Thanks for the quick review. Find attached a new revision of the patch
> which addresses all your comments.
Hi, thanks a lot for that!
We ahve now looked at it in more detail, and will merge it, if you can work
on it a bit more.
Here are the thoughts we came up with:
1. the coding style should match tghe coding style of the existing file -
right now, you enforce a different coding style in your changes.
2. We think the func: prefix is both a bit ugly, and a bit limited. How about
implementing this syntax, # is a comment:
the following two are for compatibility with existing syntax:
string-without-$ # append match
string-with-$ # replace $n with matches
# these would be new:
shell:string # same as string-with-$
perl:string # get's evaled
in the former, only \$\d+ is recognised (as before), which rules out using
$1 and so on in the shell string, but that is prpobably an o.k. trade-off,
given that you can't quite them properly anywayws.
the choice for perl: and it eval'ing trhe string would be because your
example could be easily embedded in the resources this way, and perl: as
opposed to eval: is similar to other areas of urxvt.
which leaves the question if which variables should be available in the
eval string. I suggest these must be available (and documented):
$self, $1, $2...
While I think @_ should be set to the matches as well, so your func:
example could be written like this:
perl: &urxvt::ext::inplace_vim::launch
(and would arguably not cause eyedamange due to so many : and :: in
the same word :)
I think you ran into some perl difficulties, and we thought about how to fix
them:
> - while ($text =~ /$matcher->[0]/g) {
> + while (my @captured = ($text =~ /\G.*?$matcher->[0]/)) {
This would include \G.* in the match, and is thus buggy. This should work:
> + while (my @captured = ($text =~ /\G.*?\K$matcher->[0]/)) {
(i.e. add \K in between).
> + pos($text) = $+[0];
Why is this needed? pos setting and \G "should" work in list context, or
did you have an issue with it not working? (I'm not an expert at list context
matches).
> - if ($launcher !~ /\$/) {
these matches would destroy the regex match variables, you should put these
into a function such as "launcher_type", which would return something like
"append" (string-without-$), "shell" (string-$, shell:), "perl" (perl:), and
the remaining string.
the perl: eval could then be:
local @_ = @captured;
eval $launcher;
warn $@ if $@;
or something like that.
other solutions might be possible, but that would do the job and imho be an
improvement.
this way, one could also get around the limitations with shell quoting, e.g.:
*.launcher.1: perl: $self->exec_async ("mycmd", $1, $2)
What do you think?
--
The choice of a Deliantra, the free code+content MORPG
-----==- _GNU_ http://www.deliantra.net
----==-- _ generation
---==---(_)__ __ ____ __ Marc Lehmann
--==---/ / _ \/ // /\ \/ / [email protected]
-=====/_/_//_/\_,_/ /_/\_\
_______________________________________________
rxvt-unicode mailing list
[email protected]
http://lists.schmorp.de/mailman/listinfo/rxvt-unicode
