Re: [ELPA] New Package: resize-mode

[Artur Malabarba]

Looks like a fine addition to Gelpa. The only suggestion I would make
before pushing it is that it be renamed to `resize-window'. That's more
descriptive of what it does (resize-mode could mean quite a few things)
and, besides, this package is not a mode. :-)

Below are a few comments the code. These are all just small
improvements which could be addressed before or after pushing.

daniel sutton <address@hidden> writes:

> (defface rw-background-face
>   '((t (:foreground "gray40")))
>   "Face for when resizing window.")

Why is it called `background-face' if it only sets the foreground?

Also, the current convention is to not have `-face' at the end of face
names (I know, a lot of built-in faces have this, but they're for
backwards compatibility).        

> (defvar rw-dispatch-alist
>   ;; (key function description allow-caps-for-scaled)
>   ()
>   "List of actions for `rw-dispatch-default.")
>
> (setq rw-dispatch-alist
>       '((?n rw-enlarge-down          " Resize - Expand down" t)
>         (?p rw-enlarge-up            " Resize - Expand up" t)
>         (?f rw-enlarge-horizontally  " Resize - horizontally" t)
>         (?b rw-shrink-horizontally   " Resize - shrink horizontally"
> t)
>         (?r rw-reset-windows         " Resize - reset window layour"
> nil)
>         (?w rw-cycle-window-positive " Resize - cycle window" nil)
>         (?W rw-cycle-window-negative " Resize - cycle window" nil)
>         (?? rw-display-menu          " Resize - display menu" nil)))

Is there a reason why this is done like this? Why not just set this
value inside the defvar?

> (defun rw-execute-action (choice &optional scaled)
>   "Given a CHOICE, grab values out of the alist.
> If SCALED, then call action with the rw-capital-argument."
>   ;; (char function description)
>   (let ((action (cadr choice))
>         (description (car (cdr (cdr choice)))))
>     (progn

This progn is redundant.

> (defun resize-window ()
>   "Resize the window.
> Press n to enlarge down, p to enlarge up, b to enlarge left and f
> to enlarge right.  Current ARG is not supported."
>   (interactive)
>   (setq rw-background-overlay (rw-make-background))
>   (message "Resize mode: enter character, ? for help")
>   (let ((reading-characters t)
>         ;; allow mini-buffer to collapse after displaying menu
>         (resize-mini-windows t))
>     (while reading-characters
>       (let* ((char (read-char-exclusive))
>              (choice (assoc char rw-dispatch-alist))
>              (capital (assoc (+ char 32) rw-dispatch-alist)))
>         (if choice
>             (rw-execute-action choice)
>           (if (and capital (rw-allows-capitals capital))
>               (rw-execute-action capital t)
>             (progn
>               (setq reading-characters nil)
>               (delete-overlay rw-background-overlay))))))))

Really small nitpick, but this would read better as a cond. (And that
progn is redundant).