mava / Nova-LaTeX

LaTeX Support for Nova
https://extensions.panic.com/extensions/info.varisco/info.varisco.LaTeX/
MIT License
3 stars 0 forks source link

Interested in merging your efforts and mine? #1

Closed flyx closed 2 years ago

flyx commented 2 years ago

Hi there,

I am the author of TeX.novaextension which provides features similar to what this project provides. From what I see after a quick look, the two projects are quite similar, using lakexmk as backend and Skim with SyncTeX as viewer. I can see a few differences:

Since there are no real conceptual differences, would you be interested in merging our projects to consolidate all features in one extension?

mava commented 2 years ago

Yes! Thank you for the suggestion. It makes a lot of sense.

From a first quick glance, your extension seems way more sophisticated than mine: I just let the language server take care of a lot of things (completions, issues, ….) that your extension handles itself. Another difference seems to be how the tasks are designed. My run task just calls Skim’s displayline (I’m personally no fan of latexmk’s -pv[c] options). And I only provide barebones task assistants. See also the last section of the extension’s README.

I’m not sure how to proceed. What are your thoughts? Thanks!

flyx commented 2 years ago

I’m not sure how to proceed.

I propose to update my extension to match yours in terms of features and usage of the native Nova API, by integrating the approaches you use where appropriate. When I'm done, we can evaluate the two extensions to make sure I didn't miss anything, and then use my code base as co-authors and move forward with that.

I hope that you don't see this as me trying to take away your work. Since I also support ConTeXt I think it makes more sense to keep my extension's name than continue with yours, and since mine has a larger user base (according to download stats on the Nova page) it would be simpler to migrate your users than mine. We could create a GitHub organization and move the repo there, it doesn't cost anything and may encourage others to contribute. What's your opinion on that?

Here's what I think I need to do on my side (I would be thankful for comments):

I just let the language server take care of a lot of things (completions, issues, ….) that your extension handles itself

I think for some features the language server is the more sensible tool to use. For example, it is non-trivial to extract error messages from latexmk's bulky output and not having to maintain that would be a relief. Plus, using a language server is more integrated to Nova – for example, if a build crashes, I have an AppleScript that opens the Issues sidebar after creating the issue via simulating a click in the Nova menu. The Nova API didn't provide a way to do this natively when I implemented it (not sure about now). I haven't tested it, but I assume getting issues from a language server is far more obvious to the user in the Nova UI.

I'd like to simply copy your language server integration to my project to take advantage of that.

My run task just calls Skim’s displayline (I’m personally no fan of latexmk’s -pv[c] options)

Yes this was quite the pain to get working. The reason I do this is because I start Skim in a shell with an env variable that contains the path to a FIFO file I use as communication channel. That was necessary, at the time, to be able to navigate code files via SyncTeX because Nova didn't have an API to open a specific file at a specific line. They added one sometime after but I'm unsure how well that works, can you share your experience with that? Skim does nova open "%file" -l %line but does this work reliably when multiple windows with multiple projects are open? e.g. if the file is not currently open (could happen if you spread your LaTeX document into multiple files) will it open in the correct project?

Doing away with the latexmk -pv stuff and using the far simpler callback that has been added to Skim in the recent release would drastically simplify my code so I hope that it is feasible.

My run task just calls Skim’s displayline (I’m personally no fan of latexmk’s -pv[c] options)

I see that displayline -r is sufficient for reloading the document (and could be done when saving any file to replicate the behaviour of my extension). It was my oversight to not do this (because I didn't notice the -r option) and use AppleScript instead.

mava commented 2 years ago

I propose to update my extension […]

Yes, thank you! Your plan sounds great. Including the GitHub organization idea — but if it’s easier to keep your repo where it is now, that would also be fine.

I hope that you don't see this as me trying to take away your work.

Not at all. No worries!

I'd like to simply copy your language server integration to my project to take advantage of that.

Please, go ahead and take whatever you want! As far as the language server integration is concerned, “my” code is literally nothing more than Nova’s Language Server Extension template[^1]. Autocompletion then just works, and issues are automatically displayed in the Issues Sidebar and in the editor gutter.

[^1]: And so it also works if, instead of TexLab, the path of a different language server is entered in the extension’s preferences; for example, Digestif.

can you share your experience with that?

I haven’t done extensive testing, but in my day-to-day work I’ve never encountered any problems. In particular:

does this work reliably when multiple windows with multiple projects are open? e.g. if the file is not currently open (could happen if you spread your LaTeX document into multiple files) will it open in the correct project?

Yes. And yes (if the project is open — if the project is not open in Nova, then the file will be opened in a stand-alone window.)

I see that displayline -r is sufficient for reloading the document […]

For what it’s worth, my extension doesn’t even use the -revert option in its run task, and I just enable “Check for file changes” in Skim’s Sync preferences (I should’ve mentioned this in the extension’s README). This way Skim (if it’s already open) updates in the background after every build task, without needing a run task. I call the run task only if I want to jump to and highlight the current line (and to open Skim the first time). And having Skim “Check for file changes” is something that I need when building PDFs outside of Nova, and Skim says to “not select this when using a script or program that will force a revert.”


One more thing: the language syntaxes. Yours are far more sophisticated than mine. But one thing I like about mine is the symbolication, including all environments, etc… — see the screenshot in the extension’s README. Sectioning commands, though, should be handled like you do, also showing the titles.

(By the way, I might have found a little bug in your syntax highlighting. If there’s something like \section{Is this $\alpha$ bug?}, with a $...$ inside a sectioning command, the first $ seems to get ignored, and so the second $ opens instead of closing the inline math scope. Oh, and something like \subsectionautorefname gets incorrectly parsed as a heading.)

flyx commented 2 years ago

For what it’s worth, my extension doesn’t even use the -revert option in its run task, and I just enable “Check for file changes” in Skim’s Sync preferences (I should’ve mentioned this in the extension’s README).

The main reason I decided against using Skim's „Check for file changes“ is that latexmk rebuilds the PDF multiple times so you might get a scrambled view of an intermediate build. I haven't tested this though so maybe Skim is intelligent enough to not do that. I assume it's less an issue if you don't use stuff like nicematrix and other things that rely heavily on multiple passes.

And having Skim “Check for file changes” is something that I need when building PDFs outside of Nova, and Skim says to “not select this when using a script or program that will force a revert.”

I noticed too that having to deselect the option is a bit unfortunate for such cases. I will experiment with using Skim's auto-check and may transition to that if it fares well.

But one thing I like about mine is the symbolication, including all environments, etc…

My choice to only do the sectioning titles was conservative in that I thought that this is what a user will certainly want to have. The main document I am editing currently has ~150 pages, so I am not that keen on having all environments in the symbols list. It would be nice to do that as an option though, I'll look into whether this could be toggled on/off via some project setting.

By the way, I might have found a little bug in your syntax highlighting.

I'll look into those. I hardly do any inline math so this was tested only rudimentary.


So it seems I have a bunch of things to do now. I'll report back when I've made advancements :).

flyx commented 2 years ago

Found time to start working on this, see novalatex-merge branch. I adopted your task setup for building via latexmk and launching Skim, while making a few conceptual changes:


I adopted your PDF engine setting, but removed the empty option and placed it only in the extension settings, not in pre-project settings. If a project contains a [.]latexmkrc file, the engine option is always ignored (so the file must set $pdf_mode, which it should do unless you want to build a .dvi file, which seems exotic). If no latexmkrc file exists, the extension setting is used (because one of the PDF engines must be used to be able to have a preview). I suppose this is what would be the correct default behavior for most use-cases. I would add a task template the user can instantiate to give them full control over the latexmk invocation to catch use-cases not covered by this.

Not being able to set a PDF engine per project will encourage users to write a latexmkrc file, which improves portability of the sources (compared to configuring the editor). This might be a bit opinionated so I'm open to discussion.

The other change I made to the setting is that -lualatex is the default. My personal experience is that this is the more sensible default for the vast majority of the world which needs some UTF-8 support for non-ASCII characters (umlauts in my case) which is a pain with pdfLaTeX. There may be drawbacks I am unaware of.


What I also did is to check whether the latexmkrc file contains @default_files and if so, create a task for each of the files listed there, instead of the default task that renders the current file. This seems sensible for any multi-file project. I haven't tested it yet so it's probably not functional yet. I have no idea how common multi-file projects are (I do like to spread my document over files) but typically, you'd at least have a separate bibliography file from which you can now render the PDF.


Since I am rather busy working on my Ph.D. thesis, I can't really promise steady updates on this, though I think the hardest part was getting started which I just did :).

mava commented 2 years ago

Great! I’ve installed your new version. I’m going to try it out and let you know if I have any comments. Thanks!

flyx commented 2 years ago

Some progress: I adopted your language server setup and generalized it so that it can also be used with ConTeXt (TexLab doesn't support that, but Digestif claims to, I have yet to test this).

The default paths for the servers are empty, in which case texlab (for LaTeX) / digestif (for ConTeXt) is searched in the login shell's PATH. This is where any (sane) installation setup would put it, I guess.

I am unsure how to avoid having both servers started – usually you are either in a LaTeX or a ConTeXt workspace and only need one of those. I'll read a bit through the Nova docs concerning that.

Skim setup seems to work well for LaTeX. Doesn't work for ConTeXt but that seems to be ConTeXt's fault and has been broken for a while.

I also adopted your project layout, putting the actual extension into a subdirectory, which is better for having tests like your project does.

Next up will be addressing the issues with the syntax you pointed out.

mava commented 2 years ago

Fantastic! Thank you. Last weekend I started using your new branch, but then I noticed that you kept working on it and so I decided to wait a bit for the dust to settle. I’ll get back to it and keep you posted.

mava commented 2 years ago

I’ve started playing around with your latest version, and it looks great! Besides the little fix for Nova 9, here are my thoughts about PDF engine settings, in relation to your comments above and the corresponding instructions in the README. Of course, feel free to ignore them!

My reason for providing an “empty” option in the extension’s settings is to allow users to set their default $pdf_mode in a global latexmkrc file (e.g., in their home directory). This is my own setup, and unfortunately it doesn’t work well with your implementation. In more detail, I have a ~/.latexmkrc file which sets my preferred $pdf_mode. Some (but not all) project folders may contain their own latexmkrc file, but most of these local latexmkrc files don’t set $pdf_mode and only configure other project-specific variables (e.g., @default_files). So with my implementation I just choose the “empty” option in the extension’s settings and I’m good to go. Of course I’m biased, since this is my own workflow, but I don’t think that such a “global latexmkrc scenario” is so unusual.

Regarding the default PDF engine, I believe that most users would expect it to be pdfLaTeX, and may be puzzled otherwise (as much as I personally like and use both XeLaTex and LuaLaTeX, too). After all, pdfLaTeX is the default in Overleaf, TeXShop, the major LaTeX extensions for other editors (VS Code, Atom, Sublime Text, …), most introductory guides to LaTeX, …. Moreover, arXiv and many (most?) academic publisher do not accept LuaLaTeX or XeLaTex. (Re: UTF-8 — I’m sure you already know this, but pdfLaTeX works perfectly fine with all the umlauts in the test.tex file in this repository. And since 2018 \usepackage[utf8]{inputenc} is no longer required.)

Just my two cents. Again, feel free to ignore them. Each implementation has its own advantages and drawbacks. Thanks!

flyx commented 2 years ago

My reason for providing an “empty” option in the extension’s settings is to allow users to set their default $pdf_mode in a global latexmkrc file (e.g., in their home directory).

Ah, I was not aware that this works. I am personally on the side of having the complete required configuration for building the document in its project folder, but other workflows should be supported on the same level. I'll re-add the option with appropriate documentation.

Moreover, arXiv and many (most?) academic publisher do not accept LuaLaTeX or XeLaTex.

This is a good point and probably the most objective reason for having pdfLaTeX as default. I have parted from pdfLaTeX a long time ago so I didn't know about the improvements, good to know they exist. I'll revert to having pdfLaTeX as default.

flyx commented 2 years ago

I looked a bit into symbolification. The problem here is that the document's semantic structure is disjoint from LaTeX' syntactic environments. A simple example:

\begin{center}
  \section{A}
\end{center}

\section{B}

Even with my current syntax, folding doesn't work properly here because Nova expects a single hierarchy. folding the \section will take precedence over \begin{center}, extending the latter's folding context beyond the end of the section.

I am unsure how to proceed here. Collapsing sections seems to be more important for longer documents than folding environments, but the environment folding context is required for auto-completion. (In this case, it won't work for the \end{center}). It might be best not to provide section folding at all, so that everything else works properly.

mava commented 2 years ago

Yes, this is inherently tricky! For me there is no doubt that environment auto-completion is the more important feature.

Section folding is less important. And the way it’s displayed in Nova is suboptimal anyway:

▶️ \section{A ⋯ \section{B}

all in one line — and preventing section B to be foldable (but one can first fold B and then A…). Notice also how, even with your current syntax, the behavior of the center environment changes in this example:

\section{0}

\begin{center}
  \section{A}
\end{center}

\section{B}

My syntax parses these two examples differently, but at least the folding regions never extend beyond their margins. I must admit, though, that I forgot most of the details — it’s been 15 months since I wrote my syntax. I’m not sure how to proceed either.

flyx commented 2 years ago

I updated the syntax to not make sectioning commands folding contexts. This fixes all issues with environment folding, while still showing the sectioning commands in the symbols sidebar. I also made environments to be tag symbols as your syntax does, making them appear in the sidebar as well. And finally I fixed the issue of \subsectionautorefname to be parsed as sectioning command.

There doesn't seem to be a way to parameterize a syntax, hence there's no option to turn on / off environment symbols. I think this is okay since with Skim preview, there is a hierarchical view of the headings in the Skim sidebar, and the search box can be used to find a specific heading.

I also didn't find an obvious way to not start both language servers but that doesn't seem to be a large issue since LaTeX and ConTeXt have different default language servers and you'd typically only have the relevant one installed unless you use both. So only the server you need is started anyway. And in the case where you do have both installed, one server never does anything so it seems okay to have it idling.

I think the current state now integrates all of the features of your extension. Do you see any more discrepancies?

I propose the next step, after we agree the merge is feature complete, would be to add you to my project as maintainer and release a new version to the Nova extension registry.

mava commented 2 years ago

Awesome! Thank you. I’m going to test the new version and then reply in more detail.

I’m seeing a weird bug with it, though. If the extension is running in some workspace and I deactivate it, then Nova crashes. Every single time. Even if no language server is running. Even if all other extensions are disabled. I’m running Nova 9.1, macOS 12.3 on an Apple M1 Max chip. Can you replicate it?

mava commented 2 years ago

Also, whenever I try open a single .tex file in Nova (so it would open in “editor” mode, as opposed to “workspace/IDE” mode), either via Nova → File → Open…, or via Finder → File → Open With → Nova, this happens:

Screen Shot 2022-03-22 at 10 22 40 PM
flyx commented 2 years ago

I've had some issues with crashes. I assumed that was related to changes in the preferences layout that wasn't always eager to update when I reloaded the extension. My assessment would be:

As for the second issue, I'll review the Nova API for that one. I assume this has to do with nova.workspace not being available for a single file, code needs to be fixed appropriately.

mava commented 2 years ago

Sounds good. Keep me posted. (Re: second issue — you've probably already found this, but just in case….)

In the meantime, some thoughts on the new syntax. As always, feel free to ignore them!

I updated the syntax to not make sectioning commands folding contexts. This fixes all issues with environment folding, while still showing the sectioning commands in the symbols sidebar. I also made environments to be tag symbols as your syntax does, making them appear in the sidebar as well. And finally I fixed the issue of \subsectionautorefname to be parsed as sectioning command.

There doesn't seem to be a way to parameterize a syntax, hence there's no option to turn on / off environment symbols. I think this is okay since with Skim preview, there is a hierarchical view of the headings in the Skim sidebar, and the search box can be used to find a specific heading.

Despite the quirks and bugs we discussed above, one advantage of “mak[ing] sectioning commands folding contexts” is that you can move the Symbols Sidebar’s slider to the left and then you only see the sectioning commands. Personally, I use this all the time (and it would be much better with section titles, like in your syntax). And I think that having some edge cases where folding doesn’t quite work is an acceptable tradeoff for a much better Symbols Sidebar.

One other thing I like is having labels in the sidebar (I’d chosen to parse them as bookmarks in my syntax). And conceptually I think that \[...\] and \(...\) should also be treated as tag and be foldable.

symbols

flyx commented 2 years ago

I played around a bit with the sectioning symbols and ended up doing what you did – grouping all sectioning commands in the same group. A problem with my approach was that Nova dislikes having symbols with priority and other symbols without priority mixed; that caused most problems. As far as I can see, the current state doesn't cause any problems even for edge cases. I opted to display the sectioning hierarchy by prepending dots on the heading's display names representing their depth (with \part being the topmost element). This means that if \section is your uppermost element you end up starting your hierarchy at two dots but that doesn't seem like much of an issue.

I added symbols for \[…\] and \(…\) as you proposed.

I also reverted your fix for Nova 9 since $FileDirname has been reverted to original behavior in Nova 9.1 (explicitly mentioned in release notes) and the old approach seems simpler. I did add a check for the argument being a TextEditor as per the docs you linked. Opening a standalone editor for a file now doesn't crash for me anymore.

I haven't figured out why Nova keeps crashing when deactivating the extension yet, I did send a crash report to Panic.

mava commented 2 years ago

Fantastic! The new syntax looks great, and you also added labels, and I love the idea of using dots!

(May I suggest 2 little changes? I would decrease the number of dots by one, so that it matches the sectional unit level number (used e.g. in secnumdepth/tocdepth). Sections have level number 1, so 1 dot; etc…. And maybe prepend a \part simply by Part:. I’d also make the dots slightly bigger: ·.)

I’ll keep playing around with the new version and keep you posted. Thank you!

mava commented 2 years ago

And speaking of simpler approaches — what do you think of this? (EDIT: Even simpler.) I didn’t want to mess with your code, and it’s easier to test in my much simpler extension. It seems to work, and there’s no need to reload tasks when paths change, nor to “find tools.”

flyx commented 2 years ago

I would decrease the number of dots by one, so that it matches the sectional unit level number (used e.g. in secnumdepth/tocdepth) […] And maybe prepend a \part simply by Part:

Good idea, I did that.

I’d also make the dots slightly bigger: ·

I was anxious that the fatter dots would be obnoxious but vastly overestimated their size in the sidebar. Changed it.

And speaking of simpler approaches — what do you think of this?

Yeah now find_tool.sh is obsolete there's no need for findTool() anymore, good suggestion. I translated this into my extension and removed that function.

mava commented 2 years ago

I finally tried out the @default_files feature, but unfortunately many of my latexmkrc files caused trouble. Things like:

One solution could be the following: throw out each name here with a * in it, then remove the extensions of the remaining ones, and finally use value + ".pdf" here. But this of course won’t work with things like @default_files = ("paper.new") referring to a paper.new.tex file. And it might break some other scenarios you have in mind. 🤷

(I think it would also be nice to have the “Current LaTeX File” task always available. Not only to work around the previous issues, but also to handle other .tex files which are not in @default_files, without having to create a custom task.)

flyx commented 2 years ago

I think it would also be nice to have the “Current LaTeX File” task always available. Not only to work around the previous issues, but also to handle other .tex files which are not in @default_files, without having to create a custom task.

There's certainly no harm in doing that, I'll implement it. Having that will have the merge providing the same functionality as your extension. I'd like to postpone further improvements to @default_files processing since this is not a feature that is part of the merge.

I experimented a bit with providing a custom „processor“ to latexmk (to replace the standard pdflatex invocation) that will simply output all processed files when called without argument. I didn't completely get it to work yet but if that would work, it would be guaranteed to discover the files latexmk will actually process without reimplementing its logic. It is not trivial though (because it needs to convince latexmk that it is pdflatex) which is why I'd like to postpone it.

mava commented 2 years ago

Sounds good!

I saw your post on the developer forum about the crash. 🤞 (By the way, it doesn’t only happen in dev mode. I tried to install the extension by opening it with Finder, and then, when you unselect it in the Extension Library, Nova crashes.) I wasted some time trying to narrow it down, and it looks like it’s caused by the LatexTaskProvider. Don’t register that assistant (even leaving on the one for ConTeXt), and I couldn’t reproduce the crash. Conversely, remove everything else (syntaxes, issue matchers (do you still need them?), commands, language servers, the ContextTaskProvider), and Nova still crashes. So I started to look at my own code for the actions. Even though my extension doesn’t crash, I always thought that the use of variable substitutions (${Command:…}, ${Config:…}) was maybe (?) a bit too hacky. I started rewriting them as resolvable actions, but I must be misunderstanding the Nova API. Here is a minimal example that I thought should work but doesn’t: “The task may be misconfigured or malformed.” But it does work for custom tasks added manually. Do you understand what I’m doing wrong?

flyx commented 2 years ago

Thanks for digging into this issue!

Do you understand what I’m doing wrong?

I think the issue is in the Nova API: A TaskResolvableAction has no way of defining which assistant should resolve it. It may be designed to be implicitly the task assistant that created it, but the docs do not explicitly state this. This might be an oversight in the API since there doesn't seem to be a reason to forbid returning a task that should be resolved by a different task assistant. In any case, my guess would be that an API-created TaskResolvableAction is missing the reference to the target assistant and thus fails resolving (we see in your example that resolveTasksAction won't be called).

mava commented 2 years ago

Yeah, that sounds about right…. But I’m completely baffled then, as I can’t see any way at all to use a TaskResolvableAction! 🤷

flyx commented 2 years ago

Probably an oversight, might be worth discussing on the devforum.

mava commented 2 years ago

Turns out, it’s just yet another Nova 9 bug! 🤦 I just thought to check in older Novas, and everything works there. So, this rewrite/cleanup works in Nova 8.4 and hopefully again in some future update.

flyx commented 2 years ago

I checked whether the LatexTaskProvider also works in Nova 8.4, but alas, it still crashes there.

mava commented 2 years ago

In the meantime, I added a BibTeX syntax to my extension. If you’d like, feel free to merge it, too.

flyx commented 2 years ago

After Nova 9.2 came out, I checked whether the crash was resolved but it wasn't. So I finally found a workaround: I merged both task providers into one. This doesn't crash for me anymore. I also added your BibTeX syntax.

Moreover, I added project-specific settings to disable LaTeX/ConTeXt so that you can disable the ConTeXt task if you don't use it (and vice versa). This will also disable the language server for the language.

mava commented 2 years ago

Great! Do you think that there’s anything else left to do? (I haven’t had much time to test your new version yet.)

(Nova 9.2 also fixed the TaskResolvableAction problem, so I posted that rewrite/cleanup mentioned earlier.)

flyx commented 2 years ago

I don't see any more issues directly related to the merge. This has been very productive and I think it's a good state to be published. I do admit that it could use more testing but I'm on the „release early, release often“ side of things.

So I renew my earlier proposition to add you to my project as maintainer and release a new version to the Nova extension registry.

mava commented 2 years ago

Sounds good! I agree, it’s been very productive — but you’ve done almost all the work. Thank you! You don’t need to add me to your repository; I can also just keep opening pull requests.

flyx commented 2 years ago

you’ve done almost all the work

Commentary is often, including here, as valuable as editing the code.

You don’t need to add me to your repository; I can also just keep opening pull requests.

As you like.

I'll go over everything, rename my repo (since I have a Tex.novaextension folder now that shouldn't be the repo name; GitHub handles redirects gracefully) and then publish the new version.

flyx commented 2 years ago

Done. Closing this issue.

mava commented 2 years ago

Awesome. Thank you!