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(notify): enable telescope extension of nvim-notify #1306

Closed
wants to merge 1 commit into from

Conversation

moetayuko
Copy link
Contributor

No description provided.

@@ -29,6 +29,10 @@ return {
end)
end
end,
config = function(_, opts)
require("notify").setup(opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this is needed?? Since opts by default will call the plugin's setup function? Can you test without this line 33 to see if it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Essentially I tested that in the first place, and custom opts are not applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excuse my ignorance, but how do you test about custom opts being applied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set my background to be transparent, and nvim-notify throughs a warning every time nvim launches. I have the following piece to suppress it:

  {
    "rcarriga/nvim-notify",
    opts = {
      background_colour = "#000000",
    },
  },

W/o the highlighted lines the warning shows up again.

A more accessible approach might be overriding its default timeout and observing whether the new value gets applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that you are right. I changed the value of timeout, that Lazyvim already changes in its opts and I was observing how long it would stay visible. Without setting it up in the config with the opts provided, the notification remained visible for the same amount always. Only when I uncommented setup(opts) inside the config function did I observe a decrease in the amount of time the notification stayed visible. Strange... I thought that opts should be taking care of this step.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It makes sense as you put it. Thanks for your detailed explanation.

Copy link
Contributor

Choose a reason for hiding this comment

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

@abeldekat,

@jyuan0,

You showed interest in this conversation and I read your awesome answer in this #1316 (comment). If you have the time I would be very interested in your thoughts.

Thanks, I was inspired by this awesome thread 😃.

I don't think I really have anything meaningful to add. The three of you have already thoroughly considered the possible implementations. I really appreciate the thought you have all put into lazy-loading, enabled/disabled plugins, and how to best implement things.

Regarding the proposed implementations: I think it makes sense to wait to see if the proposed lazy.nvim changes get merged upstream. This change:

{
    "telescope.nvim",
    opts = function()
      if Util.has("nvim-notify") then
        require("telescope").load_extension("notify")
      end
    end,
    dependencies = {
      {
        "nvim-notify",
        keys = {
          {
            "<leader>sN",
            function()
              require("telescope").extensions.notify.notify()
            end,
            desc = "Search Notifications",
          },
        },
      },
    },
  }

looks preferable to this:

{
        "<leader>unt",
        function()
          if Util.has("telescope.nvim") then
            require("telescope").extensions.notify.notify()
          else
            Util.on_load("which-key.nvim", function()
              require("which-key").register({
                ["<leader>unt"] = "which_key_ignore",
              })
              vim.keymap.del("n", "<leader>unt")
            end)
          end
        end,
        desc = "Open Notifications (Telescope)",
}

I am wondering if it's necessary to register the key with which-key before deleting it, but that's minor and might not even matter if the proposed lazy.nvim changes get upstreamed.

Again, thanks for this awesome conversation and PR review!

Copy link
Contributor

Choose a reason for hiding this comment

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

I created the PR for lazy.nvim, proposing the changes we discussed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good luck with your PR 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@folke improved the PR.
The changes are merged into lazy.nvim v10.4.0!

Co-authored-by: abeldekat <abel@nomail.com>
@moetayuko
Copy link
Contributor Author

PR updated

@folke folke closed this in 914ca4a May 15, 2024
@folke
Copy link
Collaborator

folke commented May 15, 2024

I just added <leader>snt that opens Noice telescope. It shows a similar view to nvim-notify telescope, but better imho, since it retains the correct message formatting.

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