Skip to content

feat: add renderer.icons.diagnostic_placement #2152

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

Closed
wants to merge 8 commits into from

Conversation

baahrens
Copy link
Collaborator

@baahrens baahrens commented Apr 20, 2023

Attempts to close #1510

This is my first proper PR in this project, so please let me know if this is the wrong approach/direction or if I broke any styling guidelines.

There's a few things I'm unsure about:

  1. Is it okay to reload the whole explorer on DiagnosticChanged or should we aim for a more finegrained approach?
  2. Should this be config.renderer.icons.diagnostic_placement or config.diagnostics.placement (this was proposed in Add nvim-tree.diagnostics.placement config option #1510)?
  3. There was a lot of logging in the original diagnostics.update function. Should this be replicated somewhere in the new implementation? If yes, where?

TODO:

  • Add support for folder diagnostics
  • documentation/help

Similar to the other icons there's now an option `diagnostic_placement`
that can either be "signcolumn" (default), "before" or "after".
@baahrens baahrens requested a review from alex-courtis April 20, 2023 10:31
@alex-courtis
Copy link
Member

alex-courtis commented Apr 23, 2023

  1. Is it okay to reload the whole explorer on DiagnosticChanged or should we aim for a more finegrained approach?

Unfortunately we cannot afford that expensive operation.

Diagnostic changes must be updated in the debounced "thread" M.update, affecting only diagnostics.

Edit: to be clear: we can draw, just not reload the tree.

@alex-courtis
Copy link
Member

2. Should this be config.renderer.icons.diagnostic_placement or config.diagnostics.placement

config.diagnostics.placement is preferable, however inconsistent with modified/git.

config.renderer.icons.diagnostic_placement please.

@alex-courtis
Copy link
Member

There was a lot of logging in the original diagnostics.update function. Should this be replicated somewhere in the new implementation? If yes, where?

Please retain the logging and profiling, in the M.update debounced "thread".

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 looking great - thanks for your work.

@baahrens
Copy link
Collaborator Author

Thanks for your feedback!

I adjusted a few things:

  • The tree is now redrawing instead of reloaded on DiagnosticChanged
  • Diagnostic are now shown on folders, respecting diagnostics.show_on_dirs and diagnostics.show_on_open_dirs (can you check, if this is working correctly? I'm not quite sure)
  • Added the logging again, although I'm not sure it makes sense like this, as the update function from before doesn't exist anymore, as diagnostic rendering is now part of the drawing process and doesn't happen in a debounced "thread"

Let me know if this is going in the right direction, I'll update the help file then.

renderer.icons.diagnostic_placement == "after"
Screenshot 2023-04-24 at 22 38 11
renderer.icons.diagnostic_placement == "before"
Screenshot 2023-04-24 at 22 38 59

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.

Looking good except for sign column folders.

Test case nvim-tree/dev/lua/nvim-tree/actions/finders/find-file.lua

  view = {
    width = {
      min = 20,
      max = 50,
    },
  },

OK:
show_on_dirs = false, show_on_open_dirs = false,
diagnostic_placement = "before",
diagnostic_placement = "signcolumn",
diagnostic_placement = "after",
diagnostic_placement = nil,

show_on_dirs = true, show_on_open_dirs = false,
diagnostic_placement = "before",
diagnostic_placement = "signcolumn",
diagnostic_placement = "after",
diagnostic_placement = nil,

show_on_dirs = false, show_on_open_dirs = true,
diagnostic_placement = "before",
diagnostic_placement = "signcolumn",
diagnostic_placement = "after",
diagnostic_placement = nil,

show_on_dirs = true, show_on_open_dirs = true,
diagnostic_placement = "before",
diagnostic_placement = "after",

FAIL:
show_on_dirs = true, show_on_open_dirs = true,
diagnostic_placement = "signcolumn",
diagnostic_placement = nil,

Same results with group_empty = true, in an empty directory tree.

@alex-courtis
Copy link
Member

  • Added the logging again, although I'm not sure it makes sense like this, as the update function from before doesn't exist anymore, as diagnostic rendering is now part of the drawing process and doesn't happen in a debounced "thread"

Please wrap the profile / logging around the "DiagnosticChanged" and "CocDiagnosticChange" autocommands. We want to capture the time spent fetching diagnostics.

@@ -364,13 +406,44 @@ end

function Builder:build(tree, unloaded_bufnr)
local num_children = self:_get_nodes_number(tree.nodes)
local diagnostic_severities_by_path = diagnostics.get_diagnostics()
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't fit in here; it should go back in diagnostics.lua.

I like _get_diagnostic_icon - nice and consistent.

Proposal:

  • Move this back into diagnostics.update
  • diagnostics.update records the diagnostic state of the node. Something like git_status
  • call draw following diagnostics.update
  • _get_diagnostic_icon calls something like local diagnostic_icon = diagnostics.get_icon(node)

@alex-courtis alex-courtis changed the title feat: Add diagnostic placement feat: add renderer.icons.diagnostic_placement May 14, 2023
@alex-courtis
Copy link
Member

Any updates on this one @baahrens ? This would be a great addition... I will be using it...

@baahrens
Copy link
Collaborator Author

baahrens commented Jun 8, 2023

I will look into this shortly, sorry for the delay

@alex-courtis
Copy link
Member

I will look into this shortly, sorry for the delay

There is no hurry; just checking whether you still intend to work on this one.

@alex-courtis
Copy link
Member

superseded by #2396

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.

Add nvim-tree.diagnostics.placement config option
2 participants