-
Notifications
You must be signed in to change notification settings - Fork 59
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: component to show real-time notification with output summary #356
Conversation
Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks neat! Left some comments, but on board with the component overall.
-- Like on_complete_notify but, for long-running commands, also show real-time output summary (based on on_output_summarize). | ||
-- Requires nvim-notify to modify the last notification window when new output arrives instead of creating new notification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's take this and put it in a long_desc
field on the component. That will ensure that it gets added to the documentation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
params = { | ||
max_lines = { | ||
desc = "Number of lines of output to show when detail > 1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove when detail > 1
because this doesn't consider the task list detail setting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
min_duration = { | ||
desc = "Minimum duration in milliseconds after which to display the notification", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a minimum, right? This is just the amount of time to delay before showing the notification. Maybe a name like delay_ms
or show_after_ms
would make more sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to delay_ms
output_on_complete = { | ||
desc = "Show output summary even when the task completed", | ||
type = "boolean", | ||
default = false, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the description, I'm not sure why anyone would ever want this to be false. It sounds like the false behavior will update the notification with the output every time there is a change in the output, except for after the process completes. Wouldn't that just ensure that the final notification is stale and doesn't show the last line(s) of the task after completed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't prevent showing the notification after completion, it just configures if the completion notification to shows the last lines of task output. I.e. (with output_on_complete=false
) during runtime we notify with status (RUNNING) and last output, while on completion we notify with status (e.g. SUCCESS) without output lines.
I changed the description and added long_desc
so it should now be clearer.
---@param bufnr integer | ||
---@param num_lines integer | ||
---@return string[] | ||
local function get_last_lines(bufnr, num_lines) | ||
local end_line = vim.api.nvim_buf_line_count(bufnr) | ||
num_lines = math.min(num_lines, end_line) | ||
local lines = {} | ||
while end_line > 0 and #lines < num_lines do | ||
local need_lines = num_lines - #lines | ||
lines = vim.list_extend( | ||
vim.api.nvim_buf_get_lines(bufnr, math.max(0, end_line - need_lines), end_line, false), | ||
lines | ||
) | ||
while | ||
not vim.tbl_isempty(lines) | ||
and (lines[#lines]:match("^%s*$") or lines[#lines]:match("^%[Process exited")) | ||
do | ||
table.remove(lines) | ||
end | ||
end_line = end_line - need_lines | ||
end | ||
return lines | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a lot of tricky logic. Let's try to find a way to share it instead of copying it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to overseer.util
|
||
---@type overseer.ComponentFileDefinition | ||
local comp = { | ||
desc = "Notify with task output summary for long-running tasks or when completed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole point of this is to leverage nvim-notify; we only support fallback behavior when it is not present. Let's call out nvim-notify
and be clearer about the intended use case for this component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added explicit note about nvim-notify
end, | ||
|
||
on_start = function(self) | ||
self.start_time = vim.uv.now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overseer does still support Neovim 0.8. Let's do a local uv = vim.uv or vim.loop
at the top of this file and use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
update_notification = function(self, task, complete) | ||
-- Don't notify on output without nvim-notify installed, as this would create | ||
-- a lot of separate notifications instead of replacing the same one. | ||
if not complete and not has_nvim_notify() then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need a separate parameter here; you could just check the task status
if task:is_running() and not has_nvim_notify() then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
e964224
to
8673930
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied the suggestions. I'm not squashing yet to make review easier.
-- Like on_complete_notify but, for long-running commands, also show real-time output summary (based on on_output_summarize). | ||
-- Requires nvim-notify to modify the last notification window when new output arrives instead of creating new notification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
---@param bufnr integer | ||
---@param num_lines integer | ||
---@return string[] | ||
local function get_last_lines(bufnr, num_lines) | ||
local end_line = vim.api.nvim_buf_line_count(bufnr) | ||
num_lines = math.min(num_lines, end_line) | ||
local lines = {} | ||
while end_line > 0 and #lines < num_lines do | ||
local need_lines = num_lines - #lines | ||
lines = vim.list_extend( | ||
vim.api.nvim_buf_get_lines(bufnr, math.max(0, end_line - need_lines), end_line, false), | ||
lines | ||
) | ||
while | ||
not vim.tbl_isempty(lines) | ||
and (lines[#lines]:match("^%s*$") or lines[#lines]:match("^%[Process exited")) | ||
do | ||
table.remove(lines) | ||
end | ||
end_line = end_line - need_lines | ||
end | ||
return lines | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to overseer.util
|
||
---@type overseer.ComponentFileDefinition | ||
local comp = { | ||
desc = "Notify with task output summary for long-running tasks or when completed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added explicit note about nvim-notify
|
||
params = { | ||
max_lines = { | ||
desc = "Number of lines of output to show when detail > 1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
update_notification = function(self, task, complete) | ||
-- Don't notify on output without nvim-notify installed, as this would create | ||
-- a lot of separate notifications instead of replacing the same one. | ||
if not complete and not has_nvim_notify() then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
end, | ||
|
||
on_start = function(self) | ||
self.start_time = vim.uv.now() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
min_duration = { | ||
desc = "Minimum duration in milliseconds after which to display the notification", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to delay_ms
output_on_complete = { | ||
desc = "Show output summary even when the task completed", | ||
type = "boolean", | ||
default = false, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't prevent showing the notification after completion, it just configures if the completion notification to shows the last lines of task output. I.e. (with output_on_complete=false
) during runtime we notify with status (RUNNING) and last output, while on completion we notify with status (e.g. SUCCESS) without output lines.
I changed the description and added long_desc
so it should now be clearer.
Awesome, thanks for the PR! |
Adds a component that can be used as a replacement to
on_complete_notify
.on_output_notify
will display a notification with task summary for long running tasks. This is helpful to get quick feedback without opening the overseer taskbar. This new component makes use of nvim-notify to update the existing notification window, so when nvim-notify is not installed it will notify only when the task completes (and will display a one-time warning about nvim-notify).To replace
on_complete_notify
:Example screencast:
overseer_on_output_notify.mp4