On 10/23/06, Brad Beveridge <brad.beveridge@gmail.com > wrote:

I've just had a quick read of the patch, here are some comments:
delete-previous-char: if you look at the primitive DELETE-CHARS
function, there is an option to delete to the left or the right of the
cursor.

It looks like we do a lot of moving the cursor by one line, lets make
a new function to advance/retreat by a line and put it in cursor.lisp.
See this Emacs reference page for ideas for function names
http://www.cse.huji.ac.il/support/emacs/elisp-help/elisp-manref/elisp_30.html#SEC478

I don't like reusing other commands inside commands, ie in
change-region.  What we should do is create a DELETE-REGION function,
and have command-d-motion and change-region call that.  Same for
subst-char, I'd rather use the primitive (DELETE-CHARS), we may want
to change/remove COMMAND-X later.

Really good, probably I'm going to have to become more disiplined in
how I code now though :)  I can easily see myself writing exactly what
you did :)

If you want to send another patch (don't bother unrecording, just add
to it) feel free, otherwise I'll make the changes when I roll it in.

Cheers
Brad

On 23/10/06, Brad Beveridge <brad.beveridge@gmail.com> wrote:
> On 23/10/06, laynor@gmail.com < laynor@gmail.com> wrote:
> > Mon Oct 23 17:59:36 CEST 2006  laynor@gmail.com
> >   * normal mode commands, cc, c<motion>, s, S, -, +, J, X
> >   Added some normal mode commands:
> >   "cc": change the active line
> >   "c<motion>": change the region defined by <motion>
> >   "s": subst char, deletes a character like "x" and then goes to insert mode
> >   "S": subst line, same as "cc"
> >   "-": goto previous line beginning, moves the cursor to previous line at the 1st column
> >   "+": goto next line beginning, moves the cursor to the next line at the 1st column
> >   "X": deletes previous char (like backspace in insert mode, but it works in normal mode)
> >   "J": Joins the active line and the next line, It deserves to be fixed, because the undo is
> >        broken (it popups the debugger when the command has no effect (when applied on the last line))
> >
> >   ---- Strange Behaviors (with respect to vim) ----
> >   "c<motion>"   doesnt behave the same as vim, just try it in gvim and in vial, it's faster than
> >                 trying to explain it here. In particular, "cj" doesnt open a new line after deleting,
> >                 and cw eats the backspace that separates words.
> >   ---- Broken behavior ----
> >   join-lines-command ("J") breaks the undo. To reproduce this behavior, just go to the last line of a file
> >   and press J, and then undo.
>
> Nice!  I'll look at it tonight after work and roll it in.  I think
> I'll start a couple of new files:
> vim-differences.txt - a list of differences to Vim
> known-bugs.txt    - known bugs in Vial
>
> Thanks a lot for the patch!
>
> Cheers
> Brad
>
_______________________________________________
vial-devel mailing list
vial-devel@common-lisp.net
http://common-lisp.net/cgi-bin/mailman/listinfo/vial-devel

I think that reusing commands inside other commands is ugly too, I'm correcting some of those ugly things now.
I did it to avoid code repetition though.
I also noticed that there are some patterns in the commands code that we could try to avoid, i.e:
(defun command-function (args) body) (define-pattern ...#' command-function ...)
This one could be better expressed with a macro that outputs those two forms, maybe something like

(define-command command-function (args)
   (body)
   (<arguments for define-pattern, without the #'command-function))

also we could then specialize this macro, to avoid the pattern
(defun command-function2 (args)
  (active-register-needs-updating)
  body)

reducing our lines of code even more, and getting rid of some patterns.
I also thought of decoupling a little the commands from the actual bindings, defining a bunch of variables that contain the actual bindings, like *join-lines-command-binding*, and bound them using a macro like
(define-bindings
  (*join-lines-command-binding* "J")
  (*change-single-line-from-active-binding* "cc")
  .....)
This would make changing actual bindings clearer I think. Also I think that surely there's a better way to do something like this than "defining a bunch of variables", but I'm a noob and now i thought of it as a simple method =)

We also have a bunch of commands that go into insert-mode, thus creating the pattern
(defun foo (args)
   body
   (enter-insert-mode))
so.... what about erasing this pattern too?

Let me know what do you think!
__
Alessandro