Skip to content

Added support to Windows#85

Draft
senioria wants to merge 2 commits intoxuhdev:masterfrom
senioria:master
Draft

Added support to Windows#85
senioria wants to merge 2 commits intoxuhdev:masterfrom
senioria:master

Conversation

@senioria
Copy link

English is not my native language; please excuse typing errors.

It seems that this plugin was incompatible with Windows, as 'env ' has shown:)
So I made some small changes to make it compatible.(at least for me)

I don't know if there is still somewhere incompatible with Windows,
since I only tested :LLPStartPreview.

@stale
Copy link

stale bot commented Nov 28, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 30 days if no further activity occurs, but feel free to re-open a closed issue if needed.
Thanks for contributing to vim-llp!

@stale stale bot added the stale label Nov 28, 2019
@senioria
Copy link
Author

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 30 days if no further activity occurs, but feel free to re-open a closed issue if needed.
Thanks for contributing to vim-llp!

Please don't close this issue: I'm still watching it and waiting for a response...

@stale stale bot removed the stale label Nov 30, 2019
@xuhdev xuhdev requested a review from badouralix January 29, 2020 01:26
@xuhdev
Copy link
Owner

xuhdev commented Jan 29, 2020

Could you briefly explain how it breaks on Windows?

@senioria
Copy link
Author

Firstly, an absolute path on Windows looks like c:\\some\path, where there's a : after driver, but : is not allowed to exist in a folder name, so directly append current path to the temp dir will build an invalid path. Since all absolute path follows the rule above, simply replace : with another char or delete it from the path is a fix.

Secondly, there's no env command on Windows --- the alternative is set. What's more, set only sets the value of an environment variable, so the compile command must be seperated.

English is not my native language, and I have little experience communicating with people; please excuse me or point it out if there's anything wrong with my language.

Copy link
Owner

@xuhdev xuhdev left a comment

Choose a reason for hiding this comment

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

Thanks for your explanation. I have added additional comments. Sorry for the delay; The PR overall looks very good!

\ 'TEXINPUTS=' . l:tmp_root_dir
\ . ':' . b:livepreview_buf_data['root_dir']
\ . ': ' .
\ (has('win32') ? '&& ' : '') .
Copy link
Owner

Choose a reason for hiding this comment

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

Could you explain this line?

Copy link
Author

Choose a reason for hiding this comment

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

On *NIX, env command has the syntax env [name=value]... [command [arg]...]; but on Windows, an environment variable can only be set by the set command, whose syntax is set [variable=[string]]. So something is needed to combine the set command and the compile command. The only syntax to do this is cmd1 || cmd2 or cmd1 && cmd2 which behaves almost exactly the same as the same shell syntax. So I chose &&, the space after && is to beautify the run_cmd.

Unfortunately, according to its syntax, set can only set one variable, and the set command should be explained as set TEXMFOUTPUT="... TEXINPUTS=..."(i.e. TEXINPUTS=... itself is a part or TEXMFOUTPUT's value), but it works as TEXINPUTS is set properly... I need more work to find out the reason.

Sorry for such long explaination, I have tried my best to simplify and clearify it.


let l:src_file_tail = expand('%:p:r')
if has('win32')
" Replace ':' after driver to allow l:src_file_tail
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
" Replace ':' after driver to allow l:src_file_tail
" Replace ':' after the drive letter with an underscore to allow l:src_file_tail

@senioria
Copy link
Author

Thank you for your review! I've just found that on Windows my changes doesn't work for previewing a single file(e.g. :LLPStartPreview %)... From the error message I guess the reason is also that the path to temp dir is not set properly.

I'm sorry that I'm just a senior school student and have little time, so it may take very long for me to test and change it...

I'm not familiar with some English expressions; please excuse this...

@badouralix
Copy link
Collaborator

👋 Thanks a lot @91khr for your work on this pr 🙇

I'm not really familiar with the Windows environment, I might need a little bit of time to get a properly working setup and be able to review the pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants