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

Use vim.notify #280

Merged
merged 1 commit into from
Aug 27, 2021
Merged

Use vim.notify #280

merged 1 commit into from
Aug 27, 2021

Conversation

Shatur
Copy link
Contributor

@Shatur Shatur commented Aug 23, 2021

I replaced all print occurrences with vim.notify. I added DAP title, so users can display notifications nicer. For example, using nvim-notify users can have nice popups.
Gitsigns also use title.

Closes #111.

Copy link
Owner

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

Thanks for the PR.

I think the second arguments should use vim.log.levels.XY. See the stock implementation of vim.notify:

function vim.notify(msg, log_level, _opts)

  if log_level == vim.log.levels.ERROR then
    vim.api.nvim_err_writeln(msg)
  elseif log_level == vim.log.levels.WARN then
    vim.api.nvim_echo({{msg, 'WarningMsg'}}, true, {})
  else
    vim.api.nvim_echo({{msg}}, true, {})
  end
end

Using a string literal as log level won't be recognized.

I'm also not sure if it is justified to use error. Stuff like "No configuration selected" would be fine as info. Messages like "Restart not supported" could probably go as warning. I think "error" should be reserved for unexpected errors - like the when using the error object from the handler response.


It may also be worth to extract the title = 'DAP' thing into a NOTIFY_OPTS constant or something?

@Shatur
Copy link
Contributor Author

Shatur commented Aug 24, 2021

I think the second arguments should use vim.log.levels.XY. See the stock implementation of vim.notify:

Oh, right, completely missed it. For some reason using string works for nvim-notify. Will fix it.

I'm also not sure if it is justified to use error. Stuff like "No configuration selected" would be fine as info. Messages like "Restart not supported" could probably go as warning. I think "error" should be reserved for unexpected errors - like the when using the error object from the handler response.

Makes sense.

It may also be worth to extract the title = 'DAP' thing into a NOTIFY_OPTS constant or something?

Where should I store it?

@Shatur
Copy link
Contributor Author

Shatur commented Aug 24, 2021

Messages like "Restart not supported" could probably go as warning.

Where this causes the function to stop, I left it as INFO. Usually "warning" means that something is working at least partially. Let me know if I should change it.

@Shatur
Copy link
Contributor Author

Shatur commented Aug 24, 2021

Where should I store it?

@mfussenegger I meant in which script should I store this variable?

@mfussenegger
Copy link
Owner

Where should I store it?

@mfussenegger I meant in which script should I store this variable?

Good question. Given that it is used in multiple places we'd need a constants.lua or something - but not sure if it is warranted for this case alone.

Another option would be to have a util.notify, that wraps vim.notify and always provides default options (in this case - the title)

Let me know what you think. Otherwise it is also fine for me to merge as is.

@Shatur
Copy link
Contributor Author

Shatur commented Aug 26, 2021

Another option would be to have a util.notify, that wraps vim.notify and always provides default options (in this case - the title)

@mfussenegger this one sounds better. Will implement this way.

Another question. Should users be allowed to customize these settings, for example, so they can specify icon field? If yes, then how you would see it?

@mfussenegger
Copy link
Owner

Another question. Should users be allowed to customize these settings, for example, so they can specify icon field? If yes, then how you would see it?

Currently I wouldn't make it possible to customize these. (And if you go for the util.notify approach, there would be the option for users to monkey patch it and set their own options.)

@Shatur
Copy link
Contributor Author

Shatur commented Aug 26, 2021

@mfussenegger done!

Copy link
Owner

@mfussenegger mfussenegger left a comment

Choose a reason for hiding this comment

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

The import in widgets currently fails, otherwise I think this is good to go

lua/dap/ui/widgets.lua Outdated Show resolved Hide resolved
@mfussenegger mfussenegger merged commit 42e1eec into mfussenegger:master Aug 27, 2021
@Shatur Shatur deleted the vim-notify branch August 27, 2021 14:01
mfussenegger added a commit that referenced this pull request Sep 10, 2021
mfussenegger added a commit that referenced this pull request Sep 10, 2021
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.

Use vim.notify to display notifications
2 participants