Skip to content

Fixed regression in vertical range target #735

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion src/processTargets/processTargets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import { ProcessedTargetsContext } from "../typings/Types";
import { ensureSingleEditor } from "../util/targetUtils";
import getMarkStage from "./getMarkStage";
import getModifierStage from "./getModifierStage";
import PlainTarget from "./targets/PlainTarget";
import PositionTarget from "./targets/PositionTarget";

/**
* Converts the abstract target descriptions provided by the user to a concrete
Expand Down Expand Up @@ -144,7 +146,17 @@ function processVerticalRangeTarget(
anchorTarget.contentRange.end.character
);

results.push(anchorTarget.withContentRange(contentRange));
if (anchorTarget instanceof PositionTarget) {
results.push(anchorTarget.withContentRange(contentRange));
} else {
results.push(
new PlainTarget({
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to use a WeakTarget instead? Might be nice to clean up white space and do weak expansion, given these are more like a bunch of marks in my mind

Copy link
Member Author

Choose a reason for hiding this comment

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

You do make a point but at the same time I do feel that the behavior of sliced targets right now is a fixed range that expands to a column and if I tell it to slice a specific column I don't want anything unexpected to change that range. This is a real edge case but I believe that plain target is more correct.

Copy link
Member

Choose a reason for hiding this comment

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

Ok we have pretty different expectations of slice semantics. Let's leave as plain for now but aim to discuss

Will you miss your rigid block slice semantics if we move to post-slice modifier application? If so then maybe we need two different kinds of slice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed we definitely need to iron this one out

I guess in that case I can just use just/raw to get the same results. But that is definitely something we need to think about.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure "just" helps you here. What exactly were you planning to say?

Fwiw we could just add a new range modifier called "strict slice"

Copy link
Member

Choose a reason for hiding this comment

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

Ok let's plan to discuss

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely

Copy link
Member Author

@AndreasArvidsson AndreasArvidsson Jun 5, 2022

Choose a reason for hiding this comment

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

We could change the grammar a bit
"take line slice air past bat"
"take slice line air past bat"

In these examples it's quite clear which modifies are run before or after the slice. Since we have the slice prefix it would actually be quite simple to turn it into a modifier instead of a (normal) compound target.

Copy link
Member

Choose a reason for hiding this comment

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

yeah worth thinking bout. Fwiw I updated #193 in prep for this discussion. Lmk if that formalism makes sense

Copy link
Member Author

Choose a reason for hiding this comment

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

Works for me

editor: anchorTarget.editor,
isReversed: anchorTarget.isReversed,
contentRange,
})
);
}

if (i === activeLine) {
return results;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
languageId: plaintext
command:
spokenForm: chuck line risk slice made
version: 2
targets:
- type: range
anchor:
type: primitive
mark: {type: decoratedSymbol, symbolColor: default, character: r}
modifiers:
- type: containingScope
scopeType: {type: line}
active:
type: primitive
mark: {type: decoratedSymbol, symbolColor: default, character: m}
excludeAnchor: false
excludeActive: false
rangeType: vertical
usePrePhraseSnapshot: true
action: {name: remove}
initialState:
documentContents: |-
short
something longer
something even longer
selections:
- anchor: {line: 0, character: 0}
active: {line: 0, character: 0}
marks:
default.r:
start: {line: 0, character: 0}
end: {line: 0, character: 5}
default.m:
start: {line: 2, character: 0}
end: {line: 2, character: 9}
finalState:
documentContents: |-

hing longer
hing even longer
selections:
- anchor: {line: 0, character: 0}
active: {line: 0, character: 0}
thatMark:
- anchor: {line: 0, character: 0}
active: {line: 0, character: 0}
- anchor: {line: 1, character: 0}
active: {line: 1, character: 0}
- anchor: {line: 2, character: 0}
active: {line: 2, character: 0}
fullTargets: [{type: range, excludeAnchor: false, excludeActive: false, rangeType: vertical, anchor: {type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: r}, modifiers: &ref_0 [{type: containingScope, scopeType: {type: line}}]}, active: {type: primitive, mark: {type: decoratedSymbol, symbolColor: default, character: m}, modifiers: *ref_0}}]