Karthik Chikmagalur <[email protected]> writes:

> I'm working on org-latex-preview actively this week, so even incremental
> feedback will be helpful as I can get started on it.

More comments on org-latex-preview.el (part 2)

> (defun org-latex-preview--ensure-overlay (beg end)
> ...
>          (overlay-put ov 'preview-state nil) ;is fragment modified?

You are using all kinds of special overlay properties.
Please,
1. Use org- prefix for the property names
2. Move these functions after the comment describing the purpose of all
   these special overlay properties.

> (setq org-latex-preview-mode--from-overlay
>           (eq (get-char-property (point) 'org-overlay-type)
>               'org-latex-overlay))

> (into-overlay-p (eq (get-char-property (point) 'org-overlay-type)
>                             'org-latex-overlay))

What if we have multiple overlays at point? For example latex preview
overlay inside table with shrunk columns.
I think it will glitch with display-live=t

> (get-char-property (point) 'invisible))    ;Overlay in invisible region

non-nil 'invisible property does not mean that the overlay is
invisible. Depending on buffer-invisibility-spec, it may be visible.
You can use `org-invisible-p'

> (get-char-property (point) 'view-text)

So, 'view-text is boolean. I think it is worth discussing the values of
'view-text and other special properties in the comment describing how
preview-mode works.

> (while (and (< (point) end)
>                         (search-forward "\n" end t))
>               (skip-chars-forward " \t")
>               (when (> (point-max) (+ 4 (point)))
>                 (push (point) line-start-positions)))

Why not simply forward-line?

> (elem-end (- (org-element-property :end element)
>                                 (or (org-element-property :post-blank 
> element) 0)
>                                 (if (eq (char-before (org-element-property 
> :end element))
>                                         ?\n)
>                                     1 0)))

This will be broken if a latex environment has blank lines that are not
mere newlines but also contain whitespace.

>          ;; delay is reduced.  Setting an 0.05s timer isn't
>          ;; necesarily the optimal duration, but from a little
>          ;; testing it appears to be fairly reasonable.
>          (run-at-time 0.01 nil #'org-latex-preview-mode--regenerate-overlay 
> ov)

So, 0.05 or 0.01?

> When LaTeX preview auto mode is on, LaTeX fragments in Org

Leftover from previous naming.

>  (if org-latex-preview-mode
>      (progn
>        (setq org-latex-preview-mode--marker (make-marker))
>        (add-hook 'pre-command-hook 
> #'org-latex-preview-mode--handle-pre-cursor nil 'local)

What if there is indirect buffer?
What if we do M-x org-tree-to-indirect-buffer with
org-latex-preview-mode active? What it is not active? What if we disable
org-latex-preview-mode in one buffer but not another?

Note that after-change-functions in one buffer are not called when edits
are done in another indirect buffer.

> (defvar-local org-latex-preview-live--element-type nil)
>
> (defvar-local org-latex-preview-live--generator nil)

Docstrings.

> (defcustom org-latex-preview-mode-display-live '(block edit-special)

:package-version is missing.

> (defvar-local org-latex-preview-live--preview-times
>     (make-vector 3 1.0)
>   "Vector containing the last three preview run times in this buffer")
> (defvar-local org-latex-preview-live--last-hash nil
>   "Last hashed fragment when live-previewing")

Missing "." at the end

> (defvar-local org-latex-preview-live--preview-times-index 0)

docstring.

> (defconst org-latex-preview-mode-display-type 'buffer

This is a duplicate of earlier defcustom.

> (defun org-latex-preview-live--ensure-overlay (&optional ov)

This name is confusing. Unlike `org-latex-preview--ensure-overlay' it
does not always create an overlay.

> (equal major-mode (org-src-get-lang-mode "latex"))

derived-mode-p maybe?

>            (let* ((element-type
>                    (with-current-buffer org-buf
>                      (or (and (string-prefix-p
>                                "\\[" (org-element-property :value element))
>                               'latex-environment)
>                          (org-element-type element))))

Here, you do special handling of \[ environments.
However, this special handling is only when the Org buffer is not
visible. When Org buffer is visible, there is no special handling in
sight. Or is it handled elsewhere I did not find?

Also, the juggle with Org buffer visible/invisible feels fragile. Well,
it is fragile. I tried with emacs -Q, where the src edit buffer occupies
half window. It kinda works (unreliably), but when I do C-x 1,
unexpectedly, no preview appears inside src buffer.
I suspect that the opposite will be also true - if src edit buffer is
maximized at the beginning, showing the Org buffer beside, will not make
the preview move to there.
And there are also indirect buffers...

Now, about fragility.
1. make repro
2. Open
\begin{equation}
  1 + 2 = 3
\end{equation}
3. M-x org-latex-preview-mode
4. C-c '
5. Edit to have 1 + 2 = 4
Debugger entered--Lisp error: (wrong-type-argument overlayp nil)
  overlay-start(nil)
  (list (overlay-start orig-ov) (overlay-end orig-ov) content)

I saw many strange bugs depending on how and when I activated the
previews and depending whether I edited the fragment or now and
depending whether previews were present before I switch to src edit
buffer or not.

-- 
Ihor Radchenko // yantar92,
Org mode maintainer,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

Reply via email to