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

feat(event): add TreeRendered #2324

Merged
merged 4 commits into from
Jul 16, 2023
Merged

Conversation

OmbraDiFenice
Copy link
Contributor

@OmbraDiFenice OmbraDiFenice commented Jul 15, 2023

Hello

I stumbled upon a problem while trying to build myself some script saving/restoring some tree state upon editor start, specifically the cursor position and the folder expanded status.

While the folder expansion seems to work fine, I struggled with resetting the cursor position. The problem was that I was subscribing to the TreeOpen event, but that event only gives access to the empty tree window; the actual content is updated by the renderer. So when trying to set the cursor position from the TreeOpen handler it did nothing because at that point the buffer only has 1 (empty) line.

The way I see it there could be 2 possible solutions to this:

  • events._dispatch_on_tree_open() should be called every time the tree window is opened after any subsequent call to renderer.draw()
  • introduce a new event triggered at the end of the rendering that can be used instead of TreeOpen to perform operations on the rendered buffer

Both have pros and cons. In this PR I'm going for the second one, hopefully the summary below will explain why.

Of course if I missed something stupid feel free to point it out and close this PR 😁 I was a bit unsure if I needed to open an issue first, but once I tried to fix the thing I saw that was simple enough so I just pushed the code changes.

Dispatch TreeOpen on any open + redraw

Pros:

  • no new events to maintain
  • arguably more useful event, since now the buffer content is always available (I was expecting this behavior)

Cons:

  • more than just 1-line code change to do (open function is called in many places)
  • less maintainable in the long run (it's very easy to forget to trigger the event in case you add new open+render calls elsewhere)
  • possible side effects breaking users of the event (it's going to be triggered more often, so any handler will be executed more than once)

Introduce the new TreeRendered event

Pros:

  • only triggered in 1 place (limited changes required to introduce it)
  • it's a specific event linked to a well defined "logical" operation
  • being new, it can't break any existing code

Cons:

  • a new event to maintain
  • associating heavy handlers to such an event could lead to performance issues

@alex-courtis
Copy link
Member

Events were fired as expected in the correct order when exercising a variety of functionality.

local Event = api.events.Event

api.events.subscribe(Event.TreeRendered, function()
  log.line("dev", "TreeRendered")
end)
api.events.subscribe(Event.TreeAttachedPost, function()
  log.line("dev", "TreeAttachedPost")
end)
api.events.subscribe(Event.TreeOpen, function()
  log.line("dev", "TreeOpen")
end)
api.events.subscribe(Event.TreeClose, function()
  log.line("dev", "TreeClose")
end)

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic, thank you for the detailed analysis. This is the best solution.

Please:

  • pass bufnr to the event as per TreeAttachedPost
  • (bonus) pass winid

We could pass bufnr/winid to TreeOpen but that might be a little difficult.

@alex-courtis alex-courtis changed the title add TreeRendered event feat(event): add TreeRendered Jul 16, 2023
@OmbraDiFenice
Copy link
Contributor Author

OmbraDiFenice commented Jul 16, 2023

If I understand well passing the buffer and the window is just a matter of calling the respective utility functions from view. The only thing it's not clear to me is if there is some nuance in case there are multiple windows with the tree buffer in them (I'm relatively new to nvim and I never bothered with such a setup so far).

Anyway I pushed the changes to add bufnr and winnr, see if it's ok. If you want I can squash the commits, rebase and force push again so that you can have a cleaner history :)

@alex-courtis
Copy link
Member

If I understand well passing the buffer and the window is just a matter of calling the respective utility functions from view. The only thing it's not clear to me is if there is some nuance in case there are multiple windows with the tree buffer in them (I'm relatively new to nvim and I never bothered with such a setup so far).

Yes. There is a lot of work to be done there: #2255

This event's buf/win will definitely be correct, and it saves the user from the extra api calls.

Anyway I pushed the changes to add bufnr and winnr, see if it's ok. If you want I can squash the commits, rebase and force push again so that you can have a cleaner history :)

No need, we merge squash.

Copy link
Member

@alex-courtis alex-courtis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks for your contribution!

@alex-courtis alex-courtis merged commit 3b62c6b into nvim-tree:master Jul 16, 2023
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.

2 participants