Don't discard everyScope selections for multiple targets

[AndreasArvidsson]

Unfortunately became quite a lot of changes to pass the modifier options, but I think this is the only way. It also opens up for more context aware modifiers in the future.

Fixes #2889

[pokey]

Wow this touched a lot of code for such a small edge case. Would it make any sense to pass into the stage constructor instead? Would that help?

I'm also not totally convinced by this fix. Eg what if you have

aaa bbb
ccc ddd

And you say "take bat and drum" and then say "take first token". Would you really expect that to behave differently from "take bat" followed by "take first token"?

I think a better heuristic would be whether all the inputs have the same number of scopes. Ie our current hack/heuristic should either apply to all input targets or none. But that does start to feel maybe hard to reason about?

[AndreasArvidsson]

Wow this touched a lot of code for such a small edge case. Would it make any sense to pass into the stage constructor instead? Would that help?

I don't think that would touch less files. There's a lot of instances where we create a modifier stage and then call it directly after.

I'm also not totally convinced by this fix. Eg what if you have

aaa bbb
ccc ddd

And you say "take bat and drum" and then say "take first token". Would you really expect that to behave differently from "take bat" followed by "take first token"?

True

I think a better heuristic would be whether all the inputs have the same number of scopes. Ie our current hack/heuristic should either apply to all input targets or none. But that does start to feel maybe hard to reason about?

You mean we would actually run the modify stage on each target and then compare the results. If needed rerun the state with different settings? This would be a level of interaction between the different paths of our pipeline we have never done before. I'm not sure if that is simpler or more complicated for the user either.

[AndreasArvidsson]

@pokey I thought some more on the idea of the different instances of the modifier being more aware of each other. My first was to add a optional function runMultiple on the modifier interface. Then the every scope modifier could handle that internally. That would be no problem at all if it was only called from the pipeline, but we have lovely pieces of code like getEveryScopeTargets where the ordinal modifier relies on the every modifier. At that point we only have a single target. Passing an option object through the way I'm already doing in this pull request is fine, but bubbling an exception or similar all the way from the every modifier all the way through getEveryScopeTargets to the ordinal modifier increases complexity quite a lot.

cursorless/packages/cursorless-engine/src/processTargets/modifiers/targetSequenceUtils.ts

Lines 38 to 48 in e5e9929

	export function getEveryScopeTargets(
	modifierStageFactory: ModifierStageFactory,
	target: Target,
	scopeType: ScopeType,
	): Target[] {
	const containingStage = modifierStageFactory.create({
	type: "everyScope",
	scopeType,
	});
	return containingStage.run(target);
	}