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 into from
Mar 18, 2014
Merged

Typing performance improvement #7193

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8960b69
don't retrieve tabSize pref until it's needed
redmunds Mar 13, 2014
3803e25
safer way to check if tabSize pref has been retrieved
redmunds Mar 14, 2014
17612b7
protect against modulo zero, and negative tab size
redmunds Mar 14, 2014
f41c78a
Merge remote-tracking branch 'origin/master' into randy/perf-status-bar
redmunds Mar 18, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/editor/Editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -878,12 +878,15 @@ define(function (require, exports, module) {
*/
Editor.prototype.getColOffset = function (pos) {
var line = this._codeMirror.getRange({line: pos.line, ch: 0}, pos),
tabSize = Editor.getTabSize(),
tabSize,
column = 0,
i;

for (i = 0; i < line.length; i++) {
if (line[i] === '\t') {
if (!tabSize) {
Copy link
Copy Markdown
Contributor

@marcelgerber marcelgerber Mar 14, 2014

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

@redmunds redmunds Mar 14, 2014

Choose a reason for hiding this comment

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

Fixed.

tabSize = Editor.getTabSize();
}
column += (tabSize - (column % tabSize));
Copy link
Copy Markdown
Contributor

@TomMalbran TomMalbran Mar 14, 2014

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

@redmunds redmunds Mar 14, 2014

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

@TomMalbran TomMalbran Mar 14, 2014

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

@redmunds redmunds Mar 14, 2014

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.

} else {
column++;
Expand Down
⚡