Re: [mentoring] a darkroom/writeroom mode for Emacs

[Rasmus]

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

Some comments follow.  I did it quickly (as you may notice), but it still
took 1½ South Park episodes.

> ;;; darkroom.el --- Remove visual distractions and focus on writing  -*- 
> lexical-binding: t; -*-

> ;; Copyright (C) 2014  João Távora

There's usually a clause somewhere about whether it's part of Emacs.  It's
probably not important.

> ;;; Commentary:

> ;; The main entrypoints to this extension are two minor modes

Colon IMO

> ;;
> ;;    M-x darkroom-mode
> ;;    M-x darkroom-tentative-mode
> ;;
> ;; The first makes current buffer to enter `darkroom-mode'
> ;; immediately: keeping the window configuration untouched, text is
> ;; enlarged, centered on the window with margins, and the modeline is
> ;; elided.

This paragraph is not clear and potentially tautologous.


> (defcustom darkroom-margins 0.15
>   "Margins to use in `darkroom-mode'.

> Its value can be:

> - a floating point value betweeen 0 and 1, specifies percentage of
>   window width in columns to use as a margin.

> - a cons cell (LEFT RIGHT) specifying the left and right margins
>   in columns.

> - a function of no arguments that returns a cons cell interpreted
>   like the previous option.

You might want to mention the function you wrote below.  Maybe it should
be default if you can sort out the FIXME  

> Value is effective when `darkroom-mode' is toggled, when
> changing window or by calling `darkroom-set-margins'"
>   :type 'float
>   :group 'darkroom)

So, there's both a function to calculate margins the possibility of
specifying it manually?  What takes priority?

> (defun darkroom-guess-margins ()
>   "Guess suitable margins for `darkroom-margins'.

I think there should be a blank line here.

> Collects some statistics about the buffer's line lengths, and
> apply a heuristic to figure out how wide to set the margins. If
> the buffer's paragraphs are mostly filled to `fill-column',
> margins should center it on the window, otherwise, margins of
> 0.15 percent are used."

Why the need for hardcoding?

>   ;;; FIXME: broken when darkroom-text-scale-increase is anything but
>   ;;; 0, since window-width ignores text scaling. Otherwise, a
>   ;;; suitable default to put in `darkroom-margins', I guess.

You can estimate the realized width rolling over some lines and measure.
Probably there's a more appropriate way of doing it.  

(save-excursion (- (progn (end-of-visual-line) (point))
                   (progn (beginning-of-visual-line) (point))))

Note, for understanding this you might get some insights from studying
`line-move' and `line-move-visual' (I don't know).

>   (if visual-line-mode
>       0.15
>     (let* ((window-width (window-width))
>            (line-widths (save-excursion
>                           (goto-char (point-min))
>                           (cl-loop for start = (point)
>                                    while (search-forward "\n"
>                                                          20000
>                                                          'no-error)
>                                    for width = (- (point) start 1)

I'm not sure this does what you want.  A quick test suggests it will find
the end of the line, not the "visual" end of the line.

You do you need to add this to the top of your file?

    (eval-when-compile (require 'cl))


>                                    unless (zerop width)
>                                    collect width)))
>            (_longest-width (cl-reduce #'max line-widths :initial-value 0))
>            (top-quartile-avg (cl-loop with n = (/ (length line-widths)
>                                                   4)
>                                       for width in (copy-sequence
>                                                     (sort line-widths #'>))
>                                       for i from 1 upto n
>                                       sum width into total
>                                       finally (return (/ total n 1.0)))))

This seems like overenginering.  What about something like this (not
tested, check that it works out with the floats).

(let ((n4 (/ (length n) 4)))
     (/ (apply '+ (subseq (sort line-width '>) 0 n4)) n4))

>       (cond
>        ((> top-quartile-avg
>            window-width)
>         (when (y-or-n-p
>                (format
>                 (mapconcat
>                  #'identity
>                  '("Long lines detected!"
>                    "(top 25%% average %s chars and window-width is %s)"
>                    "Perhaps turn on visual-line-mode for a better darkroom?")
>                  "\n")

Here you can use concat and format. . .  In general this functionally
seems a bit obstructive.  "Dont's ask me stuff..."

>                 top-quartile-avg window-width))
>           (visual-line-mode 1))
>         0.15)
>        ((> top-quartile-avg (* 0.9 fill-column))
>         (let ((margin (round (/ (- window-width top-quartile-avg) 2))))
>           (cons margin margin)))
>        (t
>         0.15)))))

I think you can simplify this and you should not hardcode .15.

> (defun darkroom-compute-margins ()
docstring, please.  What's the *idea* of this function. 

>   (let ((darkroom-margins
>          (if (functionp darkroom-margins)
>              (funcall darkroom-margins)
>            darkroom-margins)))
>     (cond ((consp darkroom-margins)
>            darkroom-margins)
>           ((and (floatp darkroom-margins)
>                 (< darkroom-margins 1))
>            (let ((delta (darkroom-float-to-columns darkroom-margins)))
>              (cons delta delta)))
>           (t
>            (error "Illegal value in `darkroom-margins'")))))



> (defun darkroom-float-to-columns (f)
>   (ceiling (* (let ((edges (window-edges)))
>                 (- (nth 2 edges) (nth 0 edges)))
>               f)))

(- (line-end-position) (line-beginning-position))

But you probably need to account for visual lines.

> (defvar darkroom-buffer-margins nil
>   "Buffer-local version of `darkroom-margins' defcustom.
> Set by `darkroom-set-margins'")

I think these should all be at the top of the file.

> (defun darkroom-set-margins (&optional margins)
>   "Set margins from MARGINS or `darkroom-buffer-margins'."
>   (let* ((window-configuration-change-hook nil))
>     (when margins
>       (when (null (car margins)) (setcar margins 0))
>       (when (null (cdr margins)) (setcdr margins 0)))
>     (set (make-local-variable 'darkroom-buffer-margins)
>          (or margins darkroom-buffer-margins))
>     (walk-windows #'(lambda (w)
>                       (when (eq (window-buffer w) (current-buffer))
>                         (setq fringes-outside-margins
>                               darkroom-fringes-outside-margins)
>                         ;; See description of
>                         ;; `fringes-outside-margins' for the reason
>                         ;; for this apparent noop
>                         (set-window-buffer w (current-buffer))
>                         (set-window-margins w (car darkroom-buffer-margins)
>                                             (cdr darkroom-buffer-margins))))
>                   nil
>                   'all-frames)))

Can't you use with-current-buffer?  The walk-windows seems excessive.  

You should test this when resizing windows and increasing fonts. . .

> (defun darkroom-increase-margins (increment)
>   (interactive (list darkroom-margin-increment))
>   (unless (and (consp darkroom-buffer-margins)
>                (numberp (car darkroom-buffer-margins))
>                (numberp (cdr darkroom-buffer-margins)))
>     (error "`darkroom-buffer-margins' corrupted. Must be a cons of numbers."))
>   (setcar darkroom-buffer-margins
>           (round (* (+ 1 increment) (car darkroom-buffer-margins))))
>   (setcdr darkroom-buffer-margins
>           (round (* (+ 1 increment) (cdr darkroom-buffer-margins))))

      (setq  darkroom-buffer-margins (cons · ·))

>   (darkroom-set-margins darkroom-buffer-margins))

> (defun darkroom-decrease-margins (decrement)

docstring

>   (interactive (list darkroom-margin-increment))
>   (darkroom-increase-margins (- decrement)))

> (defvar darkroom-mode-map

Top of file

>   (let ((map (make-sparse-keymap)))
>     (define-key map (kbd "C-M-+") 'darkroom-increase-margins)
>     (define-key map (kbd "C-M--") 'darkroom-decrease-margins)
>     map))

Ideally this should be unnecessary.  Maybe it should be called after
changing the font size or whatnot.

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

Top of file.  And docstring.

> (define-minor-mode darkroom-mode
>   "Remove visual distractions and focus on writing. When this
> mode is active, everything but the buffer's text is elided from
> view. The buffer margins are set so that text is centered on
> screen. Text size is increased (display engine allowing) by
> `darkroom-text-scale-increase'." nil nil nil
>   (cond (darkroom-mode
>          (set (make-local-variable 'darkroom-saved-margins) (window-margins))
>          (set (make-local-variable 'darkroom-saved-mode-line-format)
>               mode-line-format)
>          (set (make-local-variable 'darkroom-saved-header-line-format)
>               header-line-format)
>          (setq mode-line-format nil)
>          (setq header-line-format nil)
>          (text-scale-increase darkroom-text-scale-increase)
>          (darkroom-set-margins (darkroom-compute-margins))
>          (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 darkroom-text-scale-increase)

You need to actually record the size used before the mode is entered.  I
could increase the width after I enter darkroom. 

>          (let (darkroom-buffer-margins)
>            (darkroom-set-margins darkroom-saved-margins))
>          (remove-hook 'window-configuration-change-hook 'darkroom-set-margins
>                       t))))

> (defun darkroom-maybe-enable ()
>   (cond ((and (not darkroom-mode) (= (count-windows) 1))
>          (darkroom-mode 1))
>         ((and darkroom-mode (> (count-windows) 1))
>          (darkroom-mode -1))
>         (t
>          ;; (message "debug: buffer: %s windows: %s darkroom-mode: %s"
>          ;;          (current-buffer) (count-windows) darkroom-mode)
>          )))

You don't need the last clause.  Also, do you want to test if
(eq major-mode 'darkroom-mode) ?

Again, hope it helps.

—Rasmus

-- 
I feel emotional landscapes they puzzle me