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

Carriage returns undo and redo one at a time #18

Closed
aaxu opened this issue Jun 29, 2017 · 6 comments
Closed

Carriage returns undo and redo one at a time #18

aaxu opened this issue Jun 29, 2017 · 6 comments
Assignees

Comments

@aaxu
Copy link
Contributor

aaxu commented Jun 29, 2017

Do you think it'll be better if we undo and redo new lines the same way we do mouse actions? That is, we return a new line character if we see one after a redo. However, we only redo 2 different types of actions at a time, so if the redo chain was something like (insert, mouse, newLines), we would only undo the newLines (until there are no more left), then the mouse action.

@dschuyler
Copy link
Collaborator

I agree that that what get's done or undone as a unit could be improved. This is an area where it seems to help to try out ideas. I.e. I'd suggest trying a change for a bit to see if you like it. I've done that a couple times and found that some idea I had caused other consequences I didn't like. That's not a critique of your suggestion. I think it sounds like a good idea, but I can't be sure until trying it for awhile. Does that make sense?

@dschuyler
Copy link
Collaborator

If you want to make a PR, I'm happy to pull it in locally and try it out :)

@aaxu
Copy link
Contributor Author

aaxu commented Jul 6, 2017

Oh yeah for sure! I'll work on it. The current thing is that right now, carriage returns are always followed by a cursor move, so some if statements are never actually used and carriage returns are also never combined. I'm gonna try and clean this up a bit.

One question: Is there a particular reason why the redoAddChange for carriage returns is a tuple of 1 element?

@dschuyler
Copy link
Collaborator

This seems good to me. I've been running with the patch (PR) for awhile.

On the carriage returns tuple. IIRC, it's just that was one of the first things I did when I started writing the editor. I think I was making all redo records hold a tuple. I.e. there's a reason, but not necessarily a good reason. :)

@dschuyler
Copy link
Collaborator

Oh and is this enhancement ready to be closed?

@aaxu
Copy link
Contributor Author

aaxu commented Jul 25, 2017

Yeah I think it's finished so close away!

Edit: oh I can close it too LOL

@aaxu aaxu closed this as completed Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants