feat: Add ability to customize notification handler

[ghostbuster91]

add notify.handler to setup opts

refers to #1697

[alex-courtis]

Tested ok both commits:

• default handler
• rcarriga/nvim-notify
• notify.handler :

local function h(msg, level, opts)
	print(vim.inspect(msg))
	print(vim.inspect(level))
	print(vim.inspect(opts))
end

Please:

• Update help
• Remove camel case and rename
• Revert DEFAULT_OPTS pending @gegoune

[alex-courtis]

How about:

    *nvim-tree.notify.handler*
    Change the default notification handler, can be a string `"default"` or a function.
    The default handler will check if the nvim-notify plugin is present on the
    runtime path and use it, otherwise it will fallback to use |vim.notify()|
      Type: `string` | `function`, Default: `"default"`

    Parameters: ~
      • {msg}   (string) See |vim.notify()|
      • {level} (number) See |vim.notify()|
      • {opts}  (table)  Optional parameters

    Options: ~
      • {title} (string) Usually `"NvimTree"`

[gegoune]

Why are we checking for nvim-notify at all? Wasn't idea behind vim.notify such that it handles notifications send to it per users' handler, if it happens to be nvim-notify so be it.

Not near my computer so can't check much easily.

[alex-courtis]

Oh I see! We set handler directly to vim.notify.

Much cleaner. Just have to ensure that the arguments line up.

[ghostbuster91]

Why are we checking for nvim-notify at all? Wasn't idea behind vim.notify such that it handles notifications send to it per users' handler, if it happens to be nvim-notify so be it.

Yes, that's correct. I did it like that only to preserve the previous behavior, but I would happy to simplify it. Is there any issue/way of notifying users of such change?

Much cleaner. Just have to ensure that the arguments line up.

They do :)

[alex-courtis]

Working:
handler = require("notify").notify,
and
handler = vim.notify,

the latter case does not prepend the title, however that's expected.

[alex-courtis]

Yes, that's correct. I did it like that only to preserve the previous behavior, but I would happy to simplify it. Is there any issue/way of notifying users of such change?

If we were building this today, the user would need to specify handler = require("notify").notify however the existing auto-detection behaviour exists and I don't like to break behaviour.

How about:

• notify.handler is nil or "default" AND
• require("notify") successful

"nvim-notify plugin detected: please set notify.handler = require("notify").notify. See :help nvim-tree.notify"

Options:

1. Preserve existing behaviour, defaulting to nvim-notify
2. Remove nvim-notify fallback, requiring the user to explicitly set notify.handler

Your call @gegoune :)

[ghostbuster91]

• Remove camel case and rename
• Revert DEFAULT_OPTS
• Update help

Waiting for the confirmation regarding auto detection of nvim-notify

[alex-courtis]

Yes, that's correct. I did it like that only to preserve the previous behavior, but I would happy to simplify it. Is there any issue/way of notifying users of such change?

If we were building this today, the user would need to specify handler = require("notify").notify however the existing auto-detection behaviour exists and I don't like to break behaviour.

How about:

• notify.handler is nil or "default" AND
• require("notify") successful

"nvim-notify plugin detected: please set notify.handler = require("notify").notify. See :help nvim-tree.notify"

Options:

1. Preserve existing behaviour, defaulting to nvim-notify
2. Remove nvim-notify fallback, requiring the user to explicitly set notify.handler

Your call @gegoune :)

[gegoune]

I am all in for breaking changes if justified, they are not as disruptive as people might think if done properly.

Why are we even checking for nvim-notify? So we can call it with some other parameters/if it't not present to prefix message ourselves? If messages sent change then it barely is a breaking change at all. Am I missing on how that is a breaking change?

I am all in to setting default handler to vim.notify with an option to change it to custom function.

(Is vim.notify sender aware? If so users that have a need to change handler per plugin could do it in their handler, we wouldn't need to accommodate it at all. :)

[ghostbuster91]

Why are we even checking for nvim-notify? So we can call it with some other parameters/if it't not present to prefix message ourselves? If messages sent change then it barely is a breaking change at all. Am I missing on how that is a breaking change?

The breaking change I had in mind was regarding the auto-detection of nvim-notify.
I can imagine that some people rely on that feature but for some reason they don't have the nvim-notify attached to vim.notify directly. I know that it is probably a very rare case but still. I am fine with whatever decision you make, I just wanted to highlight that.

(Personally I think that the auto-detection feature should be removed)

[gegoune]

I agree with the removal. If someone is doing something like that we can very easily assume they know what they are doing and will be able to adapt.

[alex-courtis]

How about:

• remove it
• put a breaking change note in the readme
• add a note in the :help about handler = require("notify").notify

[gegoune]

add a note in the :help about handler = require("notify").notify

I am not sure about that bit. We should just send stuff to vim.notify, which is default mechanism de facto now. Purpose of this PR was to enable users to come up with their own handlers. I will need to review it again, why is it required exactly, as once we just use vim.notify users can override that handler in Neovim to their liking.

[ghostbuster91]

I will need to review it again, why is it required exactly, as once we just use vim.notify users can override that handler in Neovim to their liking.

Very much indeed! Because of the small changes in the direction that we did during that PR it was not obvious to spot it, but I think that you are right. Now, once the auto-detection of nvim-notify was removed, we can just simply sent everything to vim.notify and it is totally up to the user to configure it to their liking.

[gegoune]

Ok, good to know I wasn't imagining things. With that change we wouldn't need a custom handler any more.

Would you like to amend that, or create new, PR with those changes?

[ghostbuster91]

I will create a new PR to keep the things clear and refer to this one.