Fix SyncTeX #56

rekka · 2017-06-02T08:54:12Z

Fixes the produced SyncTeX file when enabled by \synctex=1 command in the tex file. Partially fixes #49.

This is achieved by:

setting the synctex parameters as if producing pdf
correctly re-initializing synctex data structure before every tex pass

Note: For an unknown reason, synctex fails if tectonic is built in a debug mode (i.e., not built with cargo build --release). This pull request does not address this.

- reinitialize SyncTeX context when xetex is initialized before a call to `start_input`. - move local static `synctex_tag_counter` to `synctex_ctxt`

pkgw

Looks great, thanks! My only request is that you actually take out a couple of comments that I don't think really provide any insight above-and-beyond what one gets from reading the code.

pkgw · 2017-06-02T13:47:28Z

tectonic/synctex.c

@@ -18,13 +18,13 @@
 *
 * Note: these settings used to depend on the no_pdf_output variable. Tectonic
 * fixes this to 1, but we do then generally run xdvipdfmx to produce a PDF.
- * Need to investigate how to deal with this.
+ * Therefore we fix this for SyncTeX to produce pdf coordinates.


I think this whole comment paragraph can just be removed now. It was basically a note-to-self that the settings might need to be twiddled to get SyncTeX working, and you figured out the right twiddling!

pkgw · 2017-06-02T13:51:00Z

tectonic/xetexini.c

@@ -4172,9 +4172,10 @@ tt_run_engine(char *dump_name, char *input_file_name)
     */

    pdf_files_init();
+    /* Initialize SyncTeX before calling start_input, which calls into SyncTeX engine! */


I think this comment can be removed. It's clear that synctex is being initialized, and in this kind of code where a bunch of functions are being called in a row, the default assumption has to be that the order in which they're called is the order in which they need to be called.

pkgw · 2017-06-02T13:55:33Z

Oh, also ... would you be able to investigate adding a simple test case for this feature? I would envision something like the the_letter_a test, except with \synctex=1 also. The test code would then also have to load and compare the synctex output file to the expected version.

I think the way to do this would be add a check_syntex: bool argument to the do_one function in tests/tex-outputs.rs.

rekka · 2017-06-03T00:09:54Z

Thanks for the review! I removed the comments. I'm looking now into the test case.

rekka · 2017-06-03T01:53:41Z

I have added a synctex test: just a simple variation of the the_letter_a with synctex output.

(The travis failure seems unrelated, one worker timed out.)

The next step would be to hook up synctex to the memory io infrastructure, so that its output is handled in the same way as the rest of the xetex output. I'll try to look into it as well, unless you think that it is not a good idea.

pkgw · 2017-06-03T12:11:19Z

Thanks, looks great!

Yes, it is important to use the new Tectonic I/O infrastructure. It looks like this will require some moderately invasive changes to synctex.c, but hopefully nothing too complicated.

In fact, the file will get simplified a bit since you don't need to worry about the "busy" file: you can just write straight to the intended destination, and the engine will only output it if the run succeeds.

It looks like you will need to write a short function that emulates fprintf and instead uses vsnprintf to fill in a buffer that you then pass off to ttstub_output_write. See core-bridge.c for an example of how to do this.

rekka · 2017-06-03T12:32:50Z

Thanks a lot for the pointers, sounds easy enough. I'll try to get it done.

- The output of synctex is routed through new `ttstub_fprinf`. - No renaming of the synctex output file (simplifies logic). - Update the SyncTeX test. - Remove `synctex_ctxt.busy_name`. - Use `ttstub_issue_warning` for SyncTeX warnings.

pkgw

Wow, very impressive work! Just a few low-level things to tidy up that I spotted.

pkgw · 2017-06-03T19:56:58Z

tectonic/core-bridge.c

+    va_end(ap);
+
+    if (len >= BUF_SIZE) {
+        _tt_abort("tt_fprintf: buffer overflow");


This shouldn't abort on overflow. I think the best behavior is to just truncate the buffer and proceed, as in dpx-error.c (the other functions in this file should also be more careful about ensuring that the string is definitely terminated, but that's something for another PR).

pkgw · 2017-06-03T19:59:12Z

tectonic/synctex.c

+    if (synctex_ctxt.file) {
+        /* We keep the file even if no tex output is produced. I assume that this means that
+         * there was an error and tectonic will not save anything anyway. */
+        /* if (synctex_ctxt.flags.not_void) { */


Please remove the commented if statement and revise the comment above to say something like "... even if no tex output is produced (synctex_ctxt.flags.not_void = 0). I assume ..."

pkgw · 2017-06-03T20:00:06Z

tectonic/synctex.c

@@ -454,8 +367,7 @@ void synctex_sheet(integer mag)
    if (synctex_ctxt.flags.off) {
        if (INTPAR(synctex) && !synctex_ctxt.flags.warn) {
            synctex_ctxt.flags.warn = 1;
-            printf
-            ("\nSyncTeX warning: Synchronization was disabled from\nthe command line with -synctex=0\nChanging the value of \\synctex has no effect.");
+            ttstub_issue_warning("SyncTeX warning: Synchronization was disabled from\nthe command line with -synctex=0\nChanging the value of \\synctex has no effect.");


Warnings issued in this way will be prefixed with the text "warning: ", so the string should be changed to "SyncTeX was disabled from ..."

pkgw · 2017-06-03T20:00:44Z

tectonic/synctex.c

@@ -693,7 +605,7 @@ void synctex_horizontal_rule_or_glue(int32_t p, int32_t this_box __attribute__ (
            }
            break;
        default:
-            printf("\nSynchronize ERROR: unknown node type %i\n", SYNCTEX_TYPE(p));
+            ttstub_issue_warning("\nSynchronize ERROR: unknown node type %i\n", SYNCTEX_TYPE(p));


This should be ttstub_issue_error with message "unknown node %d in SyncTeX". Leading and trailing newlines are not needed with this function.

pkgw · 2017-06-03T20:01:16Z

tectonic/synctex.c

@@ -716,7 +628,7 @@ void synctex_horizontal_rule_or_glue(int32_t p, int32_t this_box __attribute__ (
            synctex_record_kern(p); /*  always record synchronously: maybe some text is outside the box  */
            break;
        default:
-            printf("\nSynchronize ERROR: unknown node type %i\n", SYNCTEX_TYPE(p));
+            ttstub_issue_warning("\nSynchronize ERROR: unknown node type %i\n", SYNCTEX_TYPE(p));


Similar change as above.

rekka · 2017-06-04T00:30:31Z

Thank you for the detailed comments. I hope this commit addresses them.

Now there are two things left to have a SyncTex support on par with xetex:

a command line argument --synctex=n, where n is
- n > 0 to enable gzipped output
- n == 0 to disable
- n < 0 to enable non-gzipped output (plain text, current behavior)
gzipped output: Am I reading it correctly that this can be enabled just by passing 1 as the second argument of ttstub_output_open because of this code?

I would like to hear your opinion on the command line argument.

pkgw · 2017-06-04T12:54:15Z

Thank you, great PR!

I will make sure to credit you in the release notes for the next version. Should I credit you as Norbert Pozar, "@rekka", or something else?

To answer your second question first, yes, just flipping that argument to ttstub_output_open will gzip the resulting file.

As for the command line argument, I am always reluctant to add new command-line arguments, but it does seem like the best approach in this case. I am not sure that it is worthwhile to let the user control whether the output file is gzipped or not. Would anyone care if the file always came out gzipped? If you actually wanted to look at it yourself you would have the knowhow to decompress it yourself, I think.

pkgw · 2017-06-04T12:57:06Z

Oh, important note: I forgot the thing that you said about SyncTex only working in release mode. I recently discovered that this can indicate that there is a variable that isn't being initialized properly. See 3a62889 . I believe that in debug mode, variables are initialized to random data, whereas in release mode, they are initialized to all zeros. So in this case there is probably something that should be initialized to zero, but isn't.

rekka · 2017-06-04T14:57:47Z

Thanks for merging! Sure, you can mention me by name.

As for the important note: I cannot seem to reproduce it anymore. I have been testing the commits here using debug builds without a hitch after the first comment. Release builds work as well. But that really does not mean much with C code. :( I will check again on my Linux machine tomorrow (using Mac now).

Command line argument: I believe it is justified here---synctex is only really useful while editing, so using \synctex=1 in the actual tex source is a bit inconvenient, it's easy to forget to remove it before submitting a preprint, etc. On the other hand, I don't see a use case for a non-gzipped synctex output. Maybe some outdated editors? I suppose the best way to go is to add only the --synctex command line option, without the parameter, that just produces a gzipped output.

I can try to prepare a pull request, if you agree with the above idea. Would calling tt_set_int_variable be the way to go to enable synctex?

pkgw · 2017-06-04T15:36:15Z

Yes, I often find that these problems only arise on Linux — I think the Mac compilers are more generous about assuming that undefined values are zero. But, I just went through and tidied up a few things (commit 0ec2e87) and now I no longer have the problem on Linux either.

I am on the same page about the command-line argument. Synctex is the sort of thing that should be set from the CLI, not within the TeX file.

And yes, tt_set_int_variable would be the appropriate API to let the Rust code control this. Err, in my tidying I just got rid of the synctex_option global variable that would have been the value to set in that function ... nothing wrong with re-adding a global variable to enable the new feature, though. (Perhaps with a more informative name.)

Finally, I noticed some things in synctex.c that look like typos; e.g., line 724. Do you have any sense if those are mistakes or not?

rekka · 2017-06-04T16:17:19Z

Interesting, a missing return value. Nice catch.

OK, I will try to implement the --synctex option and prepare a pull request.

XXX lines: these definitely look like typos, but it is hard to tell. The code itself seems to be the documentation of the synctex format... It is probably not a good idea to change this. In any case, this total_length seems to be a pretty useless information for the parser, so it does not really matter I believe. It is better to be equivalent to the official synctex here.

By the way, what is your strategy for keeping tectonic's xetex engine code, including synctex, up to date with the upstream? I think it is important to be able to pull in bugfixes. I see that you have a tectonic-staging repo set up for this, but I wonder how robust it is. In particular, the C/C++ code now spits out 1000s of lines of warnings, some of them actually look like bugs, but many of them just from the way web2c inserts parentheses (-Wparentheses-equality warning). Does it make sense to try to fix them in tectonic's version of the C/C++ code?

Merge mac os specific code.

rekka added 2 commits June 2, 2017 14:05

Fix SyncTeX coordinates

cd2639b

Fix SyncTeX (re)initialization

d578087

- reinitialize SyncTeX context when xetex is initialized before a call to `start_input`. - move local static `synctex_tag_counter` to `synctex_ctxt`

rekka mentioned this pull request Jun 2, 2017

SyncTeX support #49

Closed

pkgw requested changes Jun 2, 2017

View reviewed changes

Remove unnecessary comments

95a8a18

Add a SyncTeX test

5393690

Tweak SyncTeX to use Tectonic I/O infrastructure

9656304

- The output of synctex is routed through new `ttstub_fprinf`. - No renaming of the synctex output file (simplifies logic). - Update the SyncTeX test. - Remove `synctex_ctxt.busy_name`. - Use `ttstub_issue_warning` for SyncTeX warnings.

pkgw requested changes Jun 3, 2017

View reviewed changes

Improve SyncTeX messages

8c2a2a2

pkgw approved these changes Jun 4, 2017

View reviewed changes

pkgw merged commit b1bf328 into tectonic-typesetting:master Jun 4, 2017

rekka deleted the feat-synctex branch June 4, 2017 14:28

rekka mentioned this pull request Jun 5, 2017

Add the --synctex command line option #73

Merged

PHPirates mentioned this pull request May 5, 2019

What are the available command line arguments? #373

Closed

Mrmaxmeier pushed a commit to Mrmaxmeier/tectonic that referenced this pull request Oct 1, 2019

Merge pull request tectonic-typesetting#56 from crlf0710/macos

2342fd0

Merge mac os specific code.

Fix SyncTeX #56

Fix SyncTeX #56

Uh oh!

Conversation

rekka commented Jun 2, 2017

Uh oh!

pkgw left a comment

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 2, 2017

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 2, 2017

Choose a reason for hiding this comment

Uh oh!

pkgw commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rekka commented Jun 3, 2017

Uh oh!

rekka commented Jun 3, 2017

Uh oh!

pkgw commented Jun 3, 2017

Uh oh!

rekka commented Jun 3, 2017

Uh oh!

pkgw left a comment

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 3, 2017

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 3, 2017

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 3, 2017

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 3, 2017

Choose a reason for hiding this comment

Uh oh!

pkgw Jun 3, 2017

Choose a reason for hiding this comment

Uh oh!

rekka commented Jun 4, 2017

Uh oh!

pkgw commented Jun 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pkgw commented Jun 4, 2017

Uh oh!

rekka commented Jun 4, 2017

Uh oh!

pkgw commented Jun 4, 2017

Uh oh!

rekka commented Jun 4, 2017

Uh oh!

Uh oh!

pkgw commented Jun 2, 2017 •

edited

Loading

pkgw commented Jun 4, 2017 •

edited

Loading