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

Issue with changing notification position - bug? #124

Closed
levouh opened this issue Aug 19, 2022 · 15 comments
Closed

Issue with changing notification position - bug? #124

levouh opened this issue Aug 19, 2022 · 15 comments

Comments

@levouh
Copy link

levouh commented Aug 19, 2022

Having trouble uploading gifs at the moment to demonstrate, but will update later when they are working


Taking what is in fade_in_slide_out.lua and changing the col value from vim.opt.columns:get() to just 1 moves the notifications from the top right to the top left as I would expect.

However, when changing the stages_util.DIRECTION.TOP_DOWN to stages_util.DIRECTION.BOTTOM_UP in an attempt to then move them to the bottom left results in issues where a secondary notification overlaps the first one vertically by ~2 rows rather than having them stay disjointed. Because this is an issue with the row (and I the initial position of the 2+ notification is correct), I believe this is an issue with slot_after_previous (which is used in [lua] index 2+ of the stages configuration).

Here is what I have after swapping things around:

local notify = { col_offset = 1 }

local function get_stages()
  local stages_util = require("notify.stages.util")

  return {
    function(state)
      local next_row = stages_util.available_slot(
        state.open_windows,
        state.message.height + 2,
        stages_util.DIRECTION.TOP_DOWN
      )

      if not next_row then
        return nil
      end

      return {
        relative = "editor",
        anchor = "NE",
        width = state.message.width,
        height = state.message.height,
        col = notify.col_offset,
        row = next_row,
        border = "rounded",
        style = "minimal",
        opacity = 0,
      }
    end,
    function(state, win)
      return {
        opacity = { 100 },
        col = { notify.col_offset },
        row = {
          stages_util.slot_after_previous(win, state.open_windows, stages_util.DIRECTION.TOP_DOWN),
          frequency = 3,
          complete = function()
            return true
          end,
        },
      }
    end,
    function(state, win)
      return {
        col = { notify.col_offset },
        time = true,
        row = {
          stages_util.slot_after_previous(win, state.open_windows, stages_util.DIRECTION.TOP_DOWN),
          frequency = 3,
          complete = function()
            return true
          end,
        },
      }
    end,
    function(state, win)
      return {
        width = {
          1,
          frequency = 2.5,
          damping = 0.9,
          complete = function(cur_width)
            return cur_width < 3
          end,
        },
        opacity = {
          0,
          frequency = 2,
          complete = function(cur_opacity)
            return cur_opacity <= 4
          end,
        },
        col = { notify.col_offset },
        row = {
          stages_util.slot_after_previous(win, state.open_windows, stages_util.DIRECTION.TOP_DOWN),
          frequency = 3,
          complete = function()
            return true
          end,
        },
      }
    end,
  }
end
@rcarriga
Copy link
Owner

It seems to be working fine for me, are you sure you didn't miss changing one of the direction arguments to BOTTOM_UP? Can you provide the config that isn't working for bottom up?

@levouh
Copy link
Author

levouh commented Aug 21, 2022

This is what I am currently using (notify.col_offset = 1):

  return {
    function(state)
      local next_row = stages_util.available_slot(
        state.open_windows,
        state.message.height + 2,
        stages_util.DIRECTION.BOTTOM_UP
      )

      if not next_row then
        return nil
      end

      return {
        relative = "editor",
        anchor = "NE",
        width = state.message.width,
        height = state.message.height,
        col = notify.col_offset,
        row = next_row,
        border = "rounded",
        style = "minimal",
        opacity = 0,
      }
    end,
    function(state, win)
      return {
        opacity = { 100 },
        col = { notify.col_offset },
        row = {
          stages_util.slot_after_previous(win, state.open_windows, stages_util.DIRECTION.BOTTOM_UP),
          frequency = 3,
          complete = function()
            return true
          end,
        },
      }
    end,
    function(state, win)
      return {
        col = { notify.col_offset },
        time = true,
        row = {
          stages_util.slot_after_previous(win, state.open_windows, stages_util.DIRECTION.BOTTOM_UP),
          frequency = 3,
          complete = function()
            return true
          end,
        },
      }
    end,
    function(state, win)
      return {
        width = {
          1,
          frequency = 2.5,
          damping = 0.9,
          complete = function(cur_width)
            return cur_width < 3
          end,
        },
        opacity = {
          0,
          frequency = 2,
          complete = function(cur_opacity)
            return cur_opacity <= 4
          end,
        },
        col = { notify.col_offset },
        row = {
          stages_util.slot_after_previous(win, state.open_windows, stages_util.DIRECTION.BOTTOM_UP),
          frequency = 3,
          complete = function()
            return true
          end,
        },
      }
    end,
  }

is what I'm using. I still cannot upload pictures or GIFs for some reason, otherwise I would show you what it looks like @rcarriga.

@rcarriga
Copy link
Owner

Still can't reproduce unfortunately. I'm thinking there's some setting you have enabled/disabled that the previous slot function doesn't account for.

Can you try with this init.lua

nvim --clean -u init.lua

-- ignore default config and plugins
vim.opt.runtimepath:remove(vim.fn.stdpath("config"))
vim.opt.packpath:remove(vim.fn.stdpath("data") .. "/site")
vim.opt.termguicolors = true

-- append test directory
local test_dir = "/tmp/nvim-config"
vim.opt.runtimepath:append(vim.fn.expand(test_dir))
vim.opt.packpath:append(vim.fn.expand(test_dir))

-- install packer
local install_path = test_dir .. "/pack/packer/start/packer.nvim"
local install_plugins = false

if vim.fn.empty(vim.fn.glob(install_path)) > 0 then
  vim.cmd("!git clone https://github.com/wbthomason/packer.nvim " .. install_path)
  vim.cmd("packadd packer.nvim")
  install_plugins = true
end

local packer = require("packer")

packer.init({
  package_root = test_dir .. "/pack",
  compile_path = test_dir .. "/plugin/packer_compiled.lua",
})

packer.startup(function(use)
  use("wbthomason/packer.nvim")

  use({
    "rcarriga/nvim-notify",
    config = function()
      local stages_util = require("notify.stages.util")
      require("notify").setup({
        background_colour = "#000000",
        stages = {
          function(state)
            local next_row = stages_util.available_slot(
              state.open_windows,
              state.message.height + 2,
              stages_util.DIRECTION.BOTTOM_UP
            )

            if not next_row then
              return nil
            end

            return {
              relative = "editor",
              anchor = "NE",
              width = state.message.width,
              height = state.message.height,
              col = 1,
              row = next_row,
              border = "rounded",
              style = "minimal",
              opacity = 0,
            }
          end,
          function(state, win)
            return {
              opacity = { 100 },
              col = { 1 },
              row = {
                stages_util.slot_after_previous(
                  win,
                  state.open_windows,
                  stages_util.DIRECTION.BOTTOM_UP
                ),
                frequency = 3,
                complete = function()
                  return true
                end,
              },
            }
          end,
          function(state, win)
            return {
              col = { 1 },
              time = true,
              row = {
                stages_util.slot_after_previous(
                  win,
                  state.open_windows,
                  stages_util.DIRECTION.BOTTOM_UP
                ),
                frequency = 3,
                complete = function()
                  return true
                end,
              },
            }
          end,
          function(state, win)
            return {
              width = {
                1,
                frequency = 2.5,
                damping = 0.9,
                complete = function(cur_width)
                  return cur_width < 3
                end,
              },
              opacity = {
                0,
                frequency = 2,
                complete = function(cur_opacity)
                  return cur_opacity <= 4
                end,
              },
              col = { 1 },
              row = {
                stages_util.slot_after_previous(
                  win,
                  state.open_windows,
                  stages_util.DIRECTION.BOTTOM_UP
                ),
                frequency = 3,
                complete = function()
                  return true
                end,
              },
            }
          end,
        },
      })
      vim.notify = require("notify")
    end,
  })

  if install_plugins then
    packer.sync()
  end
end)

@levouh
Copy link
Author

levouh commented Aug 22, 2022

Good point - I should have started with this as well. The above minimal configuration does not see the same issues, so I will start to bisect mine to see where things are going wrong and report back.

@levouh
Copy link
Author

levouh commented Aug 23, 2022

@rcarriga I was able to reproduce it with the init.lua file you provided above, and executing :luafile % on the following in a buffer:

vim.schedule(function()
  vim.notify("foo")

  vim.defer_fn(function()
    vim.notify({"bar", "baz"})

  end, 1500)
end)

Also media works now so here's what it looks like from my side:

bottom_left-2022-08-23_08.42.37.mp4

@levouh
Copy link
Author

levouh commented Aug 24, 2022

Not sure if it makes a difference @rcarriga, but when using (the first vim.notify is called immediately, rather than scheduled for later):

vim.notify("foo")

vim.defer_fn(function()
  vim.notify({"bar", "baz"})
end, 1500)

note that the top-most notification sliding down over the bottom one no longer happens:

bottom_left_no_overlap-2022-08-24_08.56.43.mp4

however the top-most notification doesn't move itself all the way down to the bottom either.

@levouh
Copy link
Author

levouh commented Aug 24, 2022

Spamming a bit with debugging notes - apologies in advance.


The print statements below print out next_row for the first stages function, which shows us where the window starts. The subsequent prints are for the second stages function with all other stages functions removed.

For the first window (1007):

image

and for the second window (1008) with some co-mingling of the first window as well:

image

it starts in the right place with the return value from available_slot, but then the return value from slot_after_previous ends up pushing it down by one. Later when 1007 is gone and 1008 slides down, we can also see that it is off by one (too high):

image

Will keep looking as time permits and post here if I find any updates, but hopefully this helps narrow in on what the problem actually is.

rcarriga added a commit that referenced this issue Aug 27, 2022
@rcarriga
Copy link
Owner

I was able to reproduce the issue with the second notification not going to the bottom and have fixed it, pull the latest master to confirm. Unfortunately, I'm not seeing that issue with the second appearing over the first though

@levouh
Copy link
Author

levouh commented Aug 29, 2022

Interesting that it does not reproduce for you. When you are testing, what are the values of vim.opt.columns:get() and vim.opt.lines:get()? I wonder if it has something to do with the editor size where I am running it versus where you are @rcarriga.

Also note the example the reproduces the issue has a top level vim.schedule wrapper. I cannot imagine how this makes a difference beyond timing of things though.

The issue with notifications not going back down all the way is fixed per your last commit, however the overlapping issue still exists.

rcarriga added a commit that referenced this issue Sep 6, 2022
@rcarriga
Copy link
Owner

rcarriga commented Sep 6, 2022

OK figured out the issue. Please try out master now 😄

@levouh
Copy link
Author

levouh commented Sep 6, 2022

Things are flip-flopped now. The issue with notifications overlapping is fixed, however now the second notification doesn't go all the way to the bottom of the screen.

Appreciate the effort either way though @rcarriga. Let me know if you want me to try to setup a minimal config to reproduce the "not going all the way to the bottom" issue.

rcarriga added a commit that referenced this issue Sep 6, 2022
Setting cmdheight to 0 and then entering a command would change the
cmdheight to 1 temporarily. This small change meant that the bounce back
was extremely slow and so never reached back to originial position.
Rounding fixes this

See #124
@rcarriga
Copy link
Owner

rcarriga commented Sep 6, 2022

I found an issue with cmdheight=0, did you have that set? Could you try lastest master?

@levouh
Copy link
Author

levouh commented Sep 6, 2022

I have cmdheight=1 - will try the latest master.

@levouh
Copy link
Author

levouh commented Sep 6, 2022

@rcarriga as far as I can tell it works:

tint-2022-09-06_14.37.00.mp4

There's a bit of jitter at the end there, but it goes down to the correct spot for sure. Thanks for all the work on this!

rcarriga added a commit that referenced this issue Sep 6, 2022
Should reduce lag between .40 and .50

See #124
@rcarriga
Copy link
Owner

rcarriga commented Sep 6, 2022

Brilliant 😄 I've updated the rounding to help a little with the jitter slightly. You can also tweak the damping/frequency options to change the spring physics but unfortunately rendering in a terminal has its limitations

@levouh levouh closed this as completed Sep 6, 2022
This issue was closed.
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

No branches or pull requests

2 participants