Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

Typing performance improvement#7193

Merged
ingorichter merged 4 commits intomasterfrom
randy/perf-status-bar
Mar 18, 2014
Merged

Typing performance improvement#7193
ingorichter merged 4 commits intomasterfrom
randy/perf-status-bar

Conversation

@redmunds
Copy link
Copy Markdown
Contributor

I discovered this when implementing editor preference validation. For EditorStatusBar to display the cursor position, it calls Editor.getCursorPos() with expandTabs option. This causes the "tabSize" preference to be retrieved on every keystroke. This really sucks in a document that doesn't have any Tab characters!

This changes it so the "tabSize" pref is not retrieved until a until a Tab char is found.

This value could be cached to improve it for docs with Tab chars since it rarely changes.

@ingorichter
Copy link
Copy Markdown
Contributor

Great catch!

Comment thread src/editor/Editor.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What if tabSize is 0?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Comment thread src/editor/Editor.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't let the tab size be 0, but if we just return 0 when it is 0, then this code won't break if we ever let it be 0. We know that the column will always be zero if the tabsize is 0.

Does CM, lets you use 0, for tabsize? If so, maybe we can fix other issues like this and make the min be 0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I fixed the code so that if anyone did set tabSize to 0, then we wouldn't retrieve pref on every Tab char.

I can't think of any reason why you'd want tabSize to be 0. Someone recently did something similar (#7020) but it was a hack that should have been done differently. Until I hear of a valid reason for doing it, then I think the minimum tabSize should be 1.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be just for consistency. After the validation code is merged, we could assume that the tab size will be always be a value between 1 and 10. But here you assume that it can be 0, but if it is 0, then (column % tabSize) = NaN, and the code will return NaN, which isn't what you expect. Maybe we can just add if (tabSize = 0) { return 0; }?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. I didn't notice that. Fixed.

@ingorichter
Copy link
Copy Markdown
Contributor

That looks and feels good to me. Do you want to add another test case for the "unlikely" cases of tabSize <= 0? I added this locally for me to check if everything looks good.

@redmunds
Copy link
Copy Markdown
Contributor Author

As soon as the Validate Preferences pull request is merged, then it won't be possible to set a negative value for tabSize, so I don't think this is worth trying to test.

ingorichter added a commit that referenced this pull request Mar 18, 2014
@ingorichter ingorichter merged commit 9f2c33a into master Mar 18, 2014
@TomMalbran TomMalbran deleted the randy/perf-status-bar branch March 18, 2014 05:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants