Skip to content
This repository has been archived by the owner on Aug 14, 2022. It is now read-only.

Add SyncTeX support #31

Closed
wants to merge 7 commits into from
Closed

Add SyncTeX support #31

wants to merge 7 commits into from

Conversation

urdh
Copy link
Contributor

@urdh urdh commented Sep 29, 2014

This pull request should add SyncTeX support for the Skim opener, with keymap ctrl-alt-s (syncing on current line).

Probably these things need to be looked at before merging:

  • Instructions for the Skim side of configuration (preset in Skim requested: Skim #1364)
  • Does keeping track of the PDF file name work? Is it persistent across sessions? Is it per project/file or global as implemented?
  • Tests?

@urdh urdh mentioned this pull request Sep 29, 2014
@thomasjo
Copy link
Owner

Just wanted to chime in real quick and say that this looks really promising! Will play around with it tonight or tomorrow. Thanks!

This should make sure that each "project" stores its PDF file name
persistently across sessions, while allowing for several projects
(unlike the previous solution).
urdh added 2 commits October 5, 2014 10:43
Untested methods as of this commit include:
* latex.sync
* latex.getConfigFilePath
* latex.setOutputFilePath
* latex.getOutputFilePath
@urdh
Copy link
Contributor Author

urdh commented Oct 16, 2014

Bump. I'm not sure why the build fails, but it doesn't look like it's in the test I added.
Also, I'm not sure how to write useful tests for the other new methods so I'm calling it done for now.

@@ -112,3 +127,28 @@ module.exports =
destroyErrorIndicator: ->
@errorIndicator?.destroy()
@errorIndicator = null

getConfigFilePath: ->
filePath = path.join(atom.getConfigDirPath(), latex.cson)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've forgotten to quote the filename. This line should be

filePath = path.join(atom.getConfigDirPath(), 'latex.cson')

@thomasjo
Copy link
Owner

Testing this now. The specs are passing once the unquoted string is fixed.

I'm not entirely comfortable with the config bits you've added. I want to rely on the log files for determining the output files. It should be sufficient for the time being. I plan on making the code around log parsing more robust in the very near future, and part of this will be extending the result path parsing aspect a little bit further.

Also, is there any specific reason we need both an open method and a sync method? Couldn't we just add the lineNumber parameter to open and make it optional?

@urdh
Copy link
Contributor Author

urdh commented Oct 16, 2014

The config bits mostly ensure that the file name is remembered across sessions, but if that's not a requirement they could probably be removed.

Unifying the open and sync methods makes sense I guess.

(Note: I won't be able to work on this PR for the next week or so, I have a thesis presentation coming up.)

@thomasjo
Copy link
Owner

If you want, I can apply your changes to a branch of my own and rework them a little, then have you give it a thumbs up or down? You'll retain full credit for all your efforts of course (I'll rewrite the commit history a little bit though).


P.S. Good luck on your thesis presentation!

@urdh
Copy link
Contributor Author

urdh commented Oct 16, 2014

That works for me :)

@thomasjo
Copy link
Owner

Haven't had much time to finish the adjustments, but you can track the progress on https://github.com/thomasjo/atom-latex/tree/pull-31. Completely flooded with reports and what not right now, but I might have time to finish it tonight.

@thomasjo
Copy link
Owner

thomasjo commented Nov 2, 2014

Finally got around to finalizing the tweaks, and merging this. Just published v0.14.0.

Thanks for helping out, and thanks for your patience!

@thomasjo thomasjo closed this Nov 2, 2014
@urdh urdh deleted the skim-synctex branch November 16, 2014 22:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants