feat: configurable notification level #1693
Conversation
|
Shouldn't |
I like the use of the notify table as it will allow future additions like #1697 |
| } | ||
|
|
||
| do | ||
| local has_notify, notify_plugin = pcall(require, "notify") |
There was a problem hiding this comment.
#1697 will fit nicely in here.
There was a problem hiding this comment.
threshold was proposed: #1502 (comment)
do you mean we should use threshold instead?
#1697 will fit nicely in here.
yeah, I had actually wanted to make the use of nvim-notify to be optional as well, but couldn't settle on a intuitive name for the option so I skipped it.
maybe something like this
notify = {
---@type function|string
handler = vim.notify,
---@type enum
default_level = vim.log.levels.INFO,
},tho I still think it's better to leave it for a separate PR :)
There was a problem hiding this comment.
Yes, separate. This leave the door open nicely.
add `notify.threshold` to setup opts
kylo252
requested review from
kyazdani42 and
alex-courtis
and removed request for
kyazdani42 and
alex-courtis
|
The level does not apply to legacy / config warnings. Raised #1700 to be completed in a follow up PR. |
There was a problem hiding this comment.
Works beautifully. Handles a variety of bad setup options.
I'm still not happy with the inconsistent pattern for notify.lua. Please git apply 0001-feat-configurable-notification-level-consistent-setu.patch, test and commit.
0001-feat-configurable-notification-level-consistent-setu.patch.txt
Many thanks for taking the initiative and putting in the work: the UX is greatly improved.
| { name = "error", level = vim.log.levels.ERROR }, | ||
| } | ||
|
|
||
| do |
There was a problem hiding this comment.
I would expect to see this block inside the setup function. Is it for the case of notifications before setup?
Please leave a comment explaining.
There was a problem hiding this comment.
yeah, it needs to be done this way in case the module gets called before the setup function.
There was a problem hiding this comment.
As I mentioned in #1693 (comment), I prefer not dealing with that API here since it's a bit more nuanced than what's suggested in https://github.com/nvim-tree/nvim-tree.lua/files/9894883/0001-feat-configurable-notification-level-consistent-setu.patch.txt
Problems with the proposed changes:
- it's not reusing the
config.notifytable (which was created for this specific purpose) - it's hard-coding the notify handler regardless
- users can already override
vim.notifyso there's no point in creating yet another redirection layer on top of it - and it's (highly) unlikely that a user would have two separate wrappers for
vim.notifyat the same time
my idea of an alternative source was something similar to how |vim.lsp.util.get_progress_messages()| works, where a status message can be displayed anywhere.
I'm happy to create a separate PR to track that :)
There was a problem hiding this comment.
That's fair enough. This is an unusual module.
We will be revisiting this shortly anyway.
doc/nvim-tree-lua.txt
Outdated
|
|
||
| *nvim-tree.notify.default_level* | ||
| Specify notification level, uses the values from |vim.log.levels| | ||
| Type: `enum`, Default: `vim.log.levels.INFO` |
There was a problem hiding this comment.
INFO is a good, if verbose, default that doesn't change current behaviour.
Perhaps we can indicate the type of messages that are INFO, so that users are comfortable bumping this to WARN.
Maybe "confirmation messages"? I'll think about it; doesn't block the PR.
| } | ||
|
|
||
| do | ||
| local has_notify, notify_plugin = pcall(require, "notify") |
There was a problem hiding this comment.
Yes, separate. This leave the door open nicely.
add
notify.default_levelto setup optsfixes #1502