Errors in shell_check() in actions.lua

[dnebauer]

In creating a custom action I found 2 errors in the shell_check function in lua/urlview/actions.lua.

The first is in treating the return value from vim function executable as a lua boolean:

if cmd and vim.fn.executable(cmd) then

This function returns either 1 (executable found), 0 (executable not found), or -1 (not implemented). All of these values are true in lua so, as implemented here, the test will always be true. The script line should be:

if cmd and vim.fn.executable(cmd) == 1 then

The second error is deciding whether the browser successfully opened by inspecting the return value of vim function system:

local err = vim.fn.system(cmd_args)
if err ~= "" then

Some browsers issue warnings to console even though they run successfully. The correct test relies on v:shell_error being set to the shell return value of the command:

local err = vim.fn.system(args)
if vim.v.shell_error ~= 0 then

Then you need to handle providing err when non-empty.

Here is my custom action configuration which shows everything I've discussed:

-- define 'spectate' action
local actions = require("urlview.actions")
actions["spectate"] = function(raw_url)
  local utils = require("urlview.utils")
  local cmd = "qutebrowser"
  if cmd and vim.fn.executable(cmd) == 1 then
    local args = { cmd, raw_url }
    local err = vim.fn.system(args)
    if vim.v.shell_error ~= 0 then
      if err ~= "" then
        utils.log(string.format("Could not navigate link with `%s`:\n%s", cmd, err), vim.log.levels.ERROR)
      else
        utils.log(string.format("Could not navigate link with `%s`", cmd), vim.log.levels.ERROR)
      end
    end
  else
    utils.log(
      string.format("Cannot use command `%s` to navigate links (either empty or non-executable)", cmd),
      vim.log.levels.ERROR
    )
  end
end
-- set up plugin
require("urlview").setup({
  default_action = "spectate",
  default_picker = "telescope",
})

[axieax]

Hi, thank you so much for raising this issue. This has now been resolved in #61!

[dnebauer]

I suggest this has not been satisfactorily resolved. The new test for browser failure is:

if vim.v.shell_error ~= 0 or err ~= "" then

This is still mistakenly taking any form of console output as a sign the browser failed. This is not (always) true. In the case of qutebrowser, which I use, for example, it dumps a bunch of warning messages to console, and thus err is non-empty, but it runs successfully and displays the link. So, what happens is that the browser runs successfully and then the plugin tells you, nonsensically, that it failed to navigate the link.

In my view the check of err needs to be removed entirely and used only for feedback in the event v:shell_error signals that the browser call failed.