Clarify Context

[gibbz00]

Summary:

Context -> CompositorContext
Context -> CommandContext

Driving me nuts when trying to understand code with cx/ctx/cxt when both commands::Context and compositor::Context are in scope.

[dead10ck]

compositor::Context is actually the more idiomatic naming convention, so this is not a good change even on the face of it (e.g. imagine if std::io::Result was std::io::IoResult). But even so, we shouldn't be doing these large scale renames just for the sake of it. They serve no purpose but your own personal preference, while breaking a large number of open PRs. There should be a strong justification for these kinds of changes.

[archseer]

I agree, the namespacing was intentional here. Not all types in a codebase need to have a unique name. Similarly editor::Config vs EditorConfig in the other PR was intentional.

[gibbz00]

compositor::Context is actually the more idiomatic naming convention, so this is not a good change even on the face of it (e.g. imagine if std::io::Result was std::io::IoResult).

That would be good and all were it not for the fact that Context is seldom qualified with commands/compositor::Context. (Global searching for " Compositor" returned 358 results.) And that there's statusline::RenderContext and lsp::{CompletionContext, SignatureHelpContext, ReferenceContext, CodeActionContext}...

Also, signature help never shows the fully qualified module path, only Context. So it is impossible to know which parent module it belongs to by using it over cxs. Each time I'm unsure of what type cx is, must jump back to the method signature definition, to then use the signature help on the parameter.

Here are just two examples where it is quite ambiguous what type Context is when trying to understand what the code does, good luck:

helix/helix-term/src/ui/editor.rs

Lines 1234 to 1248 in 44729fb

	if let Some(on_next_key) = self.on_next_key.take() {
	// if there's a command waiting input, do that first
	on_next_key(&mut cx, key);
	} else {
	match mode {
	Mode::Insert => {
	// let completion swallow the event if necessary
	let mut consumed = false;
	if let Some(completion) = &mut self.completion {
	// use a fake context here
	let mut cx = Context {
	editor: cx.editor,
	jobs: cx.jobs,
	scroll: None,
	};

Answer

`cx` on line 1236 is `command::Context` and `cx` on line 1244 is `compoitor::Context`.

helix/helix-term/src/commands.rs

Lines 5305 to 5317 in 44729fb

	cx.editor.macro_replaying.push(reg);
	let count = cx.count();
	cx.callback = Some(Box::new(move |compositor, cx| {
	for _ in 0..count {
	for &key in keys.iter() {
	compositor.handle_event(&compositor::Event::Key(key), cx);
	}
	}
	// The macro under replay is cleared at the end of the callback, not in the
	// macro replay context, or it will not correctly protect the user from
	// replaying recursively.
	cx.editor.macro_replaying.pop();

Answer

`cx` on line 5305 is `commands::Context` and `cx` on line 5317 is `compositor::Context`.

I would argue that there are about ~1600 or so more examples (each time cx/ctx/cxt/context is used as a placeholder). But I'll leave it at that.

In a perfect world, I would have preferred cx/ctx/cxt should be renamed to be renamed something like cmd_cx and cmp_cx respectively. But that would ofcourse be an even bigger PR that breaks a larger bunch.

Sure, the other PRs of mine will break some existing PRs. But I really do believe there should be an increased effort in keeping the code base readable and in turn maintainable. Not merging refactorings because "it will break other PRs" is a sure fire way to amassing exponential technical debt. And frankly, that is in my opinion where Helix is now in many of its components.

I really don't want to bash on you guys, the work you've done truly is awesome. And it's not like I want to make your life harder. Quite the opposite, being open to code readability improvements will in the end make future reviews much easier.

[archseer]

In a perfect world, I would have preferred

keeping the code base readable

in my opinion

A lot of this is subjective and we could of course argue about variable naming all day. I preferred the current version of the code which is why I named the variables that way in the first place.

I agree with a lot of the functional cleanups you've made in your keymap PR but as we've previously stated renaming and moving code around shouldn't be done unless there's a clear benefit beyond just personal preference. The Context renaming here can be argued and it might fit the bar but a lot of these do not (bbfeafd) and only make actual changes harder to review. I'd gladly merge #6061 today if it didn't also hold other changes for example but now it's going to be stuck until we can clarify the remaining commits in the branch.

[archseer]

Here's an example of the lengths we're going to in order to avoid breaking lots of open work: #5581 The refactor there is a lot more crucial (it unblocks new functionality) but we're ensuring there will be close to no conflicts to resolve. For each PR with break we risk the author abandoning the work and having to adopt the PRs ourselves.

[gibbz00]

Ok, so instead of going back and forth with "this might not fit the bar because it might break a lot of PRs", I've now thrown together a bash script that checks if local branch conflicts with open PRs that are mergeable with master, and pass all CI checks.

#6061, and by extension, this PR, did not conflict with a single one.(Edit: Wrong branch 😅, 32 conflicts)

Here's the script if you're interested in trying it out: https://github.com/gibbz00/conflict

[dead10ck]

Hm, that's funny, because I ran your script locally and after merging this PR into master, there are 32 conflicting PRs.

== SUMMARY ==
Number of PRs checked 261
Conflicting PRs (32) : #5393 #5523 #5577 #5580 #5748 #5751 #5803 #583
7 #5911 #5934 #5966 #5983 #6024 #6046 #6067 #4171 #4289 #4443 #4642 #
4718 #4726 #4767 #4780 #5199 #5244 #5260 #5341 #2869 #3239 #3328 #339
3 #3652
Checks faiure PRs (11) : #5635 #5636 #5768 #5964 #5991 #6027 #4398 #4
756 #5290 #3523 #3851
Unmergeable with master PRs (116) : #5390 #5472 #5492 #5531 #5534 #55
35 #5581 #5712 #5801 #5826 #6002 #4188 #4189 #4201 #4204 #4244 #4252
#4276 #4287 #4306 #4313 #4329 #4364 #4411 #4490 #4522 #4535 #4545 #45
73 #4584 #4590 #4617 #4635 #4649 #4694 #4708 #4729 #4787 #4816 #4818
#4860 #4892 #4905 #4998 #5037 #5041 #5046 #5204 #5269 #5340 #5355 #53
57 #5371 #5379 #5383 #611 #1405 #1543 #1570 #1723 #1777 #1819 #1827 #
1905 #1988 #2053 #2135 #2190 #2208 #2377 #2423 #2452 #2507 #2607 #265
3 #2672 #2704 #2705 #2715 #2733 #2796 #2852 #2857 #2883 #2905 #2935 #
2977 #3082 #3207 #3242 #3249 #3282 #3288 #3322 #3347 #3360 #3380 #345
7 #3462 #3491 #3516 #3599 #3631 #3642 #3688 #3690 #3696 #3752 #3769 #
3783 #3791 #3840 #3944 #3966 #3970 #4144

It's also likely under counting because it's not counting PRs that have existing conflicts but would also conflict with this change after they were fixed, nor is it counting PRs that would conflict after their CI checks pass.

But you're also not listening.

1. We do not like this rename, regardless of conflicting PRs
2. We do not like PRs that rename things without a clear purpose other than personal preference

Breaking PRs is inevitable—that is just going to happen when more than one person is working on the same code base. But the thing you're not getting is that we value all contributions, and the harder we make it for people to contribute, the less likely they are to do so. Every time we make someone rebase and fix conflicts, we risk frustrating them and losing their PR. So we must take care to avoid needless breakage that does not serve a good purpose.

This kind of "rename a type to suit my preferences" PR not only adds very little value to people other than yourself, but takes a shotgun to other people's contributions that we must ask them to repair. And it also makes it especially difficult to review PRs with actual value in them when this kind of change is thrown in.

[gibbz00]

Oopsie, wrong branch on my end 😅 But yeah also got 32 conflicting PRs now. #6061 also got 32, which I presume all came from this one.

So I've now closed #6061 and opened up a new one with cherry-picked commits only relevant to apply_motion.

I'll refrain from countering the other points you made, as I don't think anything constructive will come out of it.

I will keep this PR open and leave it up to @archseer to decide if he wants it closed. If he does; we just move on. To which I would be fine with now that I've had the opportunity to express my intent behind this PR.

[askreet]

Hello!

As a new contributor who recently has been learning the code base I have some fresh, um, context here. Please take this with a grain of salt as I've only been poking at the code a few weeks.

I do think the "what context is this" problem is real for understanding the codebase in the helix-term crate. But the more code I read, the more often I see this pattern:

• Pass some context object to a function.
• Probably use only the Editor from it.
• Probably use a macro to extract the current view and/or document from the Editor.

I find myself wondering why many functions aren't just accepting a combination of Editor, View and Document borrows. This also helps with the borrow checker not always getting field-level borrows correctly (especially true through function boundaries).

Would a reduction in proliferation of passing around Context objects go further to reduce cognitive load than trying to name the Context objects better or make sure they are relatively qualified (e.g., compositor::Context) more often?

Obviously a giant PR changing the signature of 1,000 functions isn't going to help anyone either, but in my experience just documenting a change in direction alongside some motivation and historical context can elicit a lot of momentum (e.g., via ADRs).

[pascalkuthe]

Hello!

As a new contributor who recently has been learning the code base I have some fresh, um, context here. Please take this with a grain of salt as I've only been poking at the code a few weeks.

I do think the "what context is this" problem is real for understanding the codebase in the helix-term crate. But the more code I read, the more often I see this pattern:

* Pass some context object to a function.

* Probably use only the Editor from it.

* Probably use a macro to extract the current view and/or document from the Editor.

I find myself wondering why many functions aren't just accepting a combination of Editor, View and Document borrows. This also helps with the borrow checker not always getting field-level borrows correctly (especially true through function boundaries).

Would a reduction in proliferation of passing around Context objects go further to reduce cognitive load than trying to name the Context objects better or make sure they are relatively qualified (e.g., compositor::Context) more often?

Obviously a giant PR changing the signature of 1,000 functions isn't going to help anyone either, but in my experience just documenting a change in direction alongside some motivation and historical context can elicit a lot of momentum (e.g., via ADRs).

Context is much more than just a way to pass editor around. Firstly you can't pass a reference to an Editor, View and Document to a function at the same time because Views and Documents are stored inside the editor so you can have either a mutable reference to views/documents or the editor itself but not both.

The Context structs exist to provide a series of utility methods and for temporary storage. For example the Context in commands.rs contains the register and count passed to a command, a callback which will be executed after the command is finished with a mutable reference to the compositor so gui elements can be spawned (this is again not possible from commands directly, because commands they are called from inside the event handler of a component which is stored inside the compositor) and a on_next_key_callback that allows more flexible keymaps (like accepting any chars after f/t).

I think passing all these arguments around manually would be much more unreadable. Someone looking at a command just needs to know: this is the context the command is executed in, which is the commands way to interact with the rest of the codebase and then only needs to care about the parts of the context a command uses (usually the editor).

This is also the only real sustainable way to pass arguments to commands. Imagine if every command received all these arguments explicitly and we decided to remove or change one of these fields. If they are all function parameters you would need to change all commands (and there are a LOT of commands). This would not only produce a huge diff in every change but also cause a huge number of merge conflicts and (arguably even worse) would break any PR that adds a new command (since the signature is now inccorect) without even showing up as a conflict (or causing a CI failure until the PR is merged or rebased)

[askreet]

I agree that having a Context to keep a stable signature for command handlers makes sense, but I'm not sure it's the only way to solve such a problem. I'm also not suggesting it's a bad way to solve such a problem. I'm academically interested in considering alternatives.

Looking back through the code, it does seem the prevailing use of Context is in the entrypoint of command handlers. I may be mis-remembering it being more pervasive than that. Many of the factored out functionality does operate on at least an Editor or even small units of data.

From a testing perspective it does force all command testing up into integration tests, since replacing a Context is a challenging endeavor. I don't really know a better pattern there than using trait-based generics for the needed functionality.

Imagine if every command received all these arguments explicitly and we decided to remove or change one of these fields.

My suggestion was more along the lines of, "Imagine if every command received only the arguments it performed operations on or with". As it stands the Context is a bit of a god object that is sliced and diced to find what each command needs. Effectively, it can only grow over time.

[dead10ck]

My suggestion was more along the lines of, "Imagine if every command received only the arguments it performed operations on or with". As it stands the Context is a bit of a god object that is sliced and diced to find what each command needs. Effectively, it can only grow over time.

This would be a maintenance nightmare. This effectively would mean there is no standard API for every command, and every command execution and handling would have to be special cased. By comparison, having extraneous data in a context object for some commands is a small price to pay, if indeed there is any practical cost at all.

I agree that having a Context to keep a stable signature for command handlers makes sense, but I'm not sure it's the only way to solve such a problem. I'm also not suggesting it's a bad way to solve such a problem. I'm academically interested in considering alternatives.

Do you have any other suggestions?

[askreet]

I want to clarify that I'm not (yet) an expert in Rust just interested in having a discussion and shared the OPs challenge in acclimating to the code base in this one area as well. I'm concerned I'm offending people by bringing the discussion up at all, and that's certainly not my intention. Working on Helix is really nice compared to many projects of this scale. For better or worse, my personality is to always push on the structures of an architecture and see where it bends and breaks in the hopes of making meaningful improvements.

This would be a maintenance nightmare.

Yeah I'm not suggesting it would be an improvement to hard-wire up a billion functions. Macros might be one option, or classifying functions into different buckets based on their needs (e.g., a function type that operates on the current document and view, versus one that operates on the editor as a whole).

Do you have any other suggestions?

The one example I have that came to mind is how bevy implements systems as plain functions for the ECS model. It uses generics that specify what types of entities will be passed to the handler function so that you end up with a strongly typed interface with copies of or references to the entities you wish to operate on.

[archseer]

From a testing perspective it does force all command testing up into integration tests

This is a net benefit from my view. "functional core imperative shell" (also the reason selections and transactions are immutable). Commands are composed of helix-core code that's unit tested, and helix-term code that's integration tested so we can actually validate how it works in the real world and in combination with other commands.

I find myself wondering why many functions aren't just accepting a combination of Editor, View and Document borrows.

Historically (~2+ years ago): I started with a simpler context that used only these fields (passing in a current doc and view) and the trouble is that some commands end up using multiple views or multiple documents. You can't pass both a mutable Editor borrow and a Document borrow (that lives on that Editor) at the same time.

I've gone through a few iterations here and thought about the API. I've also considered returning Transaction/Option<Transaction> from commands to hide away the tx application, but:

• a great deal of commands would return `None
• some commands switch documents and would have to apply it to the previous doc
• some commands apply multiple transactions
• and some apply transactions to multiple views

It's much more tenable to have a single, versatile command signature that doesn't make any assumptions about what the command is trying to do.

Effectively, it can only grow over time.

It really hasn't though. It has a reference to the editor, the current input count and the current register. The rest is just plumbing to allow you to schedule async jobs.

[askreet]

Great context, I really appreciate you writing this up.

This is a net benefit from my view. "functional core imperative shell" (also the reason selections and transactions are immutable). Commands are composed of helix-core code that's unit tested, and helix-term code that's integration tested so we can actually validate how it works in the real world and in combination with other commands.

You know it's funny, the first things I wanted to poke at were commands and I found writing tests difficult* because I couldn't reasonably pass state into a command and inspect it's output (as you say below, by getting an Option<Transaction> for example). I hadn't really considered the command structure as the imperative shell around a more function core editor experience, I like that description.

(*I have not yet taken the time to learn the integration testing suite, my bad.)

It really hasn't though. It has a reference to the editor, the current input count and the current register. The rest is just plumbing to allow you to schedule async jobs.

Fair enough!