Skip to content
This repository has been archived by the owner on May 16, 2024. It is now read-only.

sections: only display after section start #42

Merged
merged 5 commits into from
Sep 13, 2023

Conversation

emmanueltouzery
Copy link
Contributor

fixes #28
It was wrong to give all the contents to the preview/display command and then search on the output, because the command could change the format completely.
So now we submit to the converter program only data after the section start, and we don't need to search the output anymore.

@emmanueltouzery
Copy link
Contributor Author

ideally we'd also drop the content after the next section start. That would make the output shorter, and consistent (only show for the current section), and it would also speed up display for glow massively. But I went for the quickfix for now.

fixes luckasRanarison#28
It was wrong to give all the contents to the preview/display command and
then search on the output, because the command could change the format
completely.
So now we submit to the converter program only data after the section
start, and we don't need to search the output anymore.
@luckasRanarison
Copy link
Owner

luckasRanarison commented Sep 10, 2023

This is a really interesting idea, making processing before rendering was a nice thought. But there's an issue with it, docs sections are not necessarily headers, it can also be a link, a word in the inside another section, etc... and in that case the docs would get cut without having the possibility to scroll up. It's a nice workaround for the case where the section is an header though, so we should add a check and use search as a fallback.

@emmanueltouzery
Copy link
Contributor Author

interesting... but how would I know whether it's a header or not?

I printed the contents of selection in open_doc, and I get:

{
  display = "[lodash-4] _.keys",
  index = 1491,
  ordinal = "[lodash-4] _.keys",
  value = {
    alias = "lodash-4",
    link = "index#keys",
    name = "[lodash-4] _.keys",
    path = "1,### _.keys(object)[source](https://github.com/lodash/lodash/blob/4.17.10/lodash.js#L13305) [npm package](https://www.npmjs.com/package/lodash.keys)"
  }
}

I can't say here anything that tells me whether it's a header or not.

I have played a little also with converting the header text with glow to search for the converted header text. It would possibly be workable, but it's very messy, because glow emits shell escape codes for coloring. These would have to be stripped for searching. I in fact have some code to do that for some other project, but it just seems very frail...

@emmanueltouzery
Copy link
Contributor Author

i think i have a better idea, hold on...

@emmanueltouzery
Copy link
Contributor Author

I now opened an alternative PR, #45

Keeping this one open in case you still prefer the approach from this PR, after adaptations.

@luckasRanarison
Copy link
Owner

luckasRanarison commented Sep 11, 2023

interesting... but how would I know whether it's a header or not?

{
  ...
  value = {
    ...
    path = "1,### _.keys(object)[source](https://github.com/lodash/lodash/blob/4.17.10/lodash.js#L13305) [npm package](https://www.npmjs.com/package/lodash.keys)"
  }
}

By header I meant markdown headers, starting with #. I think I prefer this PR approach, it could also be extended to omit next sections. I honestly prefer making preprocessing like this instead of messing more with the transpiler. I gave up the idea of splitting the docs further because of the previous facts: sections not necessarily being headers and the nested sections. I don't want to add more complexity there just to handle some edge cases.
I'll work on this tomorrow...

@emmanueltouzery
Copy link
Contributor Author

I understand. None of the two PRs touch the transpiler, yes. For now I won't do anything because you wrote that you'll work on that. If I can help, let me know! And thank you for the plugin!

@luckasRanarison
Copy link
Owner

I'm also really thankful for all your help, I've learned so much with you :)

Simplified the implementation and implemented the next section drop
@luckasRanarison
Copy link
Owner

It's really promising, still a wip but the idea is there. Playing with lines is easier than playing with the nodes for searching sections, and the processing is not expensive. This is really the way to go. I'll change the previewer implementation and start filtering there, this would finally fix all major issues!!! Thanks for the idea :)

@luckasRanarison
Copy link
Owner

luckasRanarison commented Sep 12, 2023

Also forget about sections not being headers, I noticed this with bash docs but it still works well and the docs are still concise enough even if the section start is left off.

@luckasRanarison
Copy link
Owner

The only remaining problem is piping command with the telescope previewer. Before I just used the filepath with the cmd (glow) but now we have to read the file first and pipe the filtered lines to the cmd and I don't know how.

local term_doc_previewer = previewers.new_termopen_previewer({
  title = "Preview",
  get_command = function(entry)
    local splited_path = vim.split(entry.value.path, ",")
    local file = splited_path[1]
    local file_path = path:new(plugin_config.dir_path, "docs", entry.value.alias, file .. ".md")
    local lines = file_path:readlines()
    local filtered_lines = operations.filter_doc(lines, splited_path[2])
    local args = {} -- TODO

    return args
  end,
})

@emmanueltouzery
Copy link
Contributor Author

about the termopen issue, I'm optimistic. See
https://github.com/nvim-telescope/telescope.nvim/blob/master/lua/telescope/previewers/term_previewer.lua#L217

Telescope has a lot of options that you can see only in the code, not in the docs. Here we can see the send_input option. I think you can use it to send data to glow via stdin instead of giving a file name.

@emmanueltouzery
Copy link
Contributor Author

more about the telescope previewer options, including send_input:
https://github.com/nvim-telescope/telescope.nvim/blob/master/lua/telescope/previewers/init.lua#L63

@emmanueltouzery
Copy link
Contributor Author

@emmanueltouzery
Copy link
Contributor Author

i think you'd call self.send_input in the preview callback.

@luckasRanarison
Copy link
Owner

luckasRanarison commented Sep 13, 2023

i think you'd call self.send_input in the preview callback

It'd be too late, the string should be passed before the command, the simplest way to do this would be echo "# Hello world" | glow -p but we can't pipe commands. I tried another approach without using termopen previewer but a normal buffer previewer and then output a job result like the in the normal preview buffer, but the lines get wrapped and the output is really messy.

@emmanueltouzery
Copy link
Contributor Author

I played with it a little, but didn't get very far for now. Maybe we should ask the telescope developers, I think they have a chat.

The approach I suggested yesterday is more the telescope low-level API, not higher-level like termopen or buffer previewers.

Piping would be possible also with termopen but it gets very ugly:
https://github.com/debugloop/telescope-undo.nvim/blob/3dec002ea3e7952071d26fbb5d01e2038a58a554/lua/telescope-undo/previewer.lua#L27

Another option possibly could be to create a temporary file, pass it to glow, overwrite it every time, then deleting it in the teardown callback maybe? Although teardown is available to the buffer previewer but not to the termopen previewer.

Maybe you can make a branch with the buffer previewer attempt. Possibly we could check what does the termopen previewer does differently from the buffer previewer, maybe it's possible to fix the wrapping...

@emmanueltouzery
Copy link
Contributor Author

emmanueltouzery commented Sep 13, 2023

Maybe you can make a branch with the buffer previewer attempt

i meant so that maybe i can try to play with it, if you put it in a branch. Although I can't guarantee i'll have time to work on it very soon.

@emmanueltouzery
Copy link
Contributor Author

emmanueltouzery commented Sep 13, 2023

Otherwise if this gets really messy I'd possibly reconsider generating "cut files" at fetching time, after the transpiling. It would be a second pass, not touching the existing transpiling code.

@luckasRanarison
Copy link
Owner

luckasRanarison commented Sep 13, 2023

Maybe you can make a branch with the buffer previewer attempt. Possibly we could check what does the termopen previewer does differently from the buffer previewer, maybe it's possible to fix the wrapping...

https://github.com/luckasRanarison/nvim-devdocs/tree/telescope-previewer
In this version I sent the job result to the picker preview buffer instead of using termopen_preview, the same method used for rendering the docs before. And so the pager -p arg is no longer needed. I've never realized it but even before the lines got wrapped but with this version there are extra lines for some reason.

@luckasRanarison
Copy link
Owner

luckasRanarison commented Sep 13, 2023

Otherwise if this gets really messy I'd possibly reconsider generating "cut files" at fetching time, after the transpiling. It would be a second pass, not touching the existing transpiling code.

I really want to avoid this one. Because of nested sections, we'll need to process a single file multiple times, the building process is already slow enough. And I don't even want to imagine the number of resulting files...

@luckasRanarison
Copy link
Owner

Oh wait, I'm dumb. It actually works the extra lines were added because I used the same args for the normal previewer and picker previewer and the -w was set to 97 which caused extra lines!

@luckasRanarison luckasRanarison merged commit 378aef5 into luckasRanarison:master Sep 13, 2023
@luckasRanarison
Copy link
Owner

luckasRanarison commented Sep 13, 2023

We've finally did it!!!

@emmanueltouzery
Copy link
Contributor Author

🎉 🚀 👯

@emmanueltouzery
Copy link
Contributor Author

fantastic, looking forward to testing this tonight and i'm super optimistic that this is it 🎉

@luckasRanarison
Copy link
Owner

🚀 👯 🎉 :)
I forgot to specify the change with the picker cmd args in the commit description lol.

@emmanueltouzery
Copy link
Contributor Author

I checked quickly. it was so fast that at first I thought there's a bug and it doesn't go through glow :)

Great work, thanks!

@luckasRanarison luckasRanarison mentioned this pull request Sep 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jumping to section doesn't work for lodash _.keys
2 participants