tom-tan / auctex-latexmk

This library adds LatexMk support to AUCTeX.
93 stars 24 forks source link

support toggling pdf mode inside emacs #13

Closed izahn closed 9 years ago

izahn commented 9 years ago

Requiring users to configure latexmk by writing a .latexmkrc file makes it difficult to automate installation and configuration of this package. This PR adds support for adding the -pdf option when TeX-PDF-mode is active. This produces less surprising and more useful results out of the box, and eliminates the need to create a .latexmkrc file in most cases.

tom-tan commented 9 years ago

Thank you for sending the pull request!

However, I will not merge it because it will overwrite a setting in .latexmkrc and will break current users' setting.

For example, even if a user sets $pdf_mode=3 in his .latexmkrc (i.e. he want to use dvipdfmx instead of pdflatex), this request will use pdflatex because TeX-PDF-mode is enabled by default. I guess most CJK users use such settings because pdflatex cannot handle CJK characters.

izahn commented 9 years ago

I appreciate your thoughtful consideration, and I understand your concern. I will make only this one more appeal, and will respect your decision, but I would appreciate it if you would consider my argument before making a final decision.

It is true that TeX-PDF-mode is enabled by default, and actually I did not consider that when submitting this pull request. However, even with it being enabled by default my proposed change has a big advantage, which is that pdf generation can be easily toggled from within emacs. All that is required for those requiring $pdf_mode=3 is to add (TeX-global-PDF-mode 0), and if they ever need to override this to compile pdf 'C-c C-t C-p` is all that is required.

The main drawback I can see to my proposal is that current users who rely on auctex-latexmk ignoring TeX-PDF-mode will find that their settings no longer work as they used to. That is indeed a real problem, and I can understand if you wish to avoid it. However I believe this problem can be avoided while retaining the benefits of my proposed change. For example, this package could set (TeX-global-PDF-mode 0) and advise those who wish to use TeX-PDF-mode to set (add-hook 'LaTeX-mode-hook 'TeX-PDF-mode). Please let me know if you are open to reconsidering this pull request, or if you are open to considering a revised pull request that avoids disrupting current users as outlined above.

Again I will respect you decision and consider your response the final word.

izahn commented 9 years ago

One more quick comment: after sleeping on it I think introducing a new variable along the lines of auctex-latexmk-inherit-TeX-PDF-mode is the cleanest solution. The default can be nil so as to keep the current behavior unless the user opts in by setting it to non-nil. What do you think?

izahn commented 9 years ago

I've re-worked the pull request so that the default behavior is unchanged. Please let me know if this is acceptable.

tom-tan commented 9 years ago

One more quick comment: after sleeping on it I think introducing a new variable along the lines of auctex-latexmk-inherit-TeX-PDF-mode is the cleanest solution.

It is nice idea! It is acceptable for me.

izahn commented 9 years ago

Thank you for the suggestion. I tried it that way at first but couldn't get it to work. I will try again as soon as I have time (probably not until Monday).

izahn commented 9 years ago

I moved the if expression as you suggested; unfortunately I'm not sure I know how to make it work exactly as desired. It seems the second element of each TeX-command-list element must be a string (i.e., the if expression can't be part of the list). So the if expression has to be evaluated before being added to TeX-command-list. However, I went ahead with the change because I think it makes the code cleaner and easier to read. If you have any ideas for how to make this work better I would be very happy to hear them.

tom-tan commented 9 years ago

I moved the if expression as you suggested

What I suggested is that moving if expression to the TeX-expand-list, not TeX-command-list. That is:

  (add-to-list 'TeX-expand-list
               '("%(-PDF)"
                 (lambda ()
                   (if (and auctex-latexmk-inherit-TeX-PDF-mode ;; added
                            (not TeX-Omega-mode)
                            (or TeX-PDF-mode TeX-DVI-via-PDFTeX))
                       "-pdf" ""))))

If if expression is in TeX-command-list, it is evaluated only once when auctex-latexmk-setup is called. If it is in TeX-expand-list, auctex-latexmk-inherit-TeX-PDF-mode will be evaluated to expand "%(-PDF)" whenever "LatexMk" command is called.

izahn commented 9 years ago

Oh right, thank you! I moved the conditional to TeX-expand-list and updated the documentation to reflect the change. I also re-based on your master branch since this pull request was getting a bit complicated.

I think I've finally got this right, please let me know if you find any problems.

Finally, thank you for your patience and assistance while I fumbled through this, I really appreciate it.

izahn commented 9 years ago

I enclosed the reference to TeX-PDF-mode in quotes in the auctex-latexmk-inherit-TeX-PDF-mode docstring, thank you for the suggestion!

tom-tan commented 9 years ago

Merged. Thanks!