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!: add stack upwards option #160

Merged
merged 3 commits into from
Nov 13, 2023
Merged

Conversation

0xAdk
Copy link
Contributor

@0xAdk 0xAdk commented Nov 12, 2023

general idea: instead of having a lines and highlights arrays that we append to, we create an array of "rendered items" which then get converted into the lines and highlights arrays with correct ordering.

A render_item just contains the lines and render_item relative highlights.

Not sure about: https://github.com/0xAdk/fidget.nvim/blob/12f4be2e4f2e887b4aeec97b0d392fdd2c4794d6/lua/fidget/notification/view.lua#L144-L152
I went from string.gmatch to vim.gsplit since it's cleaner and in testing found gmatch implicitly converts from numbers to strings, so I added it explicitly.

Set stack_upwards to true by default since that was how legacy worked.

closes #152

@j-hui
Copy link
Owner

j-hui commented Nov 13, 2023

This is awesome, thanks for working on this!

general idea: instead of having a lines and highlights arrays that we append to, we create an array of "rendered items" which then get converted into the lines and highlights arrays with correct ordering.

A render_item just contains the lines and render_item relative highlights.

I really like the intermediate NotificationRenderItem, it makes the algorithm a lot easier to understand. I refactored things a little to simplify the main render() loop, mostly putting each NotificationRenderItem-generating subroutine in its own helper function. Since each render item is self-contained, this was really straightforward.

And then rather than separately reverse the list, I just iterate through the render items in the reverse order.

I went from string.gmatch to vim.gsplit since it's cleaner and in testing found gmatch implicitly converts from numbers to strings, so I added it explicitly.

I didn't realize vim.gsplit() existed, and you're right that it's a lot cleaner. We don't need to worry about converting numbers to strings, that's a type error. (I will add a separate check in a different commit to enforce that.)

Set stack_upwards to true by default since that was how legacy worked.

Sounds good to me. (Note to self: I need to re-record the demo GIF.)

@j-hui j-hui changed the title feat: add stack upwards option feat!: add stack upwards option Nov 13, 2023
@j-hui j-hui merged commit 03d2192 into j-hui:main Nov 13, 2023
@0xAdk
Copy link
Contributor Author

0xAdk commented Nov 13, 2023

one minor thing, instead of having

local foo = M.render_foo()
if foo then
	table.insert(render_items, foo)
end

can't you just have

table.insert(render_items, M.render_foo())

since table.insert(tbl, nil) doesn't do anything (I think; based on testing, couldn't find any docs about this).

@j-hui
Copy link
Owner

j-hui commented Nov 13, 2023

While that might be correct, I prefer it as I’ve written it because I think it makes it clearer that sometimes, nothing is inserted into the list.

(Micro-optimization hat on: we avoid the function call overhead to table.insert() when we don’t need it. But I don’t think this small overhead actually matters.)

j-hui added a commit that referenced this pull request Nov 13, 2023
j-hui added a commit that referenced this pull request Nov 13, 2023
In particular, PR #160 introduced this option and made it the default.

I wouldn't have had to tweak these configs if it weren't for the fact
that this new stacking order ruins the "bottom text" joke. smh
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.

feat: bring back stack_upwards
2 participants