8000 Fix the condition to judge as win & firefox environment. by yuminyamo · Pull Request #109 · previm/previm · GitHub
[go: up one dir, main page]

Skip to content

Fix the condition to judge as win & firefox environment.#109

Merged
mattn merged 2 commits intoprevim:masterfrom
yuminyamo:master
Oct 5, 2018
Merged

Fix the condition to judge as win & firefox environment.#109
mattn merged 2 commits intoprevim:masterfrom
yuminyamo:master

Conversation

@yuminyamo
Copy link
Contributor

Added parentheses to the condition to judge as win & firefox.
Under the condition before modification, if I set chrome command in windows, previm judge as win & firefox. As a result, previm was trying to open an incorrect file path with chrome and got into file not found error.

@tyru
Copy link
tyru commented Aug 26, 2018

It can be has('win32') && g:previm_open_cmd =~? 'firefox'
vim-jp/vital.vim#589

@tyru
Copy link
tyru commented Aug 27, 2018

Checking multiple win* features are commonly used to check if current environment is Windows or not.

has('win16') || has('win32') || has('win64') || ...

But actually has('win32') also checks 64bit environment, and so on (:help feature-list).

win32			Win32 version of Vim (MS-Windows 95 and later, 32 or 64 bits)
win64			Win64 version of Vim (MS-Windows 64 bit).

@yuminyamo
Copy link
Contributor Author

Thanks for your comment.

Modified version was attached.

As far as read the help, It seems unnecessary also for 'win64unix'. But this patch did not fix it because it does not cause problems.

@tyru
Copy link
tyru commented Aug 27, 2018

Thanks for the fix!
LGTM (I'm not maintainer of this repository though 😅 ).

win64unix

Yes, maybe has('win64unix') could be always 0 (I don't read vim source code though...).

@mattn mattn merged commit 60de6e1 into previm:master Oct 5, 2018
@mattn
Copy link
Member
mattn commented Oct 5, 2018

Thank you. I'll fix the rest.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

0