Open xcaptain opened 4 years ago
Great observation! I think we maybe should take cursor position into account as well? That is, cursor should stay inside or outside the braces. A good flea would be to check what IntelliJ does in this case. The fix itself shouldn’t be too hard
On Wednesday, 2 October 2019, Joey notifications@github.com wrote:
https://github.com/rust-analyzer/rust-analyzer/blob/ 31f22d85491d9e7eaadf5fd4f9754c83fc0f3ea6/crates/ra_ide_api/ src/matching_brace.rs#L41
swap the order of do_check parameters the makes the test case failed
do_check("struct Foo <|>{ a: i32, }", "struct Foo { a: i32, }<|>");
this matching behavior should be symmetrical
https://github.com/rust-analyzer/rust-analyzer/blob/ 31f22d85491d9e7eaadf5fd4f9754c83fc0f3ea6/crates/ra_ide_api/ src/matching_brace.rs#L18-L19
if the matching node is a left brace, we use start(), if the matching node is a right brace, we should use end()
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/rust-analyzer/rust-analyzer/issues/1942?email_source=notifications&email_token=AANB3MYTNRCKYXBWIMOR2VLQMRCABA5CNFSM4I4SUEAKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HPBPAWQ, or mute the thread https://github.com/notifications/unsubscribe-auth/AANB3M2SHFDOQXXCUHP4KZLQMRCABANCNFSM4I4SUEAA .
That is, cursor should stay inside or outside the braces.
I think there are two problems with that:
(<|>(
we have to choose one brace.<|>(
, I'd expect to go to <|>)
.I just happened to run into this problem, feel free to close it if it's not a bug :)
@xcaptain no, I think this is a reasonable thing to do. That is, current impl does something, but it wasn't really well thought out. I'll be happy to accept any PR which makes this more consistent, with both bar and block cursor models.
Hm, I've realized why I haven't noticed this before: I am using a block caret, and, with a block caret, current behavior is pretty reasonable: cursor is always "on" the brace.
But, for a line caret, the behavior is indeed strange, because the cursor visually jumps from outside to inside of the block.
I've checked what IntelliJ does, and it always uses start/end ranges like this issue proposes, which works perfectly for a line cursor (cursor is always outside of parenthesis). For a block cursor, IntelliJ places it on the opening brace, but after the closing brace.
The build-in VS Code brace matching does the opposite, it "favors" block cursor.
I'd like to say that both VS Code and IntelliJ are not perfect, and the behavior really should be depending on the current cursor visual. However, that means that the effect of editor's macros depends on UI settings, and that's wrong.
So I think what we need here is:
In other words, action item here is to implement this is an op-in tweak to the TS side of the plugin, here. Sorry for not digging to the root of it from the start: I thought that the correct behavior hear is "obvious", but apparently it isn't.
Yeah, the difference between block/line caret was what I was getting at :)
Hi, I'd like to try working on this during my company's hackathon week. :smile:
I think maybe we could tweak the behavior according to vscode's editor.cursorStyle
setting instead of creating a new setting for users to set.
I also observed a related matching issue with the following Rust code:
let x = (1 + (2 + 3)) * 4;
In a situation like <|>(1 + (2 + 3)) * 4
, it will go to (1 + (2 + 3)<|>) * 4
, and if user tries to query matching bracket again it will go to (1 + <|>(2 + 3)) * 4
while logically the expected result should be <|>(1 + (2 + 3)) * 4
. This behavior exists in both line cursor style and block cursor style.
Probably because when matching brace is called at (1 + (2 + 3)<|>) * 4
, the parenthesis to the left is returned at
because find_map
returns the first successful result. So when there are multiple consecutive braces, the brace to cursor's left would be returned when VSCode actually prefers to highlight the brace to the right. I am not sure if this should be viewed as a bug but it can be confusing.
https://github.com/rust-analyzer/rust-analyzer/blob/31f22d85491d9e7eaadf5fd4f9754c83fc0f3ea6/crates/ra_ide_api/src/matching_brace.rs#L41
swap the order of
do_check
parameters the makes the test case failedthis matching behavior should be symmetrical
https://github.com/rust-analyzer/rust-analyzer/blob/31f22d85491d9e7eaadf5fd4f9754c83fc0f3ea6/crates/ra_ide_api/src/matching_brace.rs#L18-L19
if the matching node is a left brace, we use
start()
, if the matching node is a right brace, we should useend()