[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: João Távora
Subject: Re: [mentoring] a darkroom/writeroom mode for Emacs
Date: Tue, 09 Dec 2014 11:28:22 +0000
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3.92 (darwin)

   [ Hi Rasmus, I took it from your thorough review of the code that you
   accept to mentor this, in the framework discussed earlier. In the
   future, perhaps change the subject line to "mentoring".

   The code was indeed sketchy, a bit on purpose, and I've integrated
   almost all your suggestions, but read on.

   I'll be using these double-personality comments throughout my reply
   to comment on the mentoring meta-process :-)]

Rasmus <address@hidden> writes:
> Why is your mode preferable?

    [ How convincing must the would-be-contributor be at this stage?
    Won't opening with this question intimidate him/her? ]

I haven't done a thorough survey of other extensions, but I remember
trying some that don't deal well with the margins, and none provide
something like `darkroom-tentative-mode'.

> Did you take care of the FSF paperwork?
    [ I've contributed to Emacs earlier, so yes. Again should this
      question be on top?]


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

   [ Either provide a pointer better than "somewhere", or defer to the
   future if you can't.]


> I guess it should go to ELPA, but you need to improve it.

OK, I'll improve it. Once it's in ELPA, how do I maintain it? Can I keep
using github for the upstream since I'm so familiar with it?

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

   [ There is no format, we're trying to figure it out. Yours is
   close enough.]

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

Done. [ I used the badly named M-x auto-insert, but yours was a good
suggestion ]

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

Yes, I got rid of it. 

>> (defvar darkroom-fringes-outside-margins t
> defcustom


>> (defun darkroom-margins ()
> IMO you should document every defun.

I've renamed this function `darkroom-compute-margins' and added a
minimal docstring.

>> (defun darkroom-float-to-columns (f)
> As above.

Will do in the future. [ Is this essential? ]

>> (defun darkroom-increase-margins ()
> docstring

Done. [ Interactive functions should indeed have a docstring. ]

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

This was redesigned. It should be better now. `darkroom-compute-margins'
is used only when entering `darkroom-mode', then the user can use
`darkroom-increase/decrease-margins' to tweak its decision.

>> (defun darkroom-fill-paragraph-maybe (really)
> 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.

Yes, it's opinionated and not very useful. I've removed it.

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

I made these internal variables (used the "--"), do I still need a

>> (defun darkroom-visual-mode-maybe-enable ()
> Docstring.  I don't understand the need of this feature.

I've removed it, since it didn't work very well, but the idea is that a
buffer in visual-line-mode (with soft wrapping of long lines), will
always enter darkroom-mode with nice margins that perfectly center the
text on the screen. A buffer with hard linebreaks (like this message) is
not perfect for darkroom-mode, since the margins won't center it.

But I've added a utility function `darkroom-guess-margins' that can be
set as the value for `darkroom-margins' and attempts to guess that. In
the writing of this message, for example, where I `fill-pargraph' all
the time, it has guessed the "correct" margins (see screenshot). In
code, it normally defaults to 15% margins.

It's not perfect, and could be improved. Unfortunately, and more
seriously, it doesn't work when the text scale is increase with
`darkroom-text-scale-increase' set to anything but 0, because
`window-width' doesn't know about text scaling
apparently. `window-width' can return pixels, but then how can I know
the pixel width of the current buffer's font? `frame-char-width' was
promising, but also always returns a constant value.

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

   [ Lol. ]

Sorry, I fixed it.

>>          (text-scale-increase 2)
> Should be a defcustom.


>>          (darkroom-set-margins '(0 . 0))
> I guess my old margin should be recorded.


>> (defun darkroom-maybe-enable ()
> Docstring.

It's internal now.

>>   (cond ((and (not darkroom-mode) (= (count-windows) 1))
> why count-windows?  Why would it not just use the buffer in focus?

The idea in `darkroom-tentative-mode' here is that `darkroom-mode' is
entered if and only if all but one window on the frame are deleted.

>>          (message "Hmm buffer: %s windows: %s darkroom-mode: %s"
> what does "hmm buffer" mean?

It's debug code. I commented it out.

>> (define-minor-mode darkroom-tentative-mode
>>   "Minor mode that enters `darkroom-mode' when all windws are deleted"
> Again, this seems like a feature and I have no idea about it cause you
> never explain the intended design.

Well, minus typo I did very briefly. But it should be clearer now from
the commits I did.

> Hope it helps,

It did. [ It did. ]

Attachment: darkroom-margins.png
Description: PNG image

reply via email to

[Prev in Thread] Current Thread [Next in Thread]