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

Add an event system #8021

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Add an event system #8021

merged 2 commits into from
Jan 23, 2024

Conversation

pascalkuthe
Copy link
Member

@pascalkuthe pascalkuthe commented Aug 20, 2023

Closes #6910
Closes #5803
Closes #2027
Closes #2581 (moving in insert mode will always close the completion popup now)
Closes #5906 (signature help will be requested regularly on edits and movement like suggested in the LSP standard and therefore closes reliably)
Closes #5962 (I think I am having trouble reproducing but i am pretty aggressive about closing the popups now)
Closes #7497

Also helps with #6248 (now the buggy completion won't be shown automatically although you can still get them manually)

This PR introduces an event/hook system to helix with the goal of decoupling logic from the core ui and editing code. This PR reimplements the completion and signature help features using that event system to showoff how the system works in practice.

I specifically choose these two subsystems since I have mentally accumulated a large list of issues with these systems that would both be fixed by treating them as an async debounced event handler. So the refactor here was necessary anyway but thanks to the event system this doesn't spread a ton of extra code everywhere.

I noticed that there are actually many more opportunities where the event system could help to move subsystem data out of the core editor code. For example:

  • diff gutter could be moved to DocumentDidChange
  • diff gutter could be turned into an async hook
  • lsp document change notification could be moved to DocumentDidChange
  • LSP inlay hint requests (fixing many of the issues described in the original PR around better timeouts)
  • both diff gutter and lsp subscribe to :reload (and lsp also to :write) that can also be handled by the event system

I refrained from implementing all of these to make this already large PR completely unmanageable. Once this PR lands I will send followup PRs to update systems where it makes sense (again with the primary focus on systems that see functional improvements such as inlay hints).

The event system also opens up the possibility to subscribe to events by name. This could allow us to run shell scripts on save (for example) and is also an important step towards a plugin system IMO. While the basic infrastructure exists it's not hooked up to a config option currently to keep this PR manageable.

Note that this PR currently includes #7814 to avoid introducing conflicts with the code there so the diff looks inflated.

The completion timeout is currently set to a very low value. This is intentional for testing (since lower values are more interesting for triggering edgecases) but will be raised before merge likely back up to 400ms. Note that the idle timeout-based completion timeout is currently broken on master anyway (always 33ms timeout) due to a regression so comparisons may seem off I am working on a fix for that too (since some stuff still uses the idle timeout) but that's orthogonal

@pascalkuthe pascalkuthe added C-enhancement Category: Improvements E-hard Call for participation: Experience needed to fix: Hard / a lot S-waiting-on-review Status: Awaiting review from a maintainer. S-waiting-on-pr Status: This is waiting on another PR to be merged first S-needs-testing Status: Needs to be tested out in order to discover potential bugs. E-testing-wanted Call for participation: Experimental features suitable for testing labels Aug 20, 2023
@pascalkuthe pascalkuthe force-pushed the event_system branch 2 times, most recently from c119feb to 60d22bc Compare August 20, 2023 23:11
@pascalkuthe
Copy link
Member Author

This could also be a good chance to close #5054, the way completions trigger is somewhat different now so we can do some testing, settle on a completion timeout (stick with 400, or lower to 300/200?) and then close that issue. The concern about the idle timeout being set to low values discussed there isn't really present anymore. Setting the completion timeout to something super low (like 0 but I would recommend 5 for those that want instant completions) so once we have made a decision about the default there is nothing that needs to change there.

helix-event/src/lib.rs Outdated Show resolved Hide resolved
helix-event/src/lib.rs Outdated Show resolved Hide resolved
@pascalkuthe pascalkuthe force-pushed the event_system branch 7 times, most recently from 4d579ef to 15fbfa9 Compare August 30, 2023 13:11
@pascalkuthe pascalkuthe removed the S-waiting-on-pr Status: This is waiting on another PR to be merged first label Aug 31, 2023
@pascalkuthe pascalkuthe added this to the next milestone Sep 1, 2023
helix-event/src/debounce.rs Outdated Show resolved Hide resolved
helix-event/src/debounce.rs Outdated Show resolved Hide resolved
helix-event/src/debounce.rs Outdated Show resolved Hide resolved
helix-event/src/debounce.rs Show resolved Hide resolved
helix-event/src/debounce.rs Outdated Show resolved Hide resolved
helix-event/src/hook.rs Outdated Show resolved Hide resolved
helix-event/src/lib.rs Outdated Show resolved Hide resolved
helix-event/src/lib.rs Outdated Show resolved Hide resolved
helix-event/src/lib.rs Outdated Show resolved Hide resolved
helix-event/src/lib.rs Outdated Show resolved Hide resolved
@inifares23lab

This comment was marked as off-topic.

# to set those most of the time. If downstream does overwrite this its not a huge
# deal since it will only break tests anyway
[target."cfg(all())"]
rustflags = ["--cfg", "tokio_unstable", "-C", "target-feature=-crt-static"]
Copy link
Member

Choose a reason for hiding this comment

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

Why is crt-static also enabled here now?

Copy link
Member

Choose a reason for hiding this comment

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

And why are we applying this flag in all cases now, could we only apply it for test builds?

Copy link
Member Author

@pascalkuthe pascalkuthe Dec 12, 2023

Choose a reason for hiding this comment

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

crt static is being disabled (not enabled that would be +crt-static instead of -crt-static). The reason its here is that if we set RUST_FLAGS env variable then rusflags from .cargo/config.toml gets overwritten. On some distros (like musl) it's necessary to disable static linking for helx to work (helix can never work with a statically linked libc).

While its true that tokio_unstable is only needed during (integration test) the flag doesn't matter much (these tokio functions always exists as they are used internally, the flag just makes them pub). I tried coming up with a way to only enable during integration testing but that doesn't seem to work since you can't do cfg based on feature in cargo/config.toml only targets. cfg(test) doesn't work since cfg test only applied to the crate being tested - in this case helix-term - and not dependencies - in this case helix-event -

the-mikedavis
the-mikedavis previously approved these changes Dec 20, 2023
Copy link
Member

@the-mikedavis the-mikedavis left a comment

Choose a reason for hiding this comment

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

I've been using this branch for quite a while and building on it locally for changes like moving DynamicPicker away from IdleTimeout. The details around lifetimes are a little over my head but I'm liking the API for creating and attaching to events, and the completion changes look good to me as well.

helix-event/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

Alright, I've reviewed again, spent some testing and I'm finally ready to merge this! Thanks for taking the time to design and implement this, it should allow us to clean up the spaghetti code around LSP handling quite a bit!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Improvements E-hard Call for participation: Experience needed to fix: Hard / a lot E-testing-wanted Call for participation: Experimental features suitable for testing S-needs-testing Status: Needs to be tested out in order to discover potential bugs. S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet