Re: [mentor-request] a darkroom/writeroom mode for Emacs

[Rasmus]

address@hidden (João Távora) writes:

> Hi, I'm looking for a mentor who would help me clean up the code I have
> in
>
>    https://github.com/capitaomorte/darkroom-mode
>
> This is my implementation of the popular "darkroom/writeroom" hack, of
> which multiple versions can be found around the web. I believe mine can
> be polished to make it into Emacs.

Why is your mode preferable?
Did you take care of the FSF paperwork?

> I'm looking for pointers on how to clone the Emacs repository after the
> recent Git transition, whether to use Emacs or ELPA for it, plus any
> other tips that increase my chances.

This is surely documented somewhere.

I guess it should go to ELPA, but you need to improve it.
BTW: I'm using writeroom.el for this.

Some quick comments follow.  Note, I'm not an expert, especially on the
Emacs display engine.  I'm not sure if this is the intended format of
[mentor-request] emails.


You need to start with a proper head of the file, including the commentary
section.  Further, for ELPA dependencies, version etc. should be listed (I
guess none).  See the top of any file in your ~/.emacs.d/elpa/ folder.

> (defvar darkroom-margins 0.15

This is a defcusom.  See (info "(elisp) Variable Definitions")


> (defvar darkroom-turns-on-visual-line-mode t

See above.  I think providing a hook is better.  People can add this
themselves if they want something different from the global value.
(info "(elisp) Hooks")


> (defvar darkroom-fringes-outside-margins t

defcustom 

> (defun darkroom-margins ()

IMO you should document every defun.


> (defun darkroom-float-to-columns (f)

As above.


> (defun darkroom-increase-margins ()
docstring 

>   (interactive)
>   (when (floatp darkroom-margins)
>     (setq darkroom-margins (+ 0.05 darkroom-margins))

0.05 should at the very least be a defconst but better a defcustom or from
some global variable.
Also, you provide three ways of setting margins. 

> (defun darkroom-decrease-margins ()
>   (interactive)
>   (when (floatp darkroom-margins)
>     (setq darkroom-margins (- darkroom-margins 0.05))
>     (darkroom-set-margins)))

> (defun darkroom-fill-paragraph-maybe (really)
>   (interactive "P")
>   (cond (visual-line-mode
>          (if (not really)
>              (message "not filling paragraph")
>            (call-interactively 'fill-paragraph)
>            (message "filled paragraph even in visual-line-mode")))
>         (t
>          (call-interactively 'fill-paragraph))))

This seems quite opinionated.  Why the need to deviate from ordinary Emacs
behavior?  You need to document these features an explain why they make
sense in the docstring and preferably in the commentary as well.

> (defvar darkroom-mode-map (let ((map (make-sparse-keymap)))
>                                   (define-key map (kbd "C-M-+") 
> 'darkroom-increase-margins)
>                                   (define-key map (kbd "C-M--") 
> 'darkroom-decrease-margins)
>                                   (define-key map (kbd "M-q") 
> 'darkroom-fill-paragraph-maybe)
>                                   map))

> (defvar darkroom-saved-mode-line-format nil)
> (defvar darkroom-saved-header-line-format nil)
> (defvar darkroom-saved-visual-line-mode nil)

Please add docstring, last argument.

> (make-variable-buffer-local 'darkroom-saved-mode-line-format)
> (make-variable-buffer-local 'darkroom-saved-header-line-format)
> (make-variable-buffer-local 'darkroom-saved-visual-line-mode)

> (defun darkroom-visual-mode-maybe-enable ()
>   (when darkroom-turns-on-visual-line-mode
>     (cond (darkroom-mode
>            (setq darkroom-saved-visual-mode visual-line-mode)
>            (visual-line-mode 1))
>           (t
>            (unless darkroom-saved-visual-line-mode
>              (visual-line-mode -1))))))

Docstring.  I don't understand the need of this feature.

> (define-minor-mode darkroom-mode
>   "Minor mode emulating the darkroom editor that I never used."

This is a bad docstring since it contains uselsess info and I need to know
what darkroom is.

>   nil nil nil
>   (cond (darkroom-mode
>          (setq darkroom-saved-mode-line-format mode-line-format
>                mode-line-format nil
>                darkroom-saved-header-line-format header-line-format
>                header-line-format nil)
>          (darkroom-set-margins)
>          (darkroom-visual-mode-maybe-enable)
>          (text-scale-increase 2)

Should be a defcustom.

>          (add-hook 'window-configuration-change-hook 'darkroom-set-margins 
> nil t))
>         (t
>          (setq mode-line-format darkroom-saved-mode-line-format
>                header-line-format darkroom-saved-header-line-format)
>          (text-scale-decrease 2)
>          (darkroom-set-margins '(0 . 0))

I guess my old margin should be recorded.

>          (darkroom-visual-mode-maybe-enable)
>          (remove-hook 'window-configuration-change-hook 'darkroom-set-margins 
> t))))

> (defun darkroom-maybe-enable ()

Docstring. 

>   (cond ((and (not darkroom-mode) (= (count-windows) 1))

why count-windows?  Why would it not just use the buffer in focus?

>          (darkroom-mode 1)) 
>         ((and darkroom-mode (> (count-windows) 1))
>          (darkroom-mode -1))
>         (t
>          (message "Hmm buffer: %s windows: %s darkroom-mode: %s"
what does "hmm buffer" mean?
>                   (current-buffer)
>                   (count-windows)
>                   darkroom-mode))))


> (define-minor-mode darkroom-tentative-mode
>   "Minor mode that enters `darkroom-mode' when all windws are deleted"
>   nil "D" nil
>   (cond (darkroom-tentative-mode
>          (add-hook 'window-configuration-change-hook 'darkroom-maybe-enable 
> nil t)
>          (darkroom-maybe-enable))
>         (t
>          (if darkroom-mode (darkroom-mode -1))
>          (remove-hook 'window-configuration-change-hook 
> 'darkroom-maybe-enable t))))

Again, this seems like a feature and I have no idea about it cause you
never explain the intended design.

Hope it helps,
Rasmus

-- 
Enough with the bla bla!