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

[joaotavora@gmail.com (João Távora)]

Rasmus <rasmus@gmx.us> writes:

>> ;; 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.

That's what M-x auto-insert gave me. I should assign it to the FSF
though.

> This paragraph is not clear and potentially tautologous.

The opening tautology is necessary to diffentiate it from
`darkroom-tentative-mode'. I wonder what else is not clear, but I have
rewritten it.

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

Yes, I had that in mind.

>> 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?

You found a bug, that second part of the docstring no longer applies.

>> (defun darkroom-guess-margins ()
>>   "Guess suitable margins for `darkroom-margins'.
> I think there should be a blank line here.

I don't think Emacs uses that particular blank, but OK.

>> 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?

To make time for southpark. Also it's cold, typing hurts and naming
variables is hard. I've fixed it.

>>   ;;; 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).

It helped. I dealt with the FIXME with the ultra-horrible
`darkroom--real-window-width'. It fixes `window-width' basically. Seems
to work. The above won't work because I need to know about window
geometry, not line geometry. Unless there's a very big line. And that's
how `darkroom--real-window-width' hacks it.

>>     (let* ((window-width (window-width))
>>            (line-widths (save-excursion
> 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.

Yes, but I want to collect actual line widths here. 

> You do you need to add this to the top of your file?
>
>     (eval-when-compile (require 'cl))

I don't think I need it. I use no compile-time cl-functions.

> 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))

Cleaner, needs cl-subseq..

>>                    "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..."

OK removed it and replaced with a message.

>
>>                 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.

Ideas welcome, I didn't think much about the heuristic, this one worked
ok. I fixed the hardcoding.

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

It's an internal function (what version of code were you looking
at). It, well, computes margins. This is a big discusison, but while I
very much sympathise with your thoroughness in documentation, docstrings
should explain the "what" carefully avoiding the "how", so you can
change that later. For internal functions, it might be a good idea to
leave them out, rather than risk them getting out of date, or hindering
others from redesigning your code. I know this from experience. Anyway,
to please my mentor, I added a docstring :-)

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

No. Here I'm concerned with the window geometry, not the current line's
geometry.

>> (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.

I prefer nearer the users. Unless the file gets big and demands a
particular structure, in which case I try to first separate by
functionality, not programming construct.

>>     (walk-windows #'(lambda (w)
>>                       (when (eq (window-buffer w) (current-buffer))
>>                   'all-frames)))
>
> Can't you use with-current-buffer?  The walk-windows seems excessive.

Keep in mind I need to affect all windows that display that buffer. There
might be several.

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

You're right. I'll do that before I submit. Did you test it?

>>   (setcdr darkroom-buffer-margins
>>           (round (* (+ 1 increment) (cdr darkroom-buffer-margins))))
>       (setq  darkroom-buffer-margins (cons · ·))

I could, but there's no big benefit and this also make the consness
explicit.

>> (defun darkroom-decrease-margins (decrement)
> docstring

You either weren't looking at the latest version, or I didn't push it.

>>   (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.

Well, the user might want to tweak it for some reason. But yes,
`darkroom--set-margins' should be called from more hooks.

>> (defvar darkroom-saved-mode-line-format nil)
>> (defvar darkroom-saved-header-line-format nil)
>> (defvar darkroom-saved-margins nil)
>
> Top of file.  And docstring.

Docstring done.

>>         (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.

I started to implement this, then realized, maybe it needn't be so,
because it works by increments. Did you try it?

>>         (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) ?

Keep it there for future debugging. I don't understand. `darkroom-mode'
is a minor mode.

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

Which ones are you watching?

João