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

PR: Improve cursor position history (Editor) #16921

Merged
merged 6 commits into from
Dec 24, 2021

Conversation

impact27
Copy link
Contributor

@impact27 impact27 commented Dec 1, 2021

Rework cursor position history

Description of Changes

  • Wrote at least one-line docstrings (for any new functions)
  • Added unit test(s) covering the changes (if testable)
  • Included a screenshot or animation (if affecting the UI, see Licecap)

Current cursor history loops through the entire history at each cursor position change. This PR avoids that.

Issue(s) Resolved

Fixes #

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct:

Rework cursor position history
@impact27 impact27 changed the base branch from master to 5.x December 1, 2021 12:33
@impact27 impact27 changed the title PR: Faster cursor position history PR: Cursor position history Dec 1, 2021
@ccordoba12
Copy link
Member

Thanks a lot for all the improvements @impact27! I'm going to ask @steff456 to take a look at this one because she implemented this functionality.

@ccordoba12 ccordoba12 requested a review from steff456 December 1, 2021 15:36
@ccordoba12 ccordoba12 added this to the v5.2.1 milestone Dec 1, 2021
@ccordoba12 ccordoba12 changed the title PR: Cursor position history PR: Improve cursor position history (Editor) Dec 1, 2021
@impact27 impact27 marked this pull request as draft December 1, 2021 23:18
@impact27
Copy link
Contributor Author

impact27 commented Dec 1, 2021

I will update that to use cursors instead of positions - that way if a line above the cursor is modified, the right position will be retrieved

@impact27 impact27 marked this pull request as ready for review December 2, 2021 09:28
@ccordoba12
Copy link
Member

@impact27, this PR has conflicts now.

@ccordoba12
Copy link
Member

@impact27, you said in the description

Current cursor history loops through the entire history at each cursor position change. This PR avoids that.

Could you expand on this? Does this rework helps to improve performance?

@impact27
Copy link
Contributor Author

impact27 commented Dec 7, 2021

looping through the entire history at each cursor move degrades performance over time when the history is long

@ccordoba12 ccordoba12 modified the milestones: v5.2.1, v5.2.2 Dec 10, 2021
Copy link
Member

@steff456 steff456 left a comment

Choose a reason for hiding this comment

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

I'm curious to know the time improvement taking into account these changes, because I'm seeing that you are creating more iterations over the list.

spyder/plugins/editor/plugin.py Show resolved Hide resolved
Copy link
Member

@steff456 steff456 left a comment

Choose a reason for hiding this comment

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

@ccordoba12 this is working as expected, I also tested it manually, I don't know if you have further comments. For me, this looks good to merge

@ccordoba12
Copy link
Member

Thanks @impact27 for your work on this and @steff456 for the review!

@ccordoba12 ccordoba12 merged commit bc9ae98 into spyder-ide:5.x Dec 24, 2021
ccordoba12 added a commit that referenced this pull request Dec 24, 2021
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.

3 participants