Skip to content
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

Implement display function to allow custom result rendering #47

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

wbthomason
Copy link

@wbthomason wbthomason commented Jun 24, 2021

Adds special handling for the display meta-field, which is an optional function returning a string from a MetaResult, and which is used to render a result for display in the selection window.

This PR is WIP. It mostly works, e.g. with the snippet:

local snap = require('snap')
local icons = require('nvim-web-devicons')
require('nvim-web-devicons').setup({default = true})

local fnamemodify = vim.fn.fnamemodify
local get_icon = icons.get_icon
local function add_icon(meta_result)
  local filename = fnamemodify(meta_result.result, ':t:r')
  local extension = fnamemodify(meta_result.result, ':e')
  local icon = get_icon(filename, extension)
  return icon .. ' ' .. meta_result.result
end

local function icons_consumer(producer)
  return function(request)
    for results in snap.consume(producer, request) do
      if type(results) == 'table' then
        if not vim.tbl_islist(results) then coroutine.yield(results) end
        coroutine.yield(vim.tbl_map(function(result)
          return snap.with_meta(result, 'display', add_icon)
        end, results))
      else
        coroutine.yield(nil)
      end
    end
  end
end

snap.register.map('n', '<Leader>s', snap.create(function()
  return {
    prompt = 'Files',
    producer = icons_consumer(snap.get('consumer.fzy')(snap.get('producer.git.file'))),
    select = snap.get('select.file').select,
    multiselect = snap.get('select.file').multiselect,
    views = {snap.get('preview.file')}
  }
end))

However:

  • It causes the results window to be empty until a filter character is pressed, even for producers which do not add a display field
  • It breaks the offsets for highlights showing character matches (I'm not sure the best way to handle this, since I don't know how those highlights are computed)
  • The icons_consumer function in the above example is not quite right - sometimes the fzy producer yields a table {continue = true}, and yielding this again causes an error.
  • The display caching doesn't work as expected, presumably because result objects are being created repeatedly and not reused. This is a minor point and can probably be ignored for now.

@camspiers: if you have a chance to take a look at this, I'd appreciate guidance on how the fzy producer works to fix the second and third items above. If you have any suggestions for why the first item may be happening, that'd be great too, but I haven't dug into that issue yet.

Closes #46.

@wbthomason wbthomason marked this pull request as ready for review June 24, 2021 07:17
@wbthomason wbthomason marked this pull request as draft June 24, 2021 07:18
@beauwilliams
Copy link
Contributor

@wbthomason I noticed same thing too (issue #1 in your above list) when adding a commit to this PR 564a9b7

Might need to check for nil in there somewhere? perhaps

local icon = get_icon(filename, extension)
  if icon ~= nil 
     return icon .. ' ' .. meta_result.result
  else
     return meta_result.result

@camspiers
Copy link
Owner

Fantastic, I can iterate on the issues you pointed out, or point you in the right direction when I get some time. I just landed #42 and so will be soon be ready for some fresh features to land.

The attempted approach to caching results was broken because display is
expected to return a string, not a meta-result. Thus, when display was
applied to a string, it created but did not return a new meta-result
table, only returning nil strings.

This could be fixed pretty easily by changing the use of display to
expect a meta-result, but I've opted to leave that as a possible future
optimization.
@wbthomason
Copy link
Author

I've fixed most of the issues with this PR, and have an updated, mostly working icon consumer example:

local snap = require 'snap'
local icons = require 'nvim-web-devicons'
require('nvim-web-devicons').setup { default = true }

local fnamemodify = vim.fn.fnamemodify
local get_icon = icons.get_icon
local function add_icon(meta_result)
  local filename = fnamemodify(meta_result.result, ':t:r')
  local extension = fnamemodify(meta_result.result, ':e')
  local icon = get_icon(filename, extension)
  return icon .. ' ' .. meta_result.result
end

local function icons_consumer(producer)
  return function(request)
    for results in snap.consume(producer, request) do
      if type(results) == 'table' then
        if not vim.tbl_islist(results) then
          coroutine.yield(results)
        else
          coroutine.yield(vim.tbl_map(function(result)
            return snap.with_meta(result, 'display', add_icon)
          end, results))
        end
      else
        coroutine.yield(nil)
      end
    end
  end
end

snap.register.map(
  'n',
  '<Leader>s',
  snap.create(function()
    return {
      prompt = 'Files',
      producer = icons_consumer(snap.get 'consumer.fzy'(snap.get 'producer.git.file')),
      select = snap.get('select.file').select,
      multiselect = snap.get('select.file').multiselect,
      views = { snap.get 'preview.file' },
    }
  end)
)

We could be more efficient by applying the consumer to the initial producer, since fzy uses the cache consumer. However, this requires a bit of fiddling to ensure that fzy.filter only gets passed strings, given that icons_consumer returns meta-results. Doable (I implemented it), but not clearly worth the added complexity since the current more naive approach is fast enough.

@camspiers, any chance you have time to take a look at the highlighting issue at some point? That's the last blocking problem for this PR - I know now where the highlight positions are calculated, but I'm not sure where the best place to update them for the displayed string is (I also don't know where the highlights get applied).

@camspiers
Copy link
Owner

I think will have some time tonight or tomorrow night to have a look, thanks so much for sticking with this, and apologies for not getting back to it as soon as I hoped.

@wbthomason
Copy link
Author

Not a problem! I absolutely understand not getting to open source things as quickly as one hopes to... (see: this PR, several PRs and issues on the packer repo, etc.)

@wbthomason
Copy link
Author

@camspiers I might have some time to work on the last remaining issue for this PR (highlight offsets) in the next few days - any chance you could point me toward what I need to modify to shift the start/end of the match highlight ranges?

@wbthomason
Copy link
Author

Actually, nevermind - I figured it out!
Here's the final icons consumer:

local snap = require 'snap'
local icons = require 'nvim-web-devicons'
require('nvim-web-devicons').setup { default = true }

local fnamemodify = vim.fn.fnamemodify
local get_icon = icons.get_icon
local function add_icon(meta_result)
  local filename = fnamemodify(meta_result.result, ':t:r')
  local extension = fnamemodify(meta_result.result, ':e')
  local icon = get_icon(filename, extension)
  return icon .. ' ' .. meta_result.result
end

local function icons_consumer(producer)
  return function(request)
    for results in snap.consume(producer, request) do
      if type(results) == 'table' then
        if not vim.tbl_islist(results) then
          coroutine.yield(results)
        else
          coroutine.yield(vim.tbl_map(function(result)
            return snap.with_metas(result, { display = add_icon, highlight_offset = 4 })
          end, results))
        end
      else
        coroutine.yield(nil)
      end
    end
  end
end

I think this PR is ready except for (1) docs and (2) deciding if we want to ship the icons consumer by default or just provide it as an example.

@wbthomason wbthomason marked this pull request as ready for review July 22, 2021 18:10
@wbthomason
Copy link
Author

Hmm, one thing I just found: if this icons consumer is applied to something with a lot of results (e.g. 10000 rg results), we get an OOM error. So it might be worth looking into a way to write the consumer that doesn't need to allocate quite so much.

@wbthomason
Copy link
Author

wbthomason commented Jul 28, 2021

@camspiers Note that, despite that last comment, I think this PR is good to review (as you have time).

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.

Request: a mechanism for adding decorations to results
3 participants