Closed alexrussell closed 10 years ago
This looks pretty reasonable to me overall. I think it makes a lot of sense to work with cursor objects for this case, especially since we need to restore their locations after toggling the quotes. I added a few comments, but it's close.
Thanks @nathansobo for getting back to me on this. I'll do a bit more work on this with your guidance and update.
Okay I've just removed the weirdness about cursor positions for the idea that @nathansobo had, and that kinda cleaned up a few of the other issues. I've kept the fact that I broke the code into two methods as it still makes sense to me to separate the command toggle quotes from the action of toggling quotes at a given position (this could theoretically could make the unit tests simpler if this was ever extended, though for now it would have no effect as we still have to test the four original cases plus my new multiple-cursors case, so I haven't modified them).
There's one interesting case I forgot to mention with my previous code - if you had two cursors in one quoted scope the previous code would leave one where it was and move the other to outside of the range (as the second one's position would only be retrieved after the first one had amended the text and thus move both cursors to the end of the range). The new code simply seems to have the effect of removing the second cursor in the scope. While that means that it doesn't maintain all of the cursors, I think it makes for less odd behaviour, so that's a win in my book.
@kevinsawicki eep! This is slowly going above my level of expertise in both Coffeescript and spec writing. I'll see what I can do!
(Speaking of which I found it relatively difficult to find out the simplest thing of how to iterate over an array in Coffeescript - most examples I saw showed off all sorts of crazy complex situations without showing me the simplest case (i.e. this one you've pointed out) first!)
@kevinsawicki thanks so much for helping me out here. I have implemented the code the way you suggest, though I did the atomicity test as a separate test, if you think it should be part of the main multiple-cursors test let me know and I'll change it.
(Did I do the second test correctly - two it
s under one describe
seemed to work fine (7/7 passed tests), I just don't know if it's okay to do so.)
I appreciate the sentiment of breaking them into two separate it
blocks, but I think in this case that it would be better to merge them into one. The main reason is that in order to test undo, you have to perform all the actions you're testing in the first it
block anyway. In this kind of case, we typically just test undo inline. You can break the test into newline-separated sections to clarify the undo portion. After this, everything is looking pretty good to me. Nice work diving in on this!
Fair enough, the new test has been merged into the previous test.
Looking forward to toggling quotes en masse. Thanks so much.
Woohoo! Thanks both for leading me down the right path and accepting.
I can't guarantee my Coffeescript skills are tip top here as I don't use it, nor whether it could have been done a better way.
Screenshot/movie:
I have a feeling the Atom core needs to open up a little on the cursors front - the previous code used
editor.getCursorBufferPosition()
to get the position of the cursor, but that assumes a single cursor (and it defaults to the last cursor in the cursors array). In order to support multiple cursors, I had to useeditor.getCursors()
to get all the cursors theneditor.displayBuffer.bufferRangeForScopeAtPosition('.string.quoted', cursor.getBufferPosition())
which, due to its use of theeditor.displayBuffer
object directly is probably not the best use of the API.I have added a test here to test that multiple cursors work, though I haven't documented my methods. The original code was undocumented and I have a feeling I'd have done it wrong anyway, so I've left it not-done.