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

Transpose Command #435

Merged
merged 5 commits into from
Nov 11, 2014
Merged

Transpose Command #435

merged 5 commits into from
Nov 11, 2014

Conversation

trapgate
Copy link
Contributor

@trapgate trapgate commented Nov 7, 2014

I thought I'd take a crack at issue #47, and implement the transpose command. Feedback welcome, obviously.

Thanks!

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) when pulling 7af7c77 on trapgate:master into b516761 on limetext:master.

@trapgate
Copy link
Contributor Author

trapgate commented Nov 7, 2014

I forgot to implement transpose_test.go, from the looks of it. Give me a bit, I can update.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) when pulling 789c21f on trapgate:master into b516761 on limetext:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.13%) when pulling e5b67a0 on trapgate:master into b516761 on limetext:master.

@FichteFoll
Copy link

Note that this is not a complete implementation of the transpose command as that swaps all the selections' contents if there are multiple.

@@ -0,0 +1,52 @@
// Copyright 2013 The lime Authors.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the file was created 2014, shouldn't it use that date also?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, no time traveling was intended. Fixed.

@quarnster
Copy link
Member

@FichteFoll This swaps all cursors contents too.

@FichteFoll
Copy link

@quarnster

>>> view.run_command("insert", {"characters": "word1 word2 word3\nword4"}); regions = [sublime.Region(i*6, i*6+5) for i in range(4)]; view.sel().add_all(regions); view.run_command("transpose"); print([(r, view.substr(r)) for r in view.sel()])
[((0, 5), 'word4'), ((6, 11), 'word1'), ((12, 17), 'word2'), ((18, 23), 'word3')]

@trapgate
Copy link
Contributor Author

trapgate commented Nov 7, 2014

Thanks for the feedback, I'll make the changes as soon as I can.

Also, @FichteFoll, you're correct. I hadn't noticed this before, but ST3 does swap between active selections - I'll need to implement that too. I was only handling multiple cursor positions, not multiple selected regions.

Rewrite the transpose function to more completely mimic ST3's behavior. Also
add more tests.
@trapgate
Copy link
Contributor Author

trapgate commented Nov 8, 2014

Thanks for the comments. When I did more experimenting the ST3's transpose command it turned out to be more interesting than I realized, so I ended up re-writing my previous attempt. The tests also now check the regions after the transpose against values from ST3 performing the same operation.

In the course of this I found a selection bug in the text layer: Buffer.Word doesn't work correctly if you run it on the last word in the buffer and there's no whitespace between the word and the end of the buffer.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.31%) when pulling 83ce811 on trapgate:master into b516761 on limetext:master.

quarnster added a commit that referenced this pull request Nov 11, 2014
@quarnster quarnster merged commit 270a677 into limetext:master Nov 11, 2014
@quarnster
Copy link
Member

Thanks for your eagle eyes @FichteFoll and excellent work in getting it implemented @trapgate!

@trapgate Could you open up a new issue number in the text repo for the Buffer.Word issue please?

@trapgate
Copy link
Contributor Author

Done: limetext/text#4.

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

Successfully merging this pull request may close these issues.

5 participants