-
-
Notifications
You must be signed in to change notification settings - Fork 23.8k
Fix ScriptEditor history update issues
#113729
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 ScriptEditor history update issues
#113729
Conversation
|
|
||
| ScriptEditorBase *seb = Object::cast_to<ScriptEditorBase>(n); | ||
| if (seb) { | ||
| lock_history = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not look correct. Please explain why it is fine to not lock the history at all here!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right this breaks a functionality that I didn't know was there (storing line jumps in the history), but with this removed the other issue is fixed. I will look for a better way to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For reference: #113619
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will solve the issue then, you can probably add the issue to your PR. Good that you noticed this
|
Duplicate PR #113619 |
|
Is this really a full duplicate? My PR only aims to solve the locking issue. I was under the impression this PR includes some other fixes. |
|
Yes but I would do a new PR for these to make it clean. Or should I just rework this PR? |
|
Feel free to do a new one if you want! Just wanted to make sure that we don't loose a useful fix here. |
Resolves #107911
This fixes two things, one is that sometimes the history would not update anymore causing the linked issue (cause for this was line 3873). The other is that closing a file when in the middle of the history would cut off the tail of the history even though it is still valid (line 906).
Also I added two helper functions to replace code used multiple times, making the code more readable (in my opinion, at least helped me with debugging). Are these changes valid? If so should I make a extra PR or a proposal?