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

Fix a few undo-related bugs #4310

Merged
merged 3 commits into from May 9, 2016

Conversation

Projects
None yet
3 participants
@joelmoniz
Member

joelmoniz commented Feb 17, 2016

This PR attempts to resolve #4280, fix #4302 and close #4303.
#4303 was mostly an issue with not considering Replace All as a compound edit. I have left Replaces to be individually undo-able, since technically each Replace i a distinct operation (unlike Replace Alls, which all happen at once).

I had to modify the JEditTextArea a little for #4280, because the replacement happens from within there, and there's no Editor object accessible (and hence, the Editor's compound edit methods can't be called). The SyntaxDocument has compound edit methods, but they were left blank, and never overridden, so I now have.
#4302 was a little tougher to crack, but primarily boiled down to each insert and deletion being considered a separate compound edit, with replacements (i.e., entries in Insert mode) being modeled as a delete+insert, which kept triggering endCompoundEdit() and starting a new one. Another issue was that a compound edit ends whenever the text in the Editor is the same before and after a key press, which could happen when the user replace a line like int k = 4; with `int x = 5;' (several character overlap here, and the set of characters between each overlapping "block" would be undone at a go, and a block could be as small as a single character).

@benfry There are changes in here to a few of the methods, and how they are called, but I have tried to keep things as neat as possible. Could you please let me know if these changes are ok, if any modification to how changes have been incorporated need to be made, or if further changes are required? Thanks!

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry May 8, 2016

Member

@JakubValtar Can you review?

Member

benfry commented May 8, 2016

@JakubValtar Can you review?

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 8, 2016

Contributor

Reviewing right now.

Contributor

JakubValtar commented May 8, 2016

Reviewing right now.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 8, 2016

Contributor

#4280 is not fixed

Other two are fixed, but I find Undo still terribly broken when more tabs are involved.

When Undo modifies more tabs, the code in Editor is updated, but they are not marked as modified (orange strip), and when I save without visiting the tabs, changes made by undo are not saved (something to do with SketchCode having both Document and String program). This is quite dangerous and confusing as hell.

I was also fighting with this with Rename and I temporarily fixed it in some places (JavaBuild, PreprocessingService) by re-reading documents from all tabs instead of using SketchCode's program.

Also when Undo affects more tabs, you can undo only from one of them, others don't have any Undo available.

I think Undo needs to be fixed before 3.1. The situation with rewriting text from code is already quite convoluted and maybe it would be better to clean it up instead of adding more complexity by merging this. If you want I can look at it tomorrow.

Contributor

JakubValtar commented May 8, 2016

#4280 is not fixed

Other two are fixed, but I find Undo still terribly broken when more tabs are involved.

When Undo modifies more tabs, the code in Editor is updated, but they are not marked as modified (orange strip), and when I save without visiting the tabs, changes made by undo are not saved (something to do with SketchCode having both Document and String program). This is quite dangerous and confusing as hell.

I was also fighting with this with Rename and I temporarily fixed it in some places (JavaBuild, PreprocessingService) by re-reading documents from all tabs instead of using SketchCode's program.

Also when Undo affects more tabs, you can undo only from one of them, others don't have any Undo available.

I think Undo needs to be fixed before 3.1. The situation with rewriting text from code is already quite convoluted and maybe it would be better to clean it up instead of adding more complexity by merging this. If you want I can look at it tomorrow.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry May 9, 2016

Member

Hm, so that would delay 3.1 for a week. Are things any worse with Undo than they have been in the past? Or is it just a matter of it being a high priority?

Member

benfry commented May 9, 2016

Hm, so that would delay 3.1 for a week. Are things any worse with Undo than they have been in the past? Or is it just a matter of it being a high priority?

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 9, 2016

Contributor

@benfry Let's merge this and I'll work on Undo and look at #4280 when it's merged

Contributor

JakubValtar commented May 9, 2016

@benfry Let's merge this and I'll work on Undo and look at #4280 when it's merged

@benfry benfry merged commit 0bcf878 into processing:master May 9, 2016

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry May 9, 2016

Member

Ok, done.

Member

benfry commented May 9, 2016

Ok, done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment