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

Rewrite fidget.nvim #131

Closed
j-hui opened this issue Jun 10, 2023 · 15 comments · Fixed by #143
Closed

Rewrite fidget.nvim #131

j-hui opened this issue Jun 10, 2023 · 15 comments · Fixed by #143
Labels
enhancement New feature or request reusability

Comments

@j-hui
Copy link
Owner

j-hui commented Jun 10, 2023

I've been sitting on this idea for too long; fidget was always a pun on "widget" but I could never decide what the right abstractions were. But now that I've studied/worked on a couple of reactive/synchronous frameworks, I'm finally going to give this a shot. Besides, #130 gave me the kick in the ass I needed (more on the PR later).

Some ramblings about the execution model: my first implementation is basically a fancy event-driven state machine, and is really not extensible. It works as a proof of concept, but it's ugly, unintuitive to configure or extend, and slow (I actually disable this plugin in environments where I am often working with noisy LSP servers).

To address the speed issue, something like neovim/neovim#23958 will provide some form of rate-limiting and also address space leaks in the messages buffer, but I also think fidget.nvim can be much smarter about how often it polls Neovim's message buffer. In particular, we shouldn't need to check it any faster than 40Hz (probably lower), because animations don't need to render faster than that, and notifications don't need to be shown immediately.

Instead, I plan on driving the execution of fidget with a heartbeat (that can be let to idle if there's no activity).

The other part I want to change is to extend fidget to be a general-purpose framework for constructing animated widgets in Neovim. I want to follow some kind of model loosely inspired by Flutter, with a tree of widgets that compose efficiently and intuitively. The LSP progress is just one instance of things we can do with this framework (and of course will always have first-class support in this plugin), but I'd also like this to be usable for notifications (vim.notify), widget dashboards, and all the stuff you might want to take out of your modeline to keep it clean and minimal. Edit: this never happened, it's simply too overkill.

Oh and also, I really want to get down to the bottom of this business with highlights with extmarks. It seems super flaky and I can't tell if the problems are due to one's editor settings, terminal, font, OS, or who knows what else. Rewriting fidget as a UI framework not only gives me a chance to rework all of that, but it'll also make it easier to test and reproduce problems (instead of sending a progress notification, we can just ask fidget to draw a text box). Edit: this didn't happen either, but is still possible and may be worth experimenting with.

This is all I'll write for now, and I primarily do so to keep myself accountable and folks informed. With any luck, I'll complete this rewrite within a couple of months.

@j-hui j-hui added enhancement New feature or request reusability labels Jun 10, 2023
@j-hui j-hui pinned this issue Jun 10, 2023
fnichol added a commit to fnichol/dotneovim that referenced this issue Jun 11, 2023
References: j-hui/fidget.nvim#131

Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
iovis added a commit to iovis/dotfiles that referenced this issue Jun 11, 2023
piouPiouM added a commit to piouPiouM/dotfiles that referenced this issue Jun 12, 2023
@kmoschcau
Copy link

It might be worth having a look at https://github.com/rcarriga/nvim-notify and getting in touch with the maintainer. Both plugins seem to aim for something similar.

Alienover added a commit to Alienover/dotfiles that referenced this issue Jun 13, 2023
cativovo pushed a commit to cativovo/.dotfiles that referenced this issue Jun 14, 2023
cativovo pushed a commit to cativovo/.dotfiles that referenced this issue Jun 14, 2023
piouPiouM added a commit to piouPiouM/dotfiles that referenced this issue Jun 14, 2023
@jyfzh
Copy link

jyfzh commented Jun 14, 2023

folke/noice.nvim seems also have the ability to show lsp progress. it can be used as a reference see: https://github.com/folke/noice.nvim/wiki/What's-New%3F

@goolord
Copy link

goolord commented Jun 15, 2023

no offense, would you consider removing your fidget.nvim will son be rewritten message? most package managers will highlight breaking: ... commits which should be plenty notice

@j-hui
Copy link
Owner Author

j-hui commented Jun 16, 2023

@goolord If you want to suppress the message, just checkout the legacy tag, which does not have the deprecation notice.

@che247
Copy link

che247 commented Jun 16, 2023

@j-hui Sorry, this might be a trivial question, but how does one checkout the 'legacy' tag in a lua config? I've tried putting 'tag' = 'legacy' when lazy loading fidget and also in a setup function for fidget but neither of these two options have worked in suppressing the message.

@nwkm
Copy link

nwkm commented Jun 16, 2023

@che247 Make sure you have also updated the plugin using plugin manager after putting tag = 'legacy'. It worked for me.

@che247
Copy link

che247 commented Jun 17, 2023

That worked for me.. I don't know if deleting it first and then having lazy reinstall the package also helped but I did that as well as manually updating the package in Lazy as well. Thank you @nwkm!

adoyle-h added a commit to adoyle-h/one.nvim that referenced this issue Jun 19, 2023
@jose8a
Copy link

jose8a commented Jun 27, 2023

That warning message is getting rather annoying, and I only just saw it in the last one hour. I will have to stop using this if I can't find a fix for it since the recommended fix isn't working for me for some reason I can't yet find.

Tried using the legacy tag a few different ways, but from comments above, this line below should work, and yet it doesn't even after removing and reinstalling fidget. Any suggestions on what else I can try?

      { 'j-hui/fidget.nvim', opts = { tag = 'legacy' } },

edit: fyi, I'm using Lazy w/mason.

@fitrh
Copy link

fitrh commented Jun 27, 2023

@jose8a AFAIK, the tag spec is not part of opts

{
  'j-hui/fidget.nvim',
  tag = 'legacy',
}

sentriz added a commit to sentriz/dotfiles that referenced this issue Jul 3, 2023
@MrZLeo
Copy link

MrZLeo commented Jul 16, 2023

I am wondering why we need to change the branch here. Can we just do the rewrite in another branch and merge into main once everything is finished?

@matzxrr
Copy link

matzxrr commented Jul 20, 2023

@jose8a AFAIK, the tag spec is not part of opts

{
  'j-hui/fidget.nvim',
  tag = 'legacy',
}

Worked for me, just remember to run Lazy's sync command.

@Integralist
Copy link

👋🏻 Any chance of getting a milestone created that people can track progress?

Thanks!

@j-hui
Copy link
Owner Author

j-hui commented Nov 9, 2023

With any luck, I'll complete this rewrite within a couple of months.

Came up a little short on luck, but hopefully not too short. Thanks for your patience, everyone.

I have something working on the dev branch. There's still a bit of documentation work that needs to be done, but at least I think the code is almost ready for release (:

Gone is the old, single-file mess of a state machine. And here's some new features:

  • Plays nice with Neovim's built-in LSP progress handler
    • No longer overriding the ["$/progress"] handler like the old implementation did
    • Compatible with both the 0.8.0-0.9.X handler and the one currently on nightly (i.e., to appear in 0.10.0)
  • Support for vim.notify()-like interface
    • Support "notification groups", which can each have a separate configuration for things like highlights and icons (still supports animations, of course). Sorted according to priority.
    • Notification groups and items are indexed by user-specified keys, so updating/overwriting them is really easy.
    • Each LSP server can be configured with its own notification group.
  • Explicit DOM/model for notifications subsystem
    • I can advance the state of the notifications model without worrying about rendering/textlock
    • Makes debugging and testing much easier (though a thorough test suite has yet to be implemented)
    • Should make cacheing renders easier, to avoid redrawing the terminal (though this is unimplemented so far)
  • Synchronous execution model, i.e., "tick-driven"
    • The LSP progress and notification subsystems are running at independent rates and communicate asynchronously
    • Only poll when work needs to be done
    • Animations are much more stable (i.e., frames are always computed wrt synchronized wall clock time)
    • Rate-limiting/deliberate frame-dropping is theoretically possible
  • Implementation is much more modular, configurable, and hackable
    • Options are now declarative, with the setup() function being constructed programmatically.
    • The structure of the options follows the structure of the implementations' modules; the two will hopefully keep each other sane and navigable.
    • A lot of modules have their members exposed publicly, so many more things beyond the setup options are technically overridable.

Yeah, some of these implementation decisions are questionable or over-engineered, but at least the code feels much more flexible and hackable now. I've added several features in the last couple of days that took little effort with the current design/organization.

A few things still need to be done before I release (and I promise I'll actually get these done within the week). Mostly busy work:

  • Add commands functions to suppress polling progress, displaying notifications, etc. Commands can come later (I want a system of subcommands to avoid clogging up the : namespace)
  • Make sure setup() function is re-entrant (don't register multiple LspUpdate callbacks!)
  • Write up the :help documentation
  • Setup a proper versioning scheme on the main branch; no more legacy branch/tag disaster decisions (it starts after this last little disaster)
  • Tag (and eventually close) all the relevant Issues
  • Write some unit tests (I won't let this hold me up but I really want everything to be properly tested)

@j-hui j-hui changed the title Rewrite fidget.nvim as an extensible UI widget framework Rewrite fidget.nvim Nov 9, 2023
@j-hui j-hui linked a pull request Nov 10, 2023 that will close this issue
@polyzen
Copy link

polyzen commented Nov 10, 2023

For any lazy.nvim users who had tried out the dev branch (it's not happy if you had specified a branch that was later deleted):
folke/lazy.nvim#1052 (comment)

Or delete the relevant fetch = lines from ~/.local/share/nvim/lazy/fidget.nvim/.git/config.

@j-hui
Copy link
Owner Author

j-hui commented Nov 10, 2023

Oof sorry about that, I forgot that GitHub would automatically delete my branch after that PR closed. But I guess I'll leave it deleted since that was only meant to exist temporarily, and I do want folks to move onto main now.

Thanks for pointing out the relevant solution @polyzen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request reusability
Projects
None yet
Development

Successfully merging a pull request may close this issue.