New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make manpager.vim work on mac #2296

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@Konfekt

Konfekt commented Nov 6, 2017

under mac, the manpage name is not in the first line, but in the first NONBLANK line.

make manpager.vim work on mac
under mac, the manpage name is not in the first line, but in the first NONBLANK line.
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 6, 2017

Codecov Report

Merging #2296 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2296      +/-   ##
==========================================
+ Coverage   74.42%   74.44%   +0.01%     
==========================================
  Files          90       90              
  Lines      132130   132140      +10     
  Branches    30730    29017    -1713     
==========================================
+ Hits        98341    98372      +31     
+ Misses      33764    33743      -21     
  Partials       25       25
Impacted Files Coverage Δ
src/undo.c 81.96% <0%> (-0.07%) ⬇️
src/window.c 80.79% <0%> (-0.07%) ⬇️
src/eval.c 80.77% <0%> (-0.05%) ⬇️
src/gui.c 47.16% <0%> (ø) ⬆️
src/version.c 81.81% <0%> (ø) ⬆️
src/evalfunc.c 83.44% <0%> (+0.03%) ⬆️
src/channel.c 82.48% <0%> (+0.04%) ⬆️
src/gui_gtk_x11.c 47.8% <0%> (+0.14%) ⬆️
src/os_unix.c 53.59% <0%> (+0.18%) ⬆️
src/if_xcmdsrv.c 87.41% <0%> (+3.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53ec795...2a42726. Read the comment docs.

codecov-io commented Nov 6, 2017

Codecov Report

Merging #2296 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2296      +/-   ##
==========================================
+ Coverage   74.42%   74.44%   +0.01%     
==========================================
  Files          90       90              
  Lines      132130   132140      +10     
  Branches    30730    29017    -1713     
==========================================
+ Hits        98341    98372      +31     
+ Misses      33764    33743      -21     
  Partials       25       25
Impacted Files Coverage Δ
src/undo.c 81.96% <0%> (-0.07%) ⬇️
src/window.c 80.79% <0%> (-0.07%) ⬇️
src/eval.c 80.77% <0%> (-0.05%) ⬇️
src/gui.c 47.16% <0%> (ø) ⬆️
src/version.c 81.81% <0%> (ø) ⬆️
src/evalfunc.c 83.44% <0%> (+0.03%) ⬆️
src/channel.c 82.48% <0%> (+0.04%) ⬆️
src/gui_gtk_x11.c 47.8% <0%> (+0.14%) ⬆️
src/os_unix.c 53.59% <0%> (+0.18%) ⬆️
src/if_xcmdsrv.c 87.41% <0%> (+3.23%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53ec795...2a42726. Read the comment docs.

Konfekt added some commits Nov 7, 2017

Show outdated Hide outdated runtime/plugin/manpager.vim Outdated
@chrisbra

This comment has been minimized.

Show comment
Hide comment
@chrisbra

chrisbra Nov 7, 2017

Member

So I guess the lowercase is there because we expect most manpages about lower case commands? Can't we simply check if the manpage exists and if not try we the lower case version (or the other way around)? Something like this (pseudo code):

" only check if man page exists, check return value
call system('man' page)
if v:shell_error
   call system('man' lower(page))
endif
if v:shell_error
   echo "Man page does not exists"
endif
Member

chrisbra commented Nov 7, 2017

So I guess the lowercase is there because we expect most manpages about lower case commands? Can't we simply check if the manpage exists and if not try we the lower case version (or the other way around)? Something like this (pseudo code):

" only check if man page exists, check return value
call system('man' page)
if v:shell_error
   call system('man' lower(page))
endif
if v:shell_error
   echo "Man page does not exists"
endif
@brammool

This comment has been minimized.

Show comment
Hide comment
@brammool

brammool Nov 7, 2017

Contributor

Thanks Enno, I'll include it.

Contributor

brammool commented Nov 7, 2017

Thanks Enno, I'll include it.

@brammool brammool closed this Nov 7, 2017

@Konfekt

This comment has been minimized.

Show comment
Hide comment
@Konfekt

Konfekt Nov 8, 2017

Yeah Christian, this might be possible. Should this check be done by the manpager.vim plugin, or rather by the command :Man in the manpage.vim ftplugin ?

Perhaps some shell scripting is needed to infer which letters of the file name are upper and lower case, starting from the (lower cased) man page title.

Konfekt commented Nov 8, 2017

Yeah Christian, this might be possible. Should this check be done by the manpager.vim plugin, or rather by the command :Man in the manpage.vim ftplugin ?

Perhaps some shell scripting is needed to infer which letters of the file name are upper and lower case, starting from the (lower cased) man page title.

@lcd047

This comment has been minimized.

Show comment
Hide comment
@lcd047

lcd047 Nov 8, 2017

Can't we simply check if the manpage exists and if not try we the lower case version (or the other way around)?

@chrisbra Nope. On OpenBSD f.i. there's a zzz and a ZZZ, and they do different things.

As I said: the non-$MAN_PN case is hopeless. Which is why I suggested to give up on it, and focus instead on writing a shell script that can syntesize a $MAN_PN before running man, if man doesn't provide a $MAN_PN.

lcd047 commented Nov 8, 2017

Can't we simply check if the manpage exists and if not try we the lower case version (or the other way around)?

@chrisbra Nope. On OpenBSD f.i. there's a zzz and a ZZZ, and they do different things.

As I said: the non-$MAN_PN case is hopeless. Which is why I suggested to give up on it, and focus instead on writing a shell script that can syntesize a $MAN_PN before running man, if man doesn't provide a $MAN_PN.

@nuko8

This comment has been minimized.

Show comment
Hide comment
@nuko8

nuko8 Nov 12, 2017

@Konfekt

Did you really test the patch yourself on macOS? I confirm that it works fine for man 1 printf, man 3 printf and so on, but fails to work for, e.g., man Xorg or man DBD::File.

For macOS, when you derive the file name of a man page from the name in its header, you need to convert the latter into the former in a "smart" way: When the whole of a header name is written in upper case, do tolowerover it; otherwise, try using it as-is, just like a patch I previously sent to vim_dev, though I don't claim I gave a correct solution there.

Surely, it doesn't appear that this "smart" way always work since there's no guarantee or specifications that file names can always be derived from the corresponding header names via (conditional) case change (I can think of a case where short forms are used for some header names). But that's an implementation limitation the current plugin/manpager.vim inherently has.

N.B. the default filesystem of macOS is case-insensitive; therefore, you don't need to take the "zzz"-vs-"ZZZ" case into account for that platform.

nuko8 commented Nov 12, 2017

@Konfekt

Did you really test the patch yourself on macOS? I confirm that it works fine for man 1 printf, man 3 printf and so on, but fails to work for, e.g., man Xorg or man DBD::File.

For macOS, when you derive the file name of a man page from the name in its header, you need to convert the latter into the former in a "smart" way: When the whole of a header name is written in upper case, do tolowerover it; otherwise, try using it as-is, just like a patch I previously sent to vim_dev, though I don't claim I gave a correct solution there.

Surely, it doesn't appear that this "smart" way always work since there's no guarantee or specifications that file names can always be derived from the corresponding header names via (conditional) case change (I can think of a case where short forms are used for some header names). But that's an implementation limitation the current plugin/manpager.vim inherently has.

N.B. the default filesystem of macOS is case-insensitive; therefore, you don't need to take the "zzz"-vs-"ZZZ" case into account for that platform.

@Konfekt

This comment has been minimized.

Show comment
Hide comment
@Konfekt

Konfekt Nov 12, 2017

Dear @nuko8, no I did not. This patch does not claim to solve the problem of case sensitive man pages in upper case letters, in fact, it claims that one cannot be smart about man pages containing upper case letters and gives up on it. For the reasoning, please see

https://github.com/vim/vim/pull/2296/files/9358b2ee9cf152390e9f97b60b3c775879c78473#r149420535

That said, @rlue came up with another solution in https://github.com/vim/vim/pull/2318/files that perhaps achieves this by other means.

Konfekt commented Nov 12, 2017

Dear @nuko8, no I did not. This patch does not claim to solve the problem of case sensitive man pages in upper case letters, in fact, it claims that one cannot be smart about man pages containing upper case letters and gives up on it. For the reasoning, please see

https://github.com/vim/vim/pull/2296/files/9358b2ee9cf152390e9f97b60b3c775879c78473#r149420535

That said, @rlue came up with another solution in https://github.com/vim/vim/pull/2318/files that perhaps achieves this by other means.

@Konfekt

This comment has been minimized.

Show comment
Hide comment
@Konfekt

Konfekt Nov 12, 2017

Ok,

You refer to

+    if !has('mac') || manpage =~ '\v^[-_.:0-9A-Z]+\('
+      let manpage = tolower(manpage)
+    endif

As I understand, the additional check is due to the case insensitivity of the Mac file system. Are there perhaps platforms besides Mac whose file system is case insensitive?

What I do not understand is that you explain that

the default filesystem of macOS is case-insensitive; therefore, you don't need to take the "zzz"-vs-"ZZZ" case into account for that platform.

However, at the same time, the man pager fails because the man page file contains upper case letters?

Then, does this conversion always hit right? Aren't there any man page files that have all capital letters?

If so, as this pull request is closed, should this go into a different pull request?

PS: To correct myself, the pull request by @rlue https://github.com/vim/vim/pull/2318/files resolves a different issue.

Konfekt commented Nov 12, 2017

Ok,

You refer to

+    if !has('mac') || manpage =~ '\v^[-_.:0-9A-Z]+\('
+      let manpage = tolower(manpage)
+    endif

As I understand, the additional check is due to the case insensitivity of the Mac file system. Are there perhaps platforms besides Mac whose file system is case insensitive?

What I do not understand is that you explain that

the default filesystem of macOS is case-insensitive; therefore, you don't need to take the "zzz"-vs-"ZZZ" case into account for that platform.

However, at the same time, the man pager fails because the man page file contains upper case letters?

Then, does this conversion always hit right? Aren't there any man page files that have all capital letters?

If so, as this pull request is closed, should this go into a different pull request?

PS: To correct myself, the pull request by @rlue https://github.com/vim/vim/pull/2318/files resolves a different issue.

@rlue

This comment has been minimized.

Show comment
Hide comment
@rlue

rlue Nov 12, 2017

If so, as this pull request is closed, should this go into a different pull request?

@Konfekt is right; let's move this discussion to #2323.

rlue commented Nov 12, 2017

If so, as this pull request is closed, should this go into a different pull request?

@Konfekt is right; let's move this discussion to #2323.

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