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][would write PR] use vim.notify for user notifications #473

Closed
emmanueltouzery opened this issue Mar 11, 2024 · 3 comments · Fixed by #507
Closed

[feat][would write PR] use vim.notify for user notifications #473

emmanueltouzery opened this issue Mar 11, 2024 · 3 comments · Fixed by #507
Assignees
Labels
enhancement New feature or request

Comments

@emmanueltouzery
Copy link
Contributor

I would be interested to write a PR for that, but I'm afraid that you'd reject it, so I would first ask if you'd agree with the approach.

It seems to me that in particular error messages (eg utils.err() in the source) are not that emphasized in the GUI. neovim introduced the vim.notify call that can be overridden by users for custom display. For instance the vim.notify plugin offers such an override:
https://github.com/rcarriga/nvim-notify

I would like to enable the user to decide between the current display (using nvim_echo) or vim.notify, that would allow the user to customize the display.

Possible options...

  1. rewrite utils.info/warn/err to directly call vim.notify, and delete echo_multiln
  2. add a new config option, for instance user_messages_with_notify = false
  3. add new hooks for user messages, for instance user_message_displayed. Although that's a bit redundant, because vim.notify already allows customization but...

If you think such a feature has its place in diffview, feel free to let me know which approach you'd rather see, and I can prepare a PR to that effect!

Thank you for an amazing plugin!

@sindrets
Copy link
Owner

I've been meaning to replace the old echo commands for a while, but never got around to it. Thank you for taking the initiative!

I think your first suggested course of action is the best one: remove echo_multiln() and call vim.notify() from the other notification functions.

vim.notify() accepts an optional table parameter, that is unused by the default handler, but many of the popular UI notification plugins at least respect the title key in this table. So let's set that to "diffview.nvim".

Are you still interested in writing a PR for this?

@sindrets sindrets added the enhancement New feature or request label May 29, 2024
@emmanueltouzery
Copy link
Contributor Author

Yes! Thank you for the feedback.

I'll open a pr soon based on your plan, or ask followup questions here 👍

@sindrets
Copy link
Owner

Great, thank you! I'll go ahead and assign you to the issue.

emmanueltouzery added a commit to emmanueltouzery/diffview.nvim that referenced this issue May 29, 2024
fixes sindrets#473
vim.notify allows the user to customize the display of messages
emmanueltouzery added a commit to emmanueltouzery/diffview.nvim that referenced this issue May 29, 2024
fixes sindrets#473
vim.notify allows the user to customize the display of messages
emmanueltouzery added a commit to emmanueltouzery/diffview.nvim that referenced this issue May 29, 2024
fixes sindrets#473
vim.notify allows the user to customize the display of messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants