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.
The next batch of comments on org-latex-preview.el (part 4, final) > (if (and (buffer-live-p (get-buffer org-latex-preview--latex-log)) > (get-buffer-process org-latex-preview--latex-log)) > (let ((buf (generate-new-buffer org-latex-preview--latex-log > t))) > (prog1 buf > (buffer-disable-undo buf) > (plist-put extended-info :proc-buffers (list buf)))) > (with-current-buffer ;Else reuse the standard process > buffer > (get-buffer-create org-latex-preview--latex-log t) > (setq buffer-undo-list t) > (erase-buffer) > (current-buffer))) This looks strange. Why do you need to maintain one org-latex-preview--latex-log buffer all the time? I get it that you reference it in the warnings, but does it mean that warnings will point to wrong buffers when the error happens in alternative buffer created by (generate-new-buffer org-latex-preview--latex-log t)? > ;; Re-generate the remaining fragments. > (org-latex-preview--create-image-async > (plist-get extended-info :processor) > (cdr fragments) > :latex-preamble (plist-get extended-info :latex-header) > :place-preview-p (plist-get extended-info :place-preview-p)) Is it normal here that the original :latex-processor and :appearance-options are not passed? > (unless (plist-get extended-info :fontsize) ... I feel like all these :errors/:fontsize/whatnot should be documented somewhere in a docstring or a comment in the file. They are hard to reason about otherwise, unless you are familiar with the code. > (when (if (and org-latex-preview-process-precompile > (re-search-forward "^PRELOADED FILES:" nil t)) > (re-search-forward "^ *hyperref\\.sty" nil t) > (re-search-forward "^(.*hyperref/hyperref\\.sty" nil t)) The filter will be ran in the process buffer, which means that we will be looked at global value of org-latex-preview-process-precompile. If org-latex-process-precompile have been disabled locally, the filter will still see that global value. This is potential source of bugs I guess. > (concurrentp (eq (plist-get extended-info :processor) 'dvipng)) With these kinds of assumptions, users changing dvipng entry in process-alist will run into problems. Maybe we can protect some parts of the processor making them constants? > (if (>= org-latex-preview--dvisvgm3-minor-version 1) > (mapc #'org-latex-preview--await-fragment-existance > fragments-to-show) > (mapc #'org-latex-preview--svg-make-fg-currentColor > fragments-to-show)) This looks weird. `org-latex-preview--await-fragment-existance' simply watches for file, while `org-latex-preview--svg-make-fg-currentColor' calls `org-latex-preview--await-fragment-existance' and does something else. It would be cleaner to call org-latex-preview--await-fragment-existence outside if and then call org-latex-preview--svg-make-fg-currentColor' only when version is old. > (defun org-latex-preview--svg-make-fg-currentColor (svg-fragment) > (org-latex-preview--await-fragment-existance svg-fragment) > (when path > (catch 'svg-exists > (dotimes (_ 1000) ; Check for svg existance over 1s. > (when (file-exists-p path) > (throw 'svg-exists t)) > (sleep-for 0.001))) This seemingly repeats what org-latex-preview--await-fragment-existence does. > (unless ; When the svg is incomplete, wait for it to be completed. > (string= (buffer-substring (max 1 (- (point-max) 6)) (point-max)) > "</svg>") > (catch 'svg-complete > (dotimes (_ 1000) ; Check for complete svg over 1s. > (if (string= (buffer-substring (max 1 (- (point-max) 6)) > (point-max)) > "</svg>") > (throw 'svg-complete t) > (erase-buffer) > (sleep-for 0.001) > (insert-file-contents path))) > (erase-buffer))) Why not doing all that in org-latex-preview--await-fragment-existence? > (defun org-latex-preview--place-images (extended-info &optional fragments) > If this is an export run, images will only be cached, not placed." Is org-latex-preview--place-images ever called as export run? Or is "export run" flagged by :place-preview-p? > ;; While /in theory/ this check isn't needed, sometimes the > ;; org-persist cache can be modified outside the current Emacs > ;; process. When this occurs the metadata of the fragment can > ;; still exist in `org-persist--index', but the image file is > ;; gone. This condition can be detected by checking if the > ;; `cadr' is nil (indicating the image has gone AWOL). > (if (cadr label-path-info) > (cons (cadr label-path-info) > (caddr label-path-info)) > (org-latex-preview--remove-cached key) > nil) 1. you are calling org-latx-preview--remove-cached without passing cache-location 2. This seems redundant and may be merged into > (if (and result (file-exists-p (car result))) > result > (org-latex-preview--remove-cached key cache-location) > nil) > (dolist (collection org-persist--index) > (when (equal (cadar (plist-get collection :container)) > org-latex-preview--cache-name) > (org-latex-preview--remove-cached > (plist-get (plist-get collection :associated) :key) > 'persist) > (cl-incf n))) It should be possible to search this via org-persist itself. Please add FIXME for future feature to add. > (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))) same issue with calculation of before-blank point. > (defun org-latex-preview--attr-color (attr) > "Return a RGB color for the LaTeX color package." checkdoc. With this, I went through all the changes, except unrelated to the latex preview branch. -- 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>
