Skip to content

Conversation

@AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Dec 8, 2023

This frequently bites me. "two states" refers to the if statement and the second print statement, but "next state" refers to the first print statement. This change makes it so it skips containing and refers to the second one instead.

    if ( | true) {
        console.log(1)
    }

    console.log(2)

Checklist

  • I have added tests
  • [-] I have updated the docs and cheatsheet
  • [-] I have not broken the cheatsheet

@pokey
Copy link
Member

pokey commented Dec 8, 2023

I think both today's behaviour and your proposed behaviour are useful. And I think both can be equally surprising. It would be quite surprising to pop out of a "curly" you can't see when you say "next curly"

Why don't we support both as separate modifiers? You can map it to "next", and I'll map it to "skip", eg "skip funk", "skip second funk", etc

If we find the other is more often useful, and less often surprising, we can change default spoken form

Make sense?

@AndreasArvidsson
Copy link
Member Author

AndreasArvidsson commented Dec 8, 2023

I'm fine with having two separate modifiers, but I do think having one canonical way of numbering the scopes and then using the same logic for two scopes and next scope is much easier for the users to grasp. I feel myself that I don't really know what Cursorless will use when I issue these different commands.

Should we make a brand new modifier or just add an additional parameter to the modifier? We could also make it a private setting and try it out?

@AndreasArvidsson
Copy link
Member Author

I think there may also be something interaction with interior we haven't experimented with yet. Curly has an interior that may change the behavior compared to statements.

@pokey
Copy link
Member

pokey commented Dec 12, 2023

Should we make a brand new modifier or just add an additional parameter to the modifier? We could also make it a private setting and try it out?

yes I'd make it private. Not sure bout new modifier vs param. One thing to keep in mind: the impl is very very close to RelativeInclusive, so I would be inclined to fold it in there rather than trying to handle both in RelativeExclusive. Or make a separate class. But I don't think it belongs in RelativeExclusive

@pokey pokey added the to discuss Plan to discuss at meet-up label Dec 12, 2023
@AndreasArvidsson
Copy link
Member Author

Should we make a brand new modifier or just add an additional parameter to the modifier? We could also make it a private setting and try it out?

yes I'd make it private. Not sure bout new modifier vs param. One thing to keep in mind: the impl is very very close to RelativeInclusive, so I would be inclined to fold it in there rather than trying to handle both in RelativeExclusive. Or make a separate class. But I don't think it belongs in RelativeExclusive

Implementation wise it might be that, but its still most definitely relative exclusive since we have offset 1?

@pokey
Copy link
Member

pokey commented Dec 12, 2023

Implementation wise it might be that, but its still most definitely relative exclusive since we have offset 1?

yes but keeping it as part of the same impl might be beneficial as it ensures the semantics is exactly the same

@AndreasArvidsson
Copy link
Member Author

We can always make utility function for that part

@AndreasArvidsson
Copy link
Member Author

Closed in favor of #2133

@AndreasArvidsson AndreasArvidsson deleted the relativeScopeInitialRange branch May 6, 2025 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

to discuss Plan to discuss at meet-up

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants