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

[RDY] Notification provider #13843

Merged
merged 3 commits into from
Feb 8, 2021
Merged

[RDY] Notification provider #13843

merged 3 commits into from
Feb 8, 2021

Conversation

teto
Copy link
Member

@teto teto commented Jan 28, 2021

Address #5562 : notifications in the command line are a pain, especially wit LSP now.
I would like the possibility to use system notifications for instance (or floating windows to mimic system level notifications).

Adds a vim.notify function to send such notifications. By default it sends it as usual but you can override it with for instance

vim.notify = function notify_external(log_level, msg)
	vim.fn.jobstart({"notify-send", msg })
end

I've noticed a mismatch between

log.levels = {
and
#define DEBUG_LOG_LEVEL 0
I suppose we should fix that (either add or remove a TRACE level) and expose it to lua.
Also could support a dict to be forward compatible with options (timeout, icon).

@mjlbach @tjdevries @bfredl

@mjlbach
Copy link
Contributor

mjlbach commented Jan 29, 2021

I'm in favor of removing anything that pipes to :messages

Maybe we could roll (the idea) of my PR handling verbosity with nvim_err into this one?
#13764

runtime/lua/vim/lsp.lua Outdated Show resolved Hide resolved
@teto teto force-pushed the notif_provider branch 2 times, most recently from ebcaf16 to 0380e9d Compare January 30, 2021 11:18
@teto teto marked this pull request as ready for review January 30, 2021 12:58
@teto
Copy link
Member Author

teto commented Jan 30, 2021

@mjlbach I find this small PR quite useful already so I prefer to keep the scope minimal to speed up merging (then we can start leveraging it in LSP). I will comment on your other PR.

I've swapped the log level and message so one doesn't need to specify a log level anymore.

NB: freebsd build errors unrelated

Copy link
Contributor

@mjlbach mjlbach left a comment

Choose a reason for hiding this comment

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

LGTM :)

function vim.notify(msg, log_level, _opts)

if log_level == vim.log.levels.ERROR then
vim.api.nvim_err_writeln(msg)
Copy link
Member

Choose a reason for hiding this comment

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

which reminds me, nvim_echo should take optional 'error' arg :)

@tjdevries
Copy link
Contributor

Should we add nvim_notify as API function that just calls this lua code?

Might nice for remote plugins.

@teto
Copy link
Member Author

teto commented Jan 30, 2021

good idea, I've just added nvim_notify.

src/nvim/api/vim.c Outdated Show resolved Hide resolved
Adds function to notify the user like this:
`:lua vim.notify("hello user")`
embeds log levels vim.log.levels.

you can then reassign vim.notify to for instance
```
function notify_external(msg, log_level, opts)
	vim.fn.jobstart({"notify-send", msg })
end
```
@tjdevries
Copy link
Contributor

Could use a test where we call notify and do screen test in Lua.

Could use a test from api side where you override
vim.notify and then call nvim_notify and assert the new function was called.

This also gives us that the code runs once 😁

After that, I'm good with merging.

@tjdevries
Copy link
Contributor

Oh, maybe we should add a name space or provider argument. So you could multiplex them if you wanted.

@teto teto force-pushed the notif_provider branch 2 times, most recently from 3c31490 to eb3abe4 Compare February 1, 2021 22:20
@teto
Copy link
Member Author

teto commented Feb 1, 2021

I've added a test, was not sure how to test the default behavior. I am fine with adding a namespace (and I think nvim_echo should too, this would allow to filter messages per plugin). but not sure if it deserves its own parameter or can be passed into the dicts.

src/nvim/api/vim.c Outdated Show resolved Hide resolved
@teto teto changed the title Notification provider [RDY] Notification provider Feb 8, 2021
@teto
Copy link
Member Author

teto commented Feb 8, 2021

There was no comment on the namespace argument so I'll merge, We can change this before release concurrently with nvim_echo (or post release via the optional dict).

@teto teto merged commit 0042373 into neovim:master Feb 8, 2021
@teto teto deleted the notif_provider branch February 8, 2021 14:49
Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

Bumping the constant's value for logging levels changes behavior / needs doc adjustment.

#define DEBUG_LOG_LEVEL 1
#define INFO_LOG_LEVEL 2
#define WARN_LOG_LEVEL 3
#define ERROR_LOG_LEVEL 4
Copy link
Contributor

Choose a reason for hiding this comment

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

This changes behavior, where CMAKE_EXTRA_FLAGS += -DMIN_LOG_LEVEL=1 now enables debug messages, whereas it was INFO only before..! (e.g. via

#if MIN_LOG_LEVEL <= INFO_LOG_LEVEL
)

Since it is released in 0.5 already it should probably stay like this, but it seems like documentation (and CI config) needs to be adjusted also (e.g.

BUILD_FLAGS="CMAKE_FLAGS=-DCI_BUILD=ON -DCMAKE_BUILD_TYPE=Debug -DCMAKE_INSTALL_PREFIX:PATH=$HOME/nvim-install -DBUSTED_OUTPUT_TYPE=nvim -DDEPS_PREFIX=$HOME/nvim-deps/usr -DMIN_LOG_LEVEL=3"
).

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.

None yet

5 participants