Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to disable reftex-parse-all #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

atreyasha
Copy link
Collaborator

@atreyasha atreyasha commented Oct 1, 2022

Overview

This PR addresses #11 by adding an option to disable the call to reftex-parse-all, which could be the cause of the performance sink reported.

Tasks before merge

@TobiasZawada
Copy link

TobiasZawada commented Oct 2, 2022

Thank you for that improvement.

Side note: I personally think a hook would have been better. You could have offered reftex-parse-all and reftex-parse-one as options for that hook but also give the user the possibility to use his own parser or, maybe, no parser at all. (Please don't be offended. That is just an opinion. I am happy about your efforts even if my approach would have been slightly different!)

I ask the OP to test your changes. (I do not have his LaTeX sources.)

@TobiasZawada
Copy link

I ask the OP to test your changes. (I do not have his LaTeX sources.)

Done: https://emacs.stackexchange.com/questions/73745/company-reftex-slow-when-searching-for-labels?noredirect=1#comment120306_73745

@atreyasha
Copy link
Collaborator Author

atreyasha commented Oct 2, 2022

You could have offered reftex-parse-all and reftex-parse-one as options for that hook but also give the user the possibility to use his own parser or, maybe, no parser at all. (Please don't be offended. That is just an opinion. I am happy about your efforts even if my approach would have been slightly different!)

No problem and thanks for the clarification. Does #14 reflect the idea you had in mind?

In case I misunderstood something, it would be good if you could add a code suggestion so I can visualize this better.

Side note: I personally think a hook would have been better.

I see your point. The only thing I am hesitant about in regards to the hook is the (perhaps unnecessary) freedom provided to the user. AFAIK there are basically two ways to go about parsing the labels; either parse the single file or parse the multi-file document. Leaving this hook empty would render the parser useless. I also don't see any practical optimization that a user can make without going deep into reftex and patching stuff there (if there is anything to be patched at all). But if a user was able to do that, then it makes more sense to make upstream changes in reftex than company-reftex since we simply use the bindings available to us from reftex.

Do you know what I mean, or do you have another perspective?

I ask the OP to test your changes. (I do not have his LaTeX sources.)

Perfect, thank you.

@ls-ptr
Copy link

ls-ptr commented Oct 4, 2022

Hello everybody, I just wanted to tune in here on the problem. I implemented the change Tobias asked me for and tried it on my system. Sadly, the problem still persists. As soon as I enter a \ref{} command with the point; the autocomplete scans all my files for the Latex project.

UPDATE:
The proposed changed did solve my problem. I did not recompile the package correctly. With the implemented changes all files still get scanned, but only once. Afterwards the performance is way better and the autocomplete suggestions turn up faster. This is a nice solution for my problem, thank you!

@atreyasha
Copy link
Collaborator Author

atreyasha commented Oct 5, 2022

Hi @Captn-LootALot. Thanks for reporting back regarding the performance. One thing to clarify though:

This is a nice solution for my problem

While setting company-reftex-labels-parse-all to nil does solve your current problem, this could introduce a new problem as originally described in #11 (comment). With this option being set to nil, if you change a label in one LaTeX file A and autocomplete \ref{} in a LaTeX file B (assuming they are part of a larger document); you will no longer (automatically) have access to the changed label in LaTeX file A. Whereas with company-reftex-labels-parse-all being t, this multifile document parsing would happen automatically. Just wanted to state this as a FYI.

Afterwards the performance is way better and the autocomplete suggestions turn up faster.

Overall, I am on board to merge this PR as it constitutes a trivial but effective change. As mentioned in #13 (comment), I am less comfortable with having a customizable hook unless @TobiasZawada or @TheBB have strong opinions in favour of the customizable hook?

PS: I created a new PR #14 for the customizable hook so we keep the diffs in separate branches

@ls-ptr
Copy link

ls-ptr commented Oct 10, 2022

Hello @atreyasha thank you for the heads-up. Whenever I need to scan all documents again I usually call company-reftex-labels inside my Emacs editor. This works just fine.

@atreyasha
Copy link
Collaborator Author

atreyasha commented Oct 10, 2022

I think there is a misunderstanding here.

Whenever I need to scan all documents again I usually call company-reftex-labels inside my Emacs editor

This PR will change how company-reftex-labels works, such that calling company-reftex-labels with the company-reftex-labels-parse-all variable being nil will no longer scan all documents files in a document. So the behaviour you mentioned that "works just fine" will no longer do the same thing post-merge. Below is a MWE to verify my point.

MWE

Environment (environment.tar.gz)

init.el

;;------------------------------------------------------------------------------
;; use-package and quelpa
;;------------------------------------------------------------------------------

;; setup archives
(require 'package)
(setq package-user-dir (file-name-directory (or load-file-name (buffer-file-name))))
(add-to-list 'package-archives '("gnu"   . "https://elpa.gnu.org/packages/"))
(add-to-list 'package-archives '("melpa" . "https://melpa.org/packages/"))
(package-initialize)

;; setup use-package
(unless (package-installed-p 'use-package)
  (package-refresh-contents)
  (package-install 'use-package))
(eval-and-compile
  (setq use-package-always-ensure t
        use-package-expand-minimally t))

;; setup quelpa
(unless (package-installed-p 'quelpa)
  (with-temp-buffer
    (url-insert-file-contents "https://raw.githubusercontent.com/quelpa/quelpa/master/quelpa.el")
    (eval-buffer)
    (quelpa-self-upgrade)))
(quelpa '(company-reftex :fetcher git
                         :url "https://github.com/TheBB/company-reftex"
                         :branch "as/parse-all-defcustom"))

;;------------------------------------------------------------------------------
;; company and reftex
;;------------------------------------------------------------------------------

;; enable RefTex in emacs latex mode
(add-hook 'latex-mode-hook 'turn-on-reftex)

;; install and enable company-reftex
(use-package company
  :config
  (setq company-idle-delay nil))
(global-company-mode t)
(use-package company-reftex)

;; disable reftex-parse-all for labels
(setq company-reftex-labels-parse-all nil)

;; add new backend
(add-to-list 'company-backends 'company-reftex-labels)

main.tex

\documentclass{article}

\begin{document}
  \include{file_1}
  \include{file_2}
\end{document}

%%% Local Variables:
%%% mode: latex
%%% TeX-master: t
%%% End:

file_1.tex

\label{Chapter 1}

%%% Local Variables:
%%% mode: latex
%%% TeX-master: "main"
%%% End:

file_2.tex

\label{Chapter 2}

%%% Local Variables:
%%% mode: latex
%%% TeX-master: "main"
%%% End:

Directory structure for latex files

.
├── file_1.tex
├── file_2.tex
└── main.tex

0 directories, 3 files

Steps to reproduce

  1. Launch emacs using the aforementioned simplified init.el via emacs -q -l /path/to/simplified/init.el.

    Note: This will dump installed packages in the same directory as init.el, so it makes sense to place this init.el outside of ~/.emacs.d/

  2. Open file_1.tex and file_2.tex

  3. Open the buffer for file_1.tex and execute company-reftex-labels with the cursor inside \ref{} in a new line. You should see autocompletions for Chapter 1 and Chapter 2:

    \label{Chapter 1}
    \ref{}  ;; cursor inside the curly brackets
    
    %%% Local Variables:
    %%% mode: latex
    %%% TeX-master: "main"
    %%% End:
  4. Add a new line in file_2.tex containing \label{foo} and save this file:

    \label{Chapter 2}
    \label{foo}
    
    %%% Local Variables:
    %%% mode: latex
    %%% TeX-master: "main"
    %%% End:
  5. Repeat step 3. You should see autocompletions for Chapter 1 and Chapter 2, but not for foo

  6. Evaluate (setq company-reftex-labels-parse-all t)

  7. Repeat step 3. You should see autocompletions for Chapter 1, Chapter 2 and foo

Hope this clarifies. I will wait for responses from @TobiasZawada and/or @TheBB before merging.

@ls-ptr
Copy link

ls-ptr commented Oct 11, 2022

Thank you for coming back to this. I tested your MWE and you are absolutely right. I do understand now that what I earlier wrote cannot be possible. I will take a look at my earlier setup and see what I did wrong. Again, thank you!

@WannabeSmith
Copy link

Thanks @atreyasha, this is a nice fix. I often work with multi-file latex projects, and this PR improves performance drastically. I just call reftex-parse-all when I change a label in another file (which is not so common, so the performance increase is really worth the tradeoff).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reftex label candidates are not automatically refreshed
4 participants