CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
* | ||
* @type {number} | ||
*/ | ||
this.maxResults = 999; | ||
this.maxResults = maxResults || 1000; |
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 could just do: this.maxResults = Math.min(maxResults || 1000, 1000);
and remove the next 3 lines of code.
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.
I thought the same at first, but then thought it may confuse some.
Let's try it.
Issue #9529 is about a performance lag which it assumes is caused by the length of the code hints list. Did you do any measurements to see what's the difference with and without your changes? It could be that most of the time is with collecting the list (which is not addressed here) as opposed to building/displaying the list (which is addressed here). Might need to pass the |
I haven't done exact measuring, but it felt faster using this branch. I used a workflow similar to the one mentioned in the issue linked above. |
So, I've just measured the time I wonder though if we can improve |
hintList = new CodeHintList(sessionEditor, insertHintOnTab); | ||
if (maxCodeHints <= 0) { | ||
maxCodeHints = undefined; // use default value in CodeHintList instead | ||
} |
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 maxCodeHints
validation code should be moved to CodeHintList
so all validation is done in 1 place.
Done with review. Yes, this is noticeably faster. Just 1 comment. |
@redmunds Ready for final review. |
To improve |
@TomMalbran We're not going to expand the focus of this pull request at this point, but I'm curious as to how you think using transforms could improve performance here. The code hint list is an absolutely positioned element so it should never cause a reflow of anything else, right? |
@marcelgerber I wasn't sure if the validation could would handle non-numeric cases (since the value comes from a json file), so I played around with this a bit. The good news is that all non-numeric types seems to be correctly ignored. The bad news is that floating point numbers seem to cause a problem. When I set value to Need to add some validation code to force value to be a whole number. |
I wast just commenting on something @marcelgerber said and I agree that it should be another PR. It is actually painting a big rectangle at the bottom-left corner of the editor, that it shouldn't. Using translate should also makes it faster to calculate. Maybe the removing the box shadow could help too. |
7a828ac
to
6d9eb94
Compare
@redmunds Integer validation is in. |
@@ -43,7 +44,7 @@ define(function (require, exports, module) { | |||
* @constructor | |||
* @param {Editor} editor |
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.
Need to add a @param
for maxResults
. I can see that it's not part of your PR, but can you also add one for insertHintOnTab
too?
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.
Done.
I'm not seeing that. Open an issue if you have a recipe for it.
I don't think @larz0 would go for that :) |
Looks good. Merging. |
@marcelgerber Can you add new pref to this doc? https://github.com/adobe/brackets/wiki/How-to-Use-Brackets#preferences |
@redmunds Done. @TomMalbran I did some performance measurements and AFAICT, the real bottleneck is not the rendering, but the creation. |
@marcelgerber I was able to remove that loop and create the jQuery objects and pass them to Mustache when rendering the list the first time. But since I still need to create the html from jQuery I am not sure if that makes it any faster. It might get faster if we could give a formatting function to the CodeHintsList and it could use it to format just the items that were required. Maybe using React here could help too, which could allow use to use virtual scrolling. |
@redmunds @marcelgerber Changing the position of an element trigger layout, paint and composition, while using transforms only triggers composition (https://csstriggers.com/) which is why it is faster. I heard that Chrome is trying to fix this, but until then, using transforms should be faster. |
* | ||
* @type {number} | ||
*/ | ||
this.maxResults = 999; | ||
this.maxResults = Math.min( | ||
(maxResults > 0 && ValidationUtils.isInteger(maxResults) && maxResults) || 1000, |
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.
I find the dense logic here a little hard to follow. It seems more clear if we did something like:
this.maxResults = ValidationUtils.isIntegerInRange(maxResults, 1, 1000) ? maxResults : 1000;
or
if (ValidationUtils.isInteger(maxResults) && maxResults > 0) {
this.maxResults = Math.min(maxResults, 1000);
} else {
this.maxResults = 1000;
}
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.
It seems odd to me that the pref defaults to 50 but the API defaults to 1000. Any reason for the difference?
We could still cap the arg at 1000 but have it default to 50 if unspecified -- the 2nd of my code suggestions above would leave the code still pretty easily understandable if we did that.
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.
Yeah, seems reasonable - feel free to open a PR.
But you should first check maxResults > 0
.
For #9529.
I confirmed this is a performance boost for Code Hints.