[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [ELPA] New Package: resize-mode
From: |
Artur Malabarba |
Subject: |
Re: [ELPA] New Package: resize-mode |
Date: |
Sun, 22 Nov 2015 12:22:31 +0000 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (gnu/linux) |
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).