search

helix-editor / helix

A post-modern modal text editor.
https://helix-editor.com
Mozilla Public License 2.0
33.58k stars 2.49k forks source link

Rotating adjacent selections' content breaks selection ranges #7272

Open CptPotato opened 1 year ago

CptPotato commented 1 year ago

Summary

I noticed that adjacent selections break when rotating their content.

Reproduction Steps

A simple way to reproduce this is to select every character in a word ( miw s.<ret> ) and then rotating the content ( A-( / A-) ). The resulting text is correct, but the selection ranges aren't.

Helix log

The log contains nothing related to the issue.

Platform

Windows

Terminal Emulator

wezterm

Helix Version

d1a4bd876 (master from 2023-05-02)

pascalkuthe commented 1 year ago

Hmm I am not entirely sure this is really considered a bug. We actually automatically merge adjacent ranges, anytime we do... anything. Adjacent ranges are sort of in a weird spot. Some commands can create them but they disappear almost immediately.

However considering we got a command to unify ranges I wonder if we should remove the automatic merging of adjacent ranges and only merge actuaually overlapping ranges.

Thaughts @the-mikedavis?

CptPotato commented 1 year ago

Would there be a way to catch this and remove the secondary selections entirely in that case? Since the result is usually very unexpected:

image

image

pascalkuthe commented 1 year ago

That might be fixed by #5930

the-mikedavis commented 1 year ago

I think we can allow some degree of overlapping/adjacent ranges like in https://github.com/helix-editor/helix/issues/2298. We could experiment with allowing overlapping/adjacent ranges in some cases and see what breaks. I suspect there are some places that will need fixes / changes where we assume the invariants from Selection::ensure_invariants are all true.

pascalkuthe commented 1 year ago

i think truly overlapping ranges should still be unified (that won't feel great a lot of day-to-day tasks in helix kind of rely on cursors automatically unifying) and getting that invariant out of the codebase would be pretty hard too (for example it would undo the recent optimization to change mapping) but I think the edge-case of perfectly aligned selections should work without issue (once #5930 is merged) since those are already possible to create right now with `s' (and I can't think of a place where we would rely on that)