Re: DocView AutoFitting via "doc-view-autofit-mode"

[Tassilo Horn <tassilo@member.fsf.org>]

Moritz Maxeiner <moritzmaxeiner@googlemail.com> writes:

Hi Moritz,

thanks for hacking on doc-view.

Here are some minor suggestions glancing at your code.

> + (defvar doc-view-autofit-mode-map
> +   (let ((map (make-sparse-keymap)))
> +     (define-key map (kbd "C-c W") 'doc-view-autofit-width)
> +     (define-key map (kbd "C-c H") 'doc-view-autofit-height)
> +     (define-key map (kbd "C-c P") 'doc-view-autofit-page)
> +     map)
> +   "Keymap used by `doc-view-autofit-mode'.")

Bindings starting with "C-c <single-char>" are usually reserved to the
user, so you should use some other binding.  Or maybe another idea was
to make the existing fitting functions accept a prefix arg activating
the autofit mode, e.g., `W' fits width once, `C-1 W' enables width
autofitting.

> + (defun doc-view-autofit-fit ()
> +   "Fits the document in the selected window's buffer
> + delayed with a timer, so multiple calls in succession
> + don't cause as much overhead."
> +   (lexical-let
> +       ((window (selected-window)))

I think doc-view doesn't rely on dynamic scoping of non-special
variables, so you could just enable lexical binding for the complete
file and use just `let'.  See (info "(elisp)Using Lexical Binding").

> +     (if (equal doc-view-autofit-timer nil)

(not doc-view-autofit-timer) would be a bit more conventional.  Or use
just `doc-view-autofit-timer' as expression and swap the branches of the
`if'.

> +         (setq doc-view-autofit-timer
> +               (run-with-timer
> +                doc-view-autofit-timer-start nil
> +                (lambda ()
> +                  (if (window-live-p window)
> +                      (save-selected-window
> +                        (select-window window)
> +                        (cancel-timer doc-view-autofit-timer)
> +                        (setq doc-view-autofit-timer nil)
> +                        (cond
> +                         ((equal 'width doc-view-autofit-type)
> +                          (doc-view-fit-width-to-window))
> +                         ((equal 'height doc-view-autofit-type)
> +                          (doc-view-fit-height-to-window))
> +                         ((equal 'page doc-view-autofit-type)
> +                          (doc-view-fit-page-to-window))))))))

Compare symbols with `eq'.  And make that lambda a named function.

> +       (timer-inc-time doc-view-autofit-timer doc-view-autofit-timer-inc))))
> +
> + (define-minor-mode doc-view-autofit-mode
> +   "Minor mode for automatic (timer based) fitting in DocView."
> +   :lighter " AFit" :keymap doc-view-autofit-mode-map :group 'doc-view
> +   (when doc-view-autofit-mode
> +     (set (make-local-variable 'doc-view-autofit-type)
> +          doc-view-autofit-default-fit)
> +     (set (make-local-variable 'doc-view-autofit-timer) nil)
> +     (add-hook 'window-configuration-change-hook
> +               'doc-view-autofit-fit nil t)
> +     (doc-view-autofit-fit))
> +   (when (not doc-view-autofit-mode)
> +     (remove-hook 'window-configuration-change-hook
> +                  'doc-view-autofit-fit t)
> +     (when doc-view-autofit-timer
> +         (cancel-timer doc-view-autofit-timer)
> +         (setq doc-view-autofit-timer nil))
> +     (setq doc-view-autofit-type nil)))

Why 2 whens instead of one if?

I didn't have time to try it out so far.  But it looks like a useful
addition, especially when using ImageMagick as backend where rescaling
doesn't trigger a reconversion.

Bye,
Tassilo