Skip to content

feat: use vim.notify for user notifications #507

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

Merged
merged 1 commit into from
May 29, 2024

Conversation

emmanueltouzery
Copy link
Contributor

fixes #473
vim.notify allows the user to customize the display of messages

I'm not sure about the schedule parameter, whether it's still needed. I left it in just in case.

The testing I performed is that I triggered the:

"The file is open with unsaved changes! Aborting file restoration."

message, by modifying a file, saving, modifying again, then asking to revert the changes in diffview, that triggered that message. I also tried modifying the location where the message is triggered to ask to trigger it with the schedule parameter being "true" and that worked too.

Copy link
Owner

@sindrets sindrets left a comment

Choose a reason for hiding this comment

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

Just remove that one early return, and this looks good!

And, yes, let's keep the schedule parameter. It's useful for two reasons:

  1. When these are called from an async context. Without scheduling the notifications are likely to fail when they try to access the nvim API.
  2. In performance critical code it's useful for deferring the handling of the notification until later.

@emmanueltouzery
Copy link
Contributor Author

I force pushed the removal the nil-check now. I had it in because the previous code also had it (well, it had if not msg[1]). Thank you for the explanation for schedule!

fixes sindrets#473
vim.notify allows the user to customize the display of messages
@emmanueltouzery
Copy link
Contributor Author

my bad about the type signature 😬
force pushed the fix now.

Copy link
Owner

@sindrets sindrets left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@sindrets sindrets changed the title [feat] use vim.notify for user notifications feat: use vim.notify for user notifications May 29, 2024
@sindrets sindrets merged commit 3afa6a0 into sindrets:main May 29, 2024
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.

[feat][would write PR] use vim.notify for user notifications
2 participants