rcarriga / nvim-notify

A fancy, configurable, notification manager for NeoVim
MIT License
3.09k stars 83 forks source link

feat: handle replacing closed notification #79

Closed camilledejoye closed 2 years ago

camilledejoye commented 2 years ago

Hi again,

There is an issue when sending an id for a closed notification, instead of having an error notification Invalid notification to replace there is the following error:

E5108: Error executing lua ...ker/start/nvim-notify/lua/notify/service/buffer/init.lua:99: Invalid buffer id: 4
stack traceback:
        [C]: in function 'nvim_buf_set_option'
        ...ker/start/nvim-notify/lua/notify/service/buffer/init.lua:99: in function 'render'
        ...ack/packer/start/nvim-notify/lua/notify/service/init.lua:64: in function 'replace'
        ...m/site/pack/packer/start/nvim-notify/lua/notify/init.lua:131: in function 'notify'
        [string ":lua"]:1: in main chunk

And if we try again with the same ID:

E5108: Error executing lua ...m/site/pack/packer/start/nvim-notify/lua/notify/init.lua:132: attempt to index local 'notif_buf' (a nil value)
stack traceback:
        ...m/site/pack/packer/start/nvim-notify/lua/notify/init.lua:132: in function 'notify'
        [string ":lua"]:1: in main chunk

Instead of the original error message and those errors I propose to simply create a new notification if we can't do the replacement.

I think it will make the plugin more easy to use in a generic way. Let's imagine we have a function that notifies about something, this function can be called multiple time. Potentially it can even be called before the previous notification was closed (for instance the user defined a long timeout or whatever). The only way to deal with that is to:

local notification_id
function doSomething()
  notification_id = require('notify').notify('message to replace if possible', vim.log.level.INFO, {
    on_close = function()
      notification_id = nil
    end,
  })
end

Currently I play with the idea of leveraging freedesktop notification's specification to see if it's possible to reliably use vim.notify as a generic port without knowing which adapter is used.
In such a situation it's not possible to use the on_close trick.

rcarriga commented 2 years ago

The error should now be fixed as part of my previous changes.

Instead of the original error message and those errors I propose to simply create a new notification if we can't do the replacement.

The decision to not do that was purposeful. The reason I didn't want it to re-open a notification is that the use case for the replacements is a thread of related notifications. If a user dismisses the thread then they should not see any further messages within it.

For your situation instead you could handle the timing logic yourself and set timeout = false if you want to avoid using on_close

camilledejoye commented 2 years ago

The error should now be fixed as part of my previous changes.

Great, thanks.

The decision to not do that was purposeful. The reason I didn't want it to re-open a notification is that the use case for the replacements is a thread of related notifications. If a user dismisses the thread then they should not see any further messages within it.

So it means that these threads were designed to be controlled by the user, setting timeout = false until the last message arrives, am I correct ? I guess that as long as I can retrieve the current configuration to get the timeout I should be able to handle it on my side with a dedicated timer.

rcarriga commented 2 years ago

No the thread is controlled by whatever is sending the notification, I just mean if a user calls notify.dismiss which remove a message in the thread, it should not show any further messages.

The sender controls the timeout by providing it in the notify options, you don't need to know the current users config. You can see https://github.com/rcarriga/nvim-notify/wiki/Usage-Recipes#progress-updates as an example