Hi Al,
Thanks, I've been testing this the past few days and it's looking
very good! Response and comments below.
> - `preview-point-where`: default 'before-string, can be set to
> 'after-string and 'buframe as well.
I wonder whether the default should instead be a new setting "nil",
meaning that previews at point are not be displayed at all (the current
default behavior in AUCTeX). To explain more fully, there are three
relevant options:
- 'preview-leave-open-previews-visible' (call it "V")
- 'preview-point-where' (call it "W")
- 'preview-always-show' (call it "S")
The current behavior is as follows (independent of W):
| S | V | shown at point |
|-----+-----+----------------|
| t | nil | TeX icon |
| nil | nil | preview |
| t | t | preview |
| nil | t | preview |
First, the difference between the first two lines seems odd. Why should
S, which ostensibly controls preview visibility *away* from point, also
affect whether it is the TeX icon or the preview graphic that is shown
*at* point?
Next, the option V currently has two unrelated effects:
(1) causing the preview graphic (rather than TeX icon) to be shown at
point, and
(2) keeping the preview graphic visible when in the "disabled" state
(post-edit, pre-regeneration).
What do you think about refactoring so that (1) is controlled entirely
by W (rather than by some odd combination of S and V, as above)?
Specifically, the proposal is that if W is nil, then the TeX icon is
shown; otherwise, the preview graphic is shown, and the (non-nil) value
of W controls the precise location. This would also address what I
found odd about the above table.
With this change, the only effect of V would be (2).
> Also, I wasn't sure if I should respect `preview-auto-reveal` when
> `preview-always-show` is nil, so I've done that for now. This means that
> to truly get the same behaviour as preview-point, set
> `preview-auto-reveal` to t. Otherwise, the default value of that
> variable only shows the preview on specific key strokes.
This also seems right to me, FWIW, because it maximizes flexibility.
I noticed the following issues (A), (B), (C):
(A) With
(setq preview-always-show t)
(setq preview-point-where 'buframe)
Create an equation environment with some math, and do 'preview-point'.
Alternatively, add some math at the end of an existing equation, and do
'preview-point'. When the buframe preview updates, it does not initially
have the correct bounds -- it's typically too small to accommodate the
previewed graphic. The bounds become correct after exiting and
re-entering the preview.
(B) With
(setq preview-leave-open-previews-visible t)
(setq preview-always-show nil)
(setq preview-point-where 'buframe)
Enter a generated preview, edit it, regenerate it in place (e.g., with
'preview-at-point'), and then exit it. Both the tex source and the
preview remain visible (until the preview is regenerated, so let's
suppose automatic previewing is disabled).
If we instead use
(setq preview-leave-open-previews-visible nil)
then the TeX icon remains visible post-edit, pre-regeneration.
In either case, it seems odd that 'preview-always-show' is nil, yet the
preview and/or TeX icon stick around after point leaves the preview.
(C) With
(setq preview-point-where 'buframe)
The buframe feature doesn't work correctly with a multi-monitor setup.
On my main monitor, it works fine, but on my other two monitors, it
either doesn't display at all or displays very far from the original
preview graphic. I haven't dug in to debug this.
--
OK, now some comments on the patch (some of which are stylistic, please
feel free to ignore):
> +By default, previews are always shown when available, but this can be
> +disabled by setting @code{preview-always-show} to @code{nil}. In this
> +case, the preview is only shown when the cursor enters the corresponding
> +TeX source.
Maybe mention 'preview-point-where' here.
> +;;; preview-point customizations and variables.
> +(defcustom preview-always-show t
> + "If non-nil, always show previews.
> +
> +When nil, previews are only shown when a cursor enters their source. See
> +`preview-point-where' to control where they are shown."
> + :type 'boolean)
Check the fill on this docstring (M-q); with standard settings, I get:
When nil, previews are only shown when a cursor enters their source.
See `preview-point-where' to control where they are shown."
> +
> +(defcustom preview-point-where 'before-string
> + "Specifies where to show the preview relative to TeX source.
> +
> +Can be `before-string', `after-string' to show the preview at before or
> +after the TeX code or `buframe' to show it in a separate frame (the
> +`buframe' package must be installed). Can also be \\='(buframe FN-POS
> +FRAME-PARAMETERS BUF-PARAMETERS) where FN-POS is a position
> +function (default is `buframe-position-right-of-overlay') and
> +FRAME-PARAMETERS is an alist of additional frame parameters, default is
> +nil and BUF-PARAMETERS is an alist of buffer local variables and their
> +values."
> + :type '(choice
> + (const :tag "Before string" before-string)
> + (const :tag "After string (default)" after-string)
"(default)" seems misplaced in the docstring
> + (const :tag "On frame" buframe)
> + (list :tag "On frame with explicit parameters"
> + (function :tag "Position function")
> + (alist :tag "Frame parameters")
> + (alist :tag "Buffer parameters"))))
> +
The defcustom signature for the final option does not match the
documentation (and if we adopt the final option via the customize
interface, then it won't work correctly). Either put 'buframe in the
first entry of the list, or test for the final option via 'listp'.
> +(defface preview-disabled-face
> + '((t (:inherit shadow)))
> + "Face used when preview is disabled."
> + :group 'preview)
It's not clear to me what the purpose of this new face is. I see that
it affects the text stored in the before/after-string and strings
properties of the overlay, but not the actual TeX code.
> +
> +(defvar preview--frame nil
> + "The last active preview popup frame.")
(Maybe consider something like 'preview--buframe' or 'preview--last-buframe'
to hint at the domain scope of this variable.)
> +
> (defun preview-string-expand (arg &optional separator)
> "Expand ARG as a string.
> It can already be a string. Or it can be a list, then it is
> @@ -709,6 +745,8 @@ and tries to restart Ghostscript if necessary."
> (let* ((err (concat preview-gs-answer "\n"
> (process-name process) " " string))
> (ov (preview-gs-behead-outstanding err)))
> + (when ov
> + (preview-overlay-updated ov))
> (when (and (null ov) preview-gs-queue)
> (save-excursion
> (goto-char (if (marker-buffer (process-mark process))
> @@ -1430,16 +1468,16 @@ Try \\[ps-run-start] \\[ps-run-buffer] and \
> (ps-open
> (let ((string
> (concat
> - (mapconcat #'shell-quote-argument
> - (append (list
> - preview-gs-command
> - outfile)
> - preview-gs-command-line)
> - " ")
> - "\nGS>"
> - preview-gs-init-string
> - (aref (overlay-get ov 'queued) 1)
> - err)))
> + (mapconcat #'shell-quote-argument
> + (append (list
> + preview-gs-command
> + outfile)
> + preview-gs-command-line)
> + " ")
> + "\nGS>"
> + preview-gs-init-string
> + (aref (overlay-get ov 'queued) 1)
> + err)))
(This looks to be just a whitespace change; my understanding was that we
usually avoid pure whitespace changes unless we're touching the code
anyway, but others may know better.)
> (lambda () (interactive "@") (preview-mouse-open-error string))))
> (str
> (preview-make-clickable
> @@ -1463,7 +1501,8 @@ Try \\[ps-run-start] \\[ps-run-buffer] and \
> (apply #'preview-mouse-open-eps
> args))])))))))
> (overlay-put ov 'strings (cons str str))
> - (preview-toggle ov)))
> + (if preview-always-show
> + (preview-toggle ov))))
>
> (defun preview-gs-transact (process answer)
> "Work off Ghostscript transaction.
> @@ -1494,7 +1533,8 @@ given as ANSWER."
> (preview-ascent-from-bb
> bbox)
> (aref preview-colors 2))))
> - (overlay-put ov 'queued nil)))))
> + (overlay-put ov 'queued nil)
> + (preview-overlay-updated ov)))))
> (while (and (< (length preview-gs-outstanding)
> preview-gs-outstanding-limit)
> (setq ov (pop preview-gs-queue)))
> @@ -1706,19 +1746,24 @@ icon is cached in the property list of the SYMBOL."
> (defun preview-ascent-from-bb (bb)
> "This calculates the image ascent from its bounding box.
> The bounding box BB needs to be a 4-component vector of
> -numbers (can be float if available)."
> +numbers (can be float if available).
> +
> +If `preview-point-where' is set to \\='buframe, this simply returns
> +\\='center."
The docstring in this last case could be reworded so as to include the
additional buframe option.
> ;; baseline is at 1in from the top of letter paper (11in), so it is
> ;; at 10in from the bottom precisely, which is 720 in PostScript
> ;; coordinates. If our bounding box has its bottom not above this
> ;; line, and its top above, we can calculate a useful ascent value.
> ;; If not, something is amiss. We just use 100 in that case.
> -
> - (let ((bottom (aref bb 1))
> - (top (aref bb 3)))
> - (if (and (<= bottom 720)
> - (> top 720))
> - (round (* 100.0 (/ (- top 720.0) (- top bottom))))
> - 100)))
> + (if (or (eq preview-point-where 'buframe)
> + (eq (car-safe preview-point-where) 'buframe))
This last condition will not be satisfied if we customize according to
the defcustom signature.
> + 'center
> + (let ((bottom (aref bb 1))
> + (top (aref bb 3)))
> + (if (and (<= bottom 720)
> + (> top 720))
> + (round (* 100.0 (/ (- top 720.0) (- top bottom))))
> + 100))))
>
> (defface preview-face '((((background dark))
> (:background "dark slate gray"))
> @@ -2082,32 +2127,46 @@ If EVENT is given, it indicates the window where the
> event
> occured, either by being a mouse event or by directly being
> the window in question. This may be used for cursor restoration
> purposes."
> - (let ((old-urgent (preview-remove-urgentization ov))
> - (preview-state
> - (if (if (eq arg 'toggle)
> - (null (eq (overlay-get ov 'preview-state) 'active))
> - arg)
> - 'active
> - 'inactive))
> - (strings (overlay-get ov 'strings)))
> - (unless (eq (overlay-get ov 'preview-state) 'disabled)
> - (overlay-put ov 'preview-state preview-state)
> - (if (eq preview-state 'active)
> - (progn
> - (overlay-put ov 'category 'preview-overlay)
> - (if (eq (overlay-start ov) (overlay-end ov))
> - (overlay-put ov 'before-string (car strings))
> + (let ((old-urgent (preview-remove-urgentization ov)))
> + (unwind-protect
> + (let ((preview-state
> + (if (if (eq arg 'toggle)
> + (null (eq (overlay-get ov 'preview-state) 'active))
> + arg)
> + 'active
> + 'inactive))
> + (prop (or (and (consp preview-point-where)
> + (car preview-point-where))
> + preview-point-where))
Consider the following, which seems equivalent for practical purposes:
(prop (if (consp preview-point-where)
(car preview-point-where)
preview-point-where))
> + (strings (overlay-get ov 'strings)))
> + (unless (eq (overlay-get ov 'preview-state) 'disabled)
> + (overlay-put ov 'preview-state preview-state)
> + (if (eq preview-state 'active)
> + (progn
> + (overlay-put ov 'category 'preview-overlay)
> + (if (eq (overlay-start ov) (overlay-end ov))
> + (overlay-put ov prop (car strings))
> + (when preview-always-show
> + (dolist (prop '(display keymap mouse-face help-echo))
> + (overlay-put ov prop
> + (get-text-property 0 prop
> + (car strings)))))
> + (overlay-put ov prop nil))
> + (overlay-put ov 'face nil))
> (dolist (prop '(display keymap mouse-face help-echo))
> - (overlay-put ov prop
> - (get-text-property 0 prop (car strings))))
> - (overlay-put ov 'before-string nil))
> - (overlay-put ov 'face nil))
> - (dolist (prop '(display keymap mouse-face help-echo))
> - (overlay-put ov prop nil))
> - (overlay-put ov 'face 'preview-face)
> - (unless (cdr strings)
> - (setcdr strings (preview-inactive-string ov)))
> - (overlay-put ov 'before-string (cdr strings)))
> + (overlay-put ov prop nil))
> + (when preview-always-show
> + (overlay-put ov 'face 'preview-face))
> + (unless (cdr strings)
> + ;; If `preview-always-show' is nil, then we should
> + ;; always show the preview (i.e. leave it open) when the
> + ;; preview is "inactive".
> + (let ((preview-leave-open-previews-visible
> + (or preview-leave-open-previews-visible
> + (not preview-always-show))))
> + (setcdr strings (preview-inactive-string ov))))
This is an instance where, as discussed above, I feel that
'preview-leave-open-previews-visible' is doing more work than it should
-- it's controlling whether to show the TeX icon or the preview graphic,
but perhaps we should defer that decision to 'preview-point-where'.
> + (overlay-put ov prop (cdr strings)))
> + (preview--update-buframe ov t)))
> (if old-urgent
> (apply #'preview-add-urgentization old-urgent))))
> (if event
> @@ -2127,13 +2186,14 @@ purposes."
>
> (defun preview-mark-point ()
> "Mark position for fake intangibility."
> - (when (eq (get-char-property (point) 'preview-state) 'active)
> - (unless preview-last-location
> - (setq preview-last-location (make-marker)))
> - (set-marker preview-last-location (point))
> - (set-marker preview-marker (point))
> - (preview-move-point))
> - (set-marker preview-marker (point)))
> + (when preview-always-show
> + (when (eq (get-char-property (point) 'preview-state) 'active)
> + (unless preview-last-location
> + (setq preview-last-location (make-marker)))
> + (set-marker preview-last-location (point))
> + (set-marker preview-marker (point))
> + (preview-move-point))
> + (set-marker preview-marker (point))))
>
> (defun preview-restore-position (ov window)
> "Tweak position after opening/closing preview.
> @@ -2177,17 +2237,19 @@ overlays not in the active window."
> (current-buffer))
> (- pt (marker-position preview-marker))))))
> (preview-open-overlays lst)
> - (while lst
> - (setq lst
> - (if (and
> - (eq (overlay-get (car lst) 'preview-state) 'active)
> - (> pt (overlay-start (car lst))))
> - (overlays-at
> - (setq pt (if (and distance (< distance 0))
> - (overlay-start (car lst))
> - (overlay-end (car lst)))))
> - (cdr lst))))
> - (goto-char pt)))))
> + (when preview-always-show
> + (while lst
> + (setq lst
> + (if (and
> + (eq (overlay-get (car lst) 'preview-state) 'active)
> + (> pt (overlay-start (car lst))))
> + (overlays-at
> + (setq pt (if (and distance (< distance 0))
> + (overlay-start (car lst))
> + (overlay-end (car lst)))))
> + (cdr lst))))
> + (goto-char pt)))))
> + (preview--update-buframe))
>
> (defun preview-open-overlays (list &optional pos)
> "Open all previews in LIST, optionally restricted to enclosing POS."
> @@ -2202,7 +2264,8 @@ overlays not in the active window."
>
> (defun preview--open-for-replace (beg end &rest _)
> "Make `query-replace' open preview text about to be replaced."
> - (preview-open-overlays (overlays-in beg end)))
> + (when preview-always-show
> + (preview-open-overlays (overlays-in beg end))))
>
> (defcustom preview-query-replace-reveal t
> "Make `query-replace' autoreveal previews."
> @@ -2314,20 +2377,25 @@ active (`transient-mark-mode'), it is run through
> `preview-region'."
>
> (defun preview-disabled-string (ov)
> "Generate a before-string for disabled preview overlay OV."
> - (concat (preview-make-clickable
> - (overlay-get ov 'preview-map)
> - preview-icon
> - "\
> + (let ((ret (concat (preview-make-clickable
> + (overlay-get ov 'preview-map)
> + (if preview-leave-open-previews-visible
> + (overlay-get ov 'preview-image)
> + preview-icon)
> + "\
> %s regenerates preview
> %s more options"
> - (lambda () (interactive) (preview-regenerate ov)))
> -;; icon on separate line only for stuff starting on its own line
> - (with-current-buffer (overlay-buffer ov)
> - (save-excursion
> - (save-restriction
> - (widen)
> - (goto-char (overlay-start ov))
> - (if (bolp) "\n" ""))))))
> + (lambda () (interactive) (preview-regenerate ov)))
> + ;; icon on separate line only for stuff starting on its
> own line
> + (with-current-buffer (overlay-buffer ov)
> + (save-excursion
> + (save-restriction
> + (widen)
> + (goto-char (overlay-start ov))
> + (if (bolp) "\n" "")))))))
> + (if preview-leave-open-previews-visible
> + (propertize ret 'face 'preview-disabled-face)
> + ret)))
>
> (defun preview-disable (ovr)
> "Change overlay behaviour of OVR after source edits."
> @@ -2341,10 +2409,10 @@ active (`transient-mark-mode'), it is run through
> `preview-region'."
> (overlay-put ovr 'preview-image nil))
> (overlay-put ovr 'timestamp nil)
> (setcdr (overlay-get ovr 'strings) (preview-disabled-string ovr))
> + (preview-toggle ovr)
> (unless preview-leave-open-previews-visible
> - (preview-toggle ovr))
> - (overlay-put ovr 'preview-state 'disabled)
> - (preview--delete-overlay-files ovr))
> + (preview--delete-overlay-files ovr))
> + (overlay-put ovr 'preview-state 'disabled))
>
> (defun preview--delete-overlay-files (ovr)
> "Delete files owned by OVR."
> @@ -2618,7 +2686,8 @@ Deletes the dvi file when finished."
> (preview-ascent-from-bb
> (aref queued 0))
> (aref preview-colors 2)))
> - (overlay-put ov 'queued nil))
> + (overlay-put ov 'queued nil)
> + (preview-overlay-updated ov))
> (push filename oldfiles)
> ;; Do note modify `filenames' if we are not replacing
> ;; it, to avoid orphaning files. The filenames will be
> @@ -2759,7 +2828,8 @@ to the close hook."
> (overlay-put ov 'strings
> (list (preview-active-string ov)))
> (preview-toggle ov t)
> - (preview-clearout start end tempdir ov))))
> + (preview-clearout start end tempdir ov)
> + (preview-overlay-updated ov))))
>
> (defun preview-counter-find (begin)
> "Fetch the next preceding or next preview-counters property.
> @@ -3803,22 +3873,28 @@ name(\\([^)]+\\))\\)\\|\
> (funcall preview-find-end-function
> region-beg)
> (point)))
> - (ovl (preview-place-preview
> - snippet
> - region-beg
> - region-end
> - (preview-TeX-bb box)
> - (cons lcounters counters)
> - tempdir
> - (cdr open-data))))
> - (setq close-data (nconc ovl close-data))
> - (when (and preview-protect-point
> - (<= region-beg point-current)
> - (< point-current region-end))
> - ;; Temporarily open the preview if it
> - ;; would bump the point.
> - (preview-toggle (car ovl))
> - (push (car ovl) preview-temporary-opened)))
> + ovl)
> + (save-excursion
> + ;; Restore point to current one before
> + ;; placing preview
> + (goto-char point-current)
Is there a new reason for adding this goto-char?
> + (setq ovl (preview-place-preview
> + snippet
> + region-beg
> + region-end
> + (preview-TeX-bb box)
> + (cons lcounters counters)
> + tempdir
> + (cdr open-data)))
> + (setq close-data (nconc ovl close-data))
> + (when (and preview-protect-point
> + preview-always-show
> + (<= region-beg point-current)
> + (< point-current region-end))
> + ;; Temporarily open the preview if it
> + ;; would bump the point.
> + (preview-toggle (car ovl))
> + (push (car ovl)
> preview-temporary-opened))))
> (with-current-buffer run-buffer
> (preview-log-error
> (list 'error
> @@ -4211,7 +4287,9 @@ The functions in this variable will each be called
> inside
> `preview-region' with one argument which is a string.")
>
> (defun preview-region (begin end)
> - "Run preview on region between BEGIN and END."
> + "Run preview on region between BEGIN and END.
> +
> +It returns the started process."
> (interactive "r")
> (let ((TeX-region-extra
> ;; Write out counter information to region.
> @@ -4426,6 +4504,97 @@ If not a regular release, the date of the last
> change.")
> (insert "\n")))
> (error nil)))
>
> +;;; buframe specific
> +
> +;; Buframe functions, it will be assumed that buframe is installed so
> +;; that it can be required.
> +(declare-function buframe-position-right-of-overlay "ext:buframe"
> + (frame ov &optional location))
> +(declare-function buframe-make-buffer "ext:buframe" (name &optional locals))
> +(declare-function buframe-make "ext:buframe"
> + (frame-or-name fn-pos buffer &optional
> + parent-buffer parent-frame parameters))
> +(declare-function buframe-disable "ext:buframe" (frame-or-name
> + &optional enable))
> +
> +(defun preview--update-buframe (&optional ov force)
> + "Show or hide a buframe popup.
> +
> +Search for overlays at point having a non-nil \\='buframe property, or
> +hide it otherwise. If OV is non-nil, this overlay is used to show the
> +buframe, or hide it if it was already being shown.
> +
> +The frame is not updated if \\='buframe property has not changed, unless
> +FORCE is non-nil."
> + (if-let* ((ov (or ov
> + (cl-find-if
> + (lambda (ov) (when (overlay-get ov 'buframe) ov))
> + (overlays-at (point)))))
> + (str (overlay-get ov 'buframe)))
> + (unless (and (not force)
> + preview--frame
> + (eq (cdr (frame-parameter preview--frame
> + 'auctex-preview))
> + str))
> + (let* ((buf (buframe-make-buffer " *auctex-preview-buffer*"
> + (car-safe
> + (cddr (cdr-safe
> + preview-point-where)))))
> + (max-image-size
> + (if (integerp max-image-size)
> + max-image-size
> + ;; Set the size max-image-size using the current frame
> + ;; since the popup frame will be small to begin with
> + (* max-image-size (frame-width)))))
> + (with-current-buffer buf
> + (let (buffer-read-only)
> + (with-silent-modifications
> + (erase-buffer)
> + (insert (propertize str
> + ;; Remove unnecessary properties
> + 'help-echo nil
> + 'keymap nil
> + 'mouse-face nil))
> + (goto-char (point-min)))))
> + (setq preview--frame
> + (buframe-make
> + "auctex-preview"
> + (lambda (frame)
> + (funcall
> + (or (car-safe (cdr-safe preview-point-where))
> + #'buframe-position-right-of-overlay)
> + frame
> + ov))
> + buf
> + (overlay-buffer ov)
> + (window-frame)
> + (car-safe (cdr (cdr-safe preview-point-where)))))
> + (set-frame-parameter preview--frame 'auctex-preview
> + (cons ov str))))
> + (when (and preview--frame
> + (or (null ov)
> + ;; Do not disable the buframe if it's showing another
> + ;; overview.
> + (eq ov (car-safe (frame-parameter preview--frame
> + 'auctex-preview)))))
> + (set-frame-parameter preview--frame 'auctex-preview nil)
> + (buframe-disable preview--frame))))
> +
> +;;; preview-point
> +(defun preview-overlay-updated (ov)
> + "Mark preview OV as updated.
> +Has effect only if `preview-always-show' is nil."
> + (when (and (not preview-always-show)
> + (let ((pt (and
> + (overlay-buffer ov)
> + (with-current-buffer (overlay-buffer ov)
> + (point)))))
Consider:
(let ((pt (when-let* ((buf (overlay-buffer ov)))
(with-current-buffer buf
(point)))))
> + (eq (overlay-buffer ov) (window-buffer))
Consider: (current-buffer)
Thanks, best,
Paul
_______________________________________________
bug-auctex mailing list
[email protected]
https://lists.gnu.org/mailman/listinfo/bug-auctex
