Multiple selections #8814
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "github/fork/twiss/multiple-selections"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
See Issue #778. Not everything is perfect yet, but it works quite well.
Todo:
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
cursorsdiv, 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.
Thanks, that's good to hear! I'll do those things.
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.
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.
Marijn, what do you think about the selection drawing stuff? (
7c1f088,8c9d277and76b15f2) 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.Whoo!
@twiss- thank you SOOO much for working on this! I have really been looking forward to seeing this feature added to CodeMirror.mentioned in issue #4472
@slang800Glad you like it!I'm on holiday right now, but will definitely spend time on this when I get
back.
On Jul 16, 2013 10:27 PM, "Daniel Huigens" notifications@github.com wrote:
Selection drawing is still quite problematic. I think the border might be too much, both in terms of visual noise compared to previous versions, and in terms of the trickiness of getting it right. It currently looks off on both FF and Chrome in Linux (didn't try any other browsers). I was able to fix this by adding border-box sizing rules and a few pixels here and there, but selections that continue across a single line without actually touching still look completely wrong, and I expect more headaches to come up. Probably not worth it.
Why remove this?
I don't see how this won't often leave the cache size at a too-low value.
It would be great if you could put unrelated fixes / changes in different pull requests, so that we only have the actual selection stuff to discuss here.
This appears to correspond to
display.cursorDiv.childNodesat all times. Do we need it?This is a nit, but I'd prefer if you'd follow my style of only adding braces when necessary (or in an
elsechain when one of the branches needs braces)Let's put this class on the wrapper of the cursors instead. Easier to update.
Well, here slightly scary code became full-on terrifying. See my other comment about the borders on the selection. Looking at this, I'm very suspicious that there are a bunch of bugs hiding in it. This is exactly what bug-hiding code looks like. We should somehow simplify it. I just don't know how yet ;)
I'm about to relent on this -- it's a bug-masking change, and I prefer for bugs to be noticeable... but yes, infinite loops are also bad. Very bad, for the user. In any case the change definitely doesn't belong in this pull request.
(Also, I think you should clip the length of the cache array itself as well when you change this.)
IE7 isn't really supported anymore, so feel free to kill the
trykludge.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.unmergedSelectionsrelate todoc.selections? What doesdoc.unmergedSeldo? What exactly are the various ways in which multiple selections can occur? Why does merging the selections only have to look at adjacent ones? I'm a bit too fuzzy on this to properly review the code at the moment.Why is
movingUpan exceptional situation?A comment with the intention of this block would be good. I guess it's a heuristic for whether the paste should be divided over the selections or repeated, but I can't figure out the exact idea.
The implications for the public API are actually not as severe as I expected. On the whole, I think your extensions of the API are quite uncontroversial. One thing I'm not clear about yet is whether we want to preserve 'handles' to selections (allowing querying and updating of a specific selection range through the API), and how the primary selection should be determined. It seems that it is currently always the last one that was made. Was that a conscious choice or a random effect of how the code works?
Tangentially, should
doc.selremain a thing, or would it be cleaner to always have code that accesses a selection be explicit about which one it wants to touch?Thanks for the feedback. I'm on a holiday as well, I'll reply when I'm back.
Fixed in
655e902.Moved to #1734.
Fixed in
962a969.Yes, except that it didn't contain secondary cursors (I tried to continue to do the same thing). Removed in
962a969.I made this change against an older version of CodeMirror, so I'm not even sure if it still does anything. Reverted in
e2e0122.That's good to hear, I hope the same goes for
0fc33fb. Another idea would be to be able to replace command names like"goCharRight"with"each.goCharRight"everywhere, which eliminates the need for every addon to be updated, and is cleaner too, but not, I think, flexible enough on its own.I'm not sure when someone would need that, but we could pass the selection index to the function in
eachSelection()to make things like that easier to do.My idea was to not have a primary selection at all, so
doc.selis now purely the current selection insideeachSelection(). The code, ineachSelectionand elsewhere, that tries to keepdoc.selconsistent is only there because some code seems to depend on that, but I didn't investigate yet.doc.unmergedSelis also part of that. However, because of that, the code to have and maintain a primary selection is already mostly there and now that I'm thinking more about it it seems like a good idea to give editors the option.Well, that would be cleaner for the functions that don't do anything sensible (yet?) outside
eachSelection()(e.g.setSelection,extendSelection,getSelection), but that's not all of them. Maybe it would be easier to just throw if those functions are called outsideeachSelection?I wanted a consistent behaviour that didn't depend on a primary selection, and it seemed like a good idea to be able to take a look at all selections, so now it scrolls to the first selection when
movingUp, the last otherwise. I agree it's bad, do you have a better idea?Added in
3616489.No reason, fixed in
7254213.Fixed in
1993a05, if there's more I've missed please let me know.Maybe have the most recent selection be the primary one, and always scroll that into view?
I find the concept of setting
.sel, and the general API ofeachSelectionquite a bit too magical.How about this:
setSelectionclearing all other selections)Selectionclass that is used to represent selections.addSelectionreturns an instance.get,replace,find,move,extend, andclearto this class.eachSelectionpasses 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.Yes, that's fine too. See
46f44b8and800ff17.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, insidereadInput, which is strange visually and breaksundo.postponeEndOperationputs 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, orreadInputreturns false for some reason).Fixed in
44f6291.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:
Well, that's a possibility but it wouldn't change much by itself since the operation should end after the timeout in
fastPoll(), right?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.unmergedSelectionsfor, to be able to do that.doc.unmergedSelis the index of the primary selection indoc.unmergedSelections. We want to know that because that selection doesn't necessarily exist separately indoc.selections.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.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
mergeSelectionslooks for.I've pushed a version of your code, merged onto the current master branch, to the
multi-selbranch 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.
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.
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.
Alright, go ahead :)
Okay, I'm currently at " aaaaargh!
withSelectionis 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
indentSelectionorkillLineand 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
selAfterargument is completely broken with regards to multiple selections in its current form.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.Any more thoughts here?
@ibdknoxWe're working with@marijnhto get this work completed in November.Awesome. Thanks for the heads up. :)
mentioned in issue #5690
mentioned in issue #483
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).
mentioned in issue #4142
Pull request closed