James-Yu / LaTeX-Workshop

Boost LaTeX typesetting efficiency with preview, compile, autocomplete, colorize, and more.
MIT License
10.55k stars 522 forks source link

Fix forward SyncTex accuracy in internal browser by providing a range location indication alternative #4194

Closed pyk1998 closed 4 months ago

pyk1998 commented 6 months ago

This proposed solution for issue #4168 involves implementing a range indicator in the PDF viewer based on the line position in the editor. The solution includes several major modifications:

To use this solution, follow these steps:

By implementing these changes, the PDF viewer will display a range rectangular indicator that corresponds to the selected line in the editor.

James-Yu commented 6 months ago

Thanks for the contribution. Before looking into the implementation (which is good, just a few function merges to comply with the practice), may I know if this feature is portable to synctex.js?

@jlelong It looks like the binary is still providing a bit more info about the location over synctex.js, making #4186 valid. What do you think of adopting a priority of execution (binary > javascript) to balance utility (binary) and availability (js as fallback)?

pyk1998 commented 6 months ago

@James-Yu Thank you for the review. If there's any modification needed, please let me know. I'm very happy to have a chance learning from the community.

I've checked the related code, it looks also possible to apply the same logic to synctex.js.

By default, the forward search in synctex.js also returns a single location: https://github.com/James-Yu/LaTeX-Workshop/blob/2128e1ea0599bf7c9dee0365469de2f859532186/src/locate/synctex/worker.ts#L75-L90

However, it constains a lot of low-level code handling blocks in synctex file: https://github.com/James-Yu/LaTeX-Workshop/blob/2128e1ea0599bf7c9dee0365469de2f859532186/src/locate/synctex/worker.ts#L156-L176

To me, it is not as intuitive to modify them as simply invoking the binary. I will try to analyze them.

pyk1998 commented 6 months ago

Having tried to modifed synctex.js with no luck. Maybe the low-level synctex block handling code seems to be incomplete?

According to the comment, the synctex.js seems not handling all the blocks: https://github.com/James-Yu/LaTeX-Workshop/blob/2128e1ea0599bf7c9dee0365469de2f859532186/src/locate/synctex/worker.ts#L75-L77

A similar but more complete parser is the one used in SumatraPDF: https://github.com/sumatrapdfreader/sumatrapdf/blob/85a0fa0f09d729e06c7f6e14bf55445723fe0304/ext/synctex/synctex_parser.c#L3310-L3429

(Note that the latest parser used in https://github.com/jlaurens/synctex/blob/main/synctex_parser.c have changed from these two greatly.)

This caused the record returned from the synctex.js is not enough for viewer showing the range of the forward syntex location. From my simple opinon, using the binary directly is more convenient and easy to maintenance.

jlelong commented 6 months ago

@jlelong It looks like the binary is still providing a bit more info about the location over synctex.js, making #4186 valid. What do you think of adopting a priority of execution (binary > javascript) to balance utility (binary) and availability (js as fallback)?

I agree that we can change the order and put the binary first and only keep the js implementation as a fallback.

pyk1998 commented 5 months ago

@James-Yu Thank you so much for the detailed and kind advice. I have tried my best to merge the functions and types. However, I found that the forwardSynctex seems a lot different from forwardSynctexRange from both parameters and internal logic. I think we can jut let them separate?

pyk1998 commented 5 months ago

@James-Yu I have also tried to merge forwardSynctexRange with forwardSynctex in viewer in the latest commit. Please kindly review all the refactors.

James-Yu commented 5 months ago

Thanks for the changes. Will review soon (but not too soon).

pyk1998 commented 4 months ago

@James-Yu I think I have changed the code according to the comments. Any further updates?

James-Yu commented 4 months ago

Not having time to touch this. There are indeed a few issues with code style, yet I will directly commit to the PR shortly.

pyk1998 commented 4 months ago

@James-Yu Thank you so much for the abundant and kind work on the code style. I've really got so many lessons to learn from the followed up commits. Thank you again for the maintanence of such great extension, and I am so happy having a chance to participate. Wish you all the best.