Multiple selections

[twiss]

See Issue #778. Not everything is perfect yet, but it works quite well.
Todo:

• testing on a mac
• block selecting across the edges doesn't scroll the editor
• multiple block selections (alt+drag, release, ctrl+alt+drag)
• smarter copy & paste
• keyboard shortcuts
• autocomplete
• clearer styling of adjacent selections (with a border or rounded corners), although showCursorWhenSelecting also helps
• the new code in adjustPos is probably wrong (the second branch, which adjacent selections need, breaks the vim and emacs modes, but I don't know vim or emacs so I'm not sure when or why. with a single selection everything is fine though)

[marijnh]

Hey, I just wanted to acknowledge this for now. I've seen it, and on the whole, it looks good. I'd prefer if you factored some of the functions that become much larger with your changes into smaller functions, but my original code was also guilty of huge functions, so it's not an important point, use your judgement.

The way cursors are drawn and maintained is probably cleaner if you just put them all in a single cursors div, clear it before redrawing the selection, and make that blink as a whole. That'd also make the order of the dom elements (which influences drawing order) clearer.

I'll give some more though on how the existing API should interact with the presence of multiple selections. Backward compatibility is a must, of course, but it'd be nice if it were easy for addon writers to just do the simple thing and still have their code work in a multi-selection situation.

I will review the code more closely soon.

[twiss]

Thanks, that's good to hear! I'll do those things.

[marijnh]

Great progress. Is there a reason selections aren't kept in sorted order all the time? That'd save some of the copying and re-sorting.

By the way, github doesn't notify me of new commits pushed to a pull request. So when you think this is ready to merge, or want me to review something, add a comment.

[twiss]

Not really, I changed it. The copying still happens though, because I don't want to merge the selections while selecting.

I'll let you know.

[twiss]

Marijn, what do you think about the selection drawing stuff? (7c1f088, 8c9d277 and 76b15f2) It does make it very clear what you've selected and it looks a bit more polished too, but the code is a real mess, and I'm not even sure I like how it looks, so maybe I should just drop it.

[notslang]

Whoo! @twiss - thank you SOOO much for working on this! I have really been looking forward to seeing this feature added to CodeMirror.

[marijnh]

I find the concept of setting .sel, and the general API of eachSelection quite a bit too magical.

How about this:

• The last selection made is the primary selection.
• The old-style selection methods act on this selection. (with setSelection clearing all other selections)
• We introduce a Selection class that is used to represent selections. addSelection returns an instance.
• We add methods get, replace, find, move, extend, and clear to this class.
• eachSelection passes the selection object to its argument function.

That requires a bit more changes to existing code to properly conform to the multi-selection API, but gives more control, and is easier to reason about.

What is postponeEndOperation? I'm really worried about breaking the invariant that an operation wraps a dynamic scope.

[twiss]

How about this:

Yes, that's fine too. See 46f44b8 and 800ff17.

What is postponeEndOperation?

Without it, when an addon handles a character and selectively returns CodeMirror.Pass to eachSelection, some of the changes to the document are made immediately (by the addon) and some later, inside readInput, which is strange visually and breaks undo. postponeEndOperation puts all changes in one operation. I agree it's scary, but I don't see a very clean solution except removing this functionality or making all the changes immediately, e.g. by parsing the onKeyPress event (I'm not sure that's possible, though). Perhaps it could be made more robust, though, by testing some edge cases (e.g. the handler throws, or readInput returns false for some reason).

[marijnh]

Operations may nest (inner ones become part of the outer one) and
operations that don't change anything are very cheap, so I think you should
be able to simply wrap the whole thing in an operation. Or am I missing
something?
On Aug 14, 2013 3:49 PM, "Daniel Huigens" notifications@github.com wrote:

How about this:

Yes, that's fine too. See 46f44b846f44b8and
800ff17 800ff17.

What is postponeEndOperation?

Without it, when an addon handles a character and selectively returns
CodeMirror.Pass to eachSelection, some of the changes to the document are
made immediately (by the addon) and some later, inside readInput, which
is strange visually and breaks undo. postponeEndOperation puts all
changes in one operation. I agree it's scary, but I don't see a very clean
solution except removing this functionality or making all the changes
immediately, e.g. by parsing the onKeyPress event (I'm not sure that's
possible, though). Perhaps it could be made more robust, though, by testing
some edge cases (e.g. the handler throws, or readInput returns false for
some reason).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1656#issuecomment-22636392
.

[twiss]

Well, that's a possibility but it wouldn't change much by itself since the operation should end after the timeout in fastPoll(), right?

[twiss]

Could you write a bit more (possibly as a comment in the code) on how selecting and merging of selections is supposed to work? How does doc.unmergedSelections relate to doc.selections? What does doc.unmergedSel do?

We want to merge selections while still selecting to be able to draw them correctly, e.g. when you add a selection around a cursor, that cursor should disappear. (Also so that in the edge case that some text is typed while selecting something reasonable happens.)

However, when you move your mouse back to where you started, the merging should be undone. That's doc.unmergedSelections for, to be able to do that. doc.unmergedSel is the index of the primary selection in doc.unmergedSelections. We want to know that because that selection doesn't necessarily exist separately in doc.selections.

What exactly are the various ways in which multiple selections can occur?

Well, you can ctrl-click, ctrl-drag or call addSelection to add one selection, and (ctrl-)alt-drag or (but not on linux) (ctrl-)middle-drag to add a "block" selection, i.e. either selections around all text in the selected area, or a vertical line of cursors. In the current version of mergeSelections, selections can't touch, except if they both have text in them.

Why does merging the selections only have to look at adjacent ones?

Assuming the selections array is always sorted, all pairs or triples of selections that need to be merged will be "in a row", which is what mergeSelections looks for.

[marijnh]

I've pushed a version of your code, merged onto the current master branch, to the multi-sel branch of this repository. Let's work from that now. I did remove your changes to a few addons (active-line, matchbrackets, and matchtags) because the merging was messy and I felt those addons are actually saner if they just act on the main selection.

I'll start reviewing the code in detail and pushing patches when I encounter things I want to do differently. Please, if you do more work on this, work from my merged branch.

[twiss]

Sure! Should I open a new pull request or should I rebase this branch to be the same as yours? Sorry, I'm not very familiar with git. In the matchtags addon, do you still want the jump-to-matching-tag command to work for all selections? It seems bad to just throw selections away.

[marijnh]

Rebasing this branch should work.

I'm having some second thoughts about the API I proposed, as well as about some decisions in your code, so maybe it's best if you just let me run with it for a bit, rather than doing conflicting work.

[twiss]

Alright, go ahead :)

[marijnh]

Okay, I'm currently at " aaaaargh! withSelection is way too subtle, and doesn't work right, but I can't come up with an interface that actually does work conveniently and correctly ".

Will continue thinking about this, but as it stands I'm not sure how to proceed. A big problem is that you'll often want to somehow first compute a changeset from the set of selections, and then apply that atomically. This makes for much more awkward code, for example for indentSelection or killLine and various addons. But iterating over the selections and making changes will change the selections from underneath you, and cause hard-to-reason-about and unlikely-to-be-correct code.

Also, the way of adjusting the selection(s) for a change needs to be deeply rethought. The selAfter argument is completely broken with regards to multiple selections in its current form.

[twiss]

Will continue thinking about this, but as it stands I'm not sure how to proceed. A big problem is that you'll often want to somehow first compute a changeset from the set of selections, and then apply that atomically. This makes for much more awkward code, for example for indentSelection or killLine and various addons. But iterating over the selections and making changes will change the selections from underneath you, and cause hard-to-reason-about and unlikely-to-be-correct code.

Well, what about this: commands are passed (cm, selections), and addons are responsible for iterating over the selections themselves? That's more flexible than what we have now and also more explicit.

[ibdknox]

Any more thoughts here?

[adrocknaphobia]

@ibdknox We're working with @marijnh to get this work completed in November.

[ibdknox]

Awesome. Thanks for the heads up. :)

[marijnh]

My implementation (tracked under #778) is now available. It's a full rewrite, since I wanted a fundamentally more robust model. Feedback welcome (I think I got all the features that are present in this code, except for the rounded corners on the selection, but I'm not entirely sure).