Skip to content
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

Selection/cursor API Cleanup #1607

Closed
rayat-amperity opened this issue Mar 19, 2022 · 2 comments
Closed

Selection/cursor API Cleanup #1607

rayat-amperity opened this issue Mar 19, 2022 · 2 comments

Comments

@rayat-amperity
Copy link

rayat-amperity commented Mar 19, 2022

TL;DR: Let's cleanup some of the selection/cursor code.


This is an issue to track the following comment made in #610:

I think that generally it would be good to start with cleaning away all the places where selectionLeft is used, where selection.active should have been used instead. I don't know, without investigating, wether there are any cases where we actually need Left/Right semantics. It's probably rare, if at all. In those cases we can probably use functions or dynamic properties that derive from anchor/active.
A PR that only does this. Or only does this at the places we deem perfectly safe, would be a good start, and a huge improvement of the codebase, regardless of multi-cursors. Please also touch base on the slack.

Originally posted by @PEZ in #610 (comment)

link to my clojurians#calva message / thread

tagging my other github account here for easy access @riotrah

@rayat-amperity
Copy link
Author

@PEZ Hi!

I have an important question to ask regarding this work. It is NOT blocking:

  • Calva/Paredit always assumes that the active cursor in a selection is the selection's right-most/end boundary.
    • For example:
      1. User selects some form(s) from right to left with either kb/mouse
      2. They observe thus that the (blinking) active cursor is at the left most position
      3. They invoke some Paredit command
      4. Said command does not acknowledge the cursor's actual position (left-most) and instead behaves as though it is right-most, before Paredit performs the request command
    • In other words, it assumes all selections are "left to right"
    • However, per VSCode reference, this is not always true, and its API exposes associated information about that if desired
    • Specifically, Selections offer - on top of .anchor & .active as already consumed in calva - .start .end .isReversed properties
    • From a UX perspective, I have myself noticed with (so far) very mild annoyance/confusion, when using paredit commands, that my selections are always taken as left-to-right
      • However, those moments were not salient, painful or frequent enough to really remember exact examples, especially as I didn't even know I'd ever be in a position to potentially address them
    • That being said, I do not know at all what the history or reason behind the existing behavior is. For all I know, there might have been some necessary compromise or simplification made.
  • In summation - it would be awesome @PEZ (or anyone) if I could be informed of:
    1. If it is desirable to ever implement said selection direction / cursor position nuance
    2. If doing so would have any negative effect (ie, any special considerations or historic causes?)
    3. If I should do so separately to this work
      a. As preparatory work, I could simply add the properties and API for accessing start/end/isReversed in the models, without changing any code that already reads anchor/active with the preexisting assumptions, so future work could opt-in to said directionality.

I don't believe that it falls "obviously" within the scope of this purely refactorial work to do this, but since I'm touching the same areas, it's worth the ask, perhaps if only to prepare for future work, or to triage for later.

@rayat-amperity
Copy link
Author

Oh, yet another question. This one is slightly more "blocking":

  1. Without implementing multi-cursor, should I still at least assume there are >=1 cursors per document during cursor/selection reads?
    • Implementing multi-cursor amounts to both reading and mutating the models within cursor-doc such that selection expansion, slurps, drags and jumps invoke batch edit operations per command.
    • OTOH purely assuming/reading multi-cursors as subtitled above would only concern the first half.
    • Without any change to functionality, code that accesses selection/cursor will assume there are multiple, but only act upon the "primary" cursor.
      • Paredit currently already implicitly does that (example here), where textEditor.selection is accessed.
      • Per docs, this is simply a shorthand for textEditor.selections[0]
    • So there are two ways to go about this, if greenlit
      1. EditableDocument loses .selection and gains .selections, and previous references change to .selections[0]. Setters are updated so that if .selections = <a single selection> is tried, it auto wraps as single-element array.
      2. EditableDocument loses nothing, but does gain .selections. Getters for .selection just become a shorthand to .selections[0]. Setters to selection/s are updated so that .selection = <a single selection> actually calls selections[0] = [<a single selection>]

Until your/someone's feedback, I won't touch anything related to the above, and will simply cleanup the more obvious area.

  • eg. .selectionLeft ➡️ .selection.anchor etc

riotrah added a commit to rayat-amperity/calva that referenced this issue Mar 26, 2022
riotrah added a commit to rayat-amperity/calva that referenced this issue Apr 1, 2022
PEZ added a commit that referenced this issue Apr 2, 2022
…lection-cursor-api-cleanup

WIP: #1607 - Cleanup selection/cursor API
@bpringe bpringe closed this as completed in d8d7a79 Apr 2, 2022
riotrah added a commit to rayat-amperity/calva that referenced this issue Apr 2, 2022
riotrah added a commit to rayat-amperity/calva that referenced this issue Apr 2, 2022
riotrah added a commit to rayat-amperity/calva that referenced this issue Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant