R-nvim / R.nvim

Neovim plugin to edit R files
GNU General Public License v3.0
128 stars 15 forks source link

Add vim.ui.open support for pdf opening #144

Closed Ben10164 closed 1 month ago

Ben10164 commented 1 month ago

In Neovim 0.10, a new function vim.ui.open was added which opens ANY file with the system's configured application for said file's filetype.

As I'm not very familiar with the structure of this project, this PR just adds a line and suppresses an error. I implemented it quickly by letting the user specify pdfviewer = "" in their config. My hope is that someone with more familiarity to the layout and style of this repo could help me implement this in a way consistent with the repo.

(I understand that using vim.ui.open will most likely result in a loss of support for SyncTeX, but the point of this PR is to start discussing basic opening ability with the new command)

jalvesaq commented 1 month ago

The current Ubuntu Linux distribution ships Neovim 0.9.5. So, I think we can implement features that depend on Neovim 0.10, but it's better to keep compatibility with 0.9.5.

I think it makes sense to use "" to open the default PDF viewer, but users should be able to find this information in the documentation. So, I think your pull request needs to:

In the future, we can remove reference to open/xdg-open from the documentation.

Ben10164 commented 1 month ago
  • check if Neovim version is equal or higher than 0.10 (vim.fn.has("nvim-0.10")).

Added in latest commit!

  • document in doc/R.nvim.txt that if Neovim >= 0.10, "" can be used as an alternative to open or xdg-open.

Also added in latest commit, but let me know if you want me to go into further detail. Currently it is:

R.nvim will call Sumatra to open the PDF on Windows, Skim on Mac OS X, and
Zathura on Linux, but you can change the values of `synctex` and `pdfviewer`
in your config if you don't need SyncTeX support. For systems using 
Neovim v10.0+, setting `pdfviewer` to an empty string will open the PDF with 
the OS default application. Example:
>lua
   synctex = false
   pdfviewer = "open"     -- Default PDF viewer (macOS or Windows)
   pdfviewer = "xdg-open" -- Default PDF viewer (Linux)
   pdfviewer = ""         -- Default PDF viewer (Any) [Requires Neovim v0.10+]
<

(Starts at line 1483)

jalvesaq commented 1 month ago

It's OK now! Thanks!

We only have cosmetic complains from selene and stylua. To silence them:

    if vim.fn.has("nvim-0.10") == 0 and config.pdfviewer == "" then
        warn(
            'R.nvim: Having a `pdfviewer` value of `""` is only supported for Neovim 0.10+.'
        )
    end

    if vim.fn.executable(config.pdfviewer) == 0 and config.pdfviewer ~= "" then
        warn(
            "R.nvim: Please, set the value of `pdfviewer`. The application `"
                .. config.pdfviewer
                .. "` was not found."
        )
    end

and

    if config.pdfviewer == "" then
        vim.ui.open(fullpath)
        return
    end
jalvesaq commented 1 month ago

It's OK now! Thanks!

We only have cosmetic complains from selene and stylua. To silence them:

    if vim.fn.has("nvim-0.10") == 0 and config.pdfviewer == "" then
        warn(
            'R.nvim: Having a `pdfviewer` value of `""` is only supported for Neovim 0.10+.'
        )
    end

    if vim.fn.executable(config.pdfviewer) == 0 and config.pdfviewer ~= "" then
        warn(
            "R.nvim: Please, set the value of `pdfviewer`. The application `"
                .. config.pdfviewer
                .. "` was not found."
        )
    end

and

    if config.pdfviewer == "" then
        vim.ui.open(fullpath)
        return
    end
folke commented 1 month ago

You can just do:

      require("r.pdf.generic").open = vim.ui.open
jalvesaq commented 1 month ago

Thanks, @folke !

@Ben10164, I refactored your pull request to accept "" as meaning default PDF viewer regardless of Neovim's version. That way, the documentation becomes simpler than before, and, when we change the required version of Neovim to be 0.10 or higher, we will only have to delete the block converting "" into either "open" or "xg-open". However, I could only test the code on Linux. It would be good to confirm that it also works on Mac OS and Windows when pdfviewer = "".

Ben10164 commented 1 month ago

@jalvesaq Can confirms it works on MacOS!