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

Add new pref maxCodeHints #9791

Merged
merged 4 commits into from
Nov 21, 2014
Merged

Add new pref maxCodeHints #9791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7048173
Add new pref maxCodeHints
Nov 2, 2014
d9bf2d7
ValidationUtils improvements
Nov 20, 2014
6d9eb94
Validate maxResults is an integer
Nov 20, 2014
98ee9ae
JSDoc
Nov 21, 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
22 changes: 14 additions & 8 deletions src/editor/CodeHintList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,13 @@ define(function (require, exports, module) {
"use strict";

// Load dependent modules
var Menus = require("command/Menus"),
var KeyBindingManager = require("command/KeyBindingManager"),
Menus = require("command/Menus"),
KeyEvent = require("utils/KeyEvent"),
StringUtils = require("utils/StringUtils"),
PopUpManager = require("widgets/PopUpManager"),
ValidationUtils = require("utils/ValidationUtils"),
ViewUtils = require("utils/ViewUtils"),
KeyBindingManager = require("command/KeyBindingManager"),
KeyEvent = require("utils/KeyEvent");
PopUpManager = require("widgets/PopUpManager");

var CodeHintListHTML = require("text!htmlContent/code-hint-list.html");

Expand All @@ -42,8 +43,10 @@ define(function (require, exports, module) {
*
* @constructor
* @param {Editor} editor
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

* @param {boolean} insertHintOnTab Whether pressing tab inserts the selected hint
* @param {number} maxResults Maximum hints displayed at once. Defaults to 1000
*/
function CodeHintList(editor, insertHintOnTab) {
function CodeHintList(editor, insertHintOnTab, maxResults) {

/**
* The list of hints to display
Expand All @@ -60,11 +63,14 @@ define(function (require, exports, module) {
this.selectedIndex = -1;

/**
* The maximum number of hints to display
* The maximum number of hints to display. Can be overriden via maxCodeHints pref
*
* @type {number}
*/
this.maxResults = 999;
this.maxResults = Math.min(
(maxResults > 0 && ValidationUtils.isInteger(maxResults) && maxResults) || 1000,
Copy link
Member

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;
}

Copy link
Member

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.

Copy link
Contributor Author

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.

1000
);

/**
* Is the list currently open?
Expand Down Expand Up @@ -215,7 +221,7 @@ define(function (require, exports, module) {
}
} else {
this.hints.some(function (item, index) {
if (index > self.maxResults) {
if (index >= self.maxResults) {
return true;
}

Expand Down
6 changes: 4 additions & 2 deletions src/editor/CodeHintManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ define(function (require, exports, module) {

PreferencesManager.definePreference("showCodeHints", "boolean", true);
PreferencesManager.definePreference("insertHintOnTab", "boolean", false);
PreferencesManager.definePreference("maxCodeHints", "integer", 50);

PreferencesManager.on("change", "showCodeHints", function () {
codeHintsEnabled = PreferencesManager.get("showCodeHints");
Expand Down Expand Up @@ -484,7 +485,8 @@ define(function (require, exports, module) {

// If a provider is found, initialize the hint list and update it
if (sessionProvider) {
var insertHintOnTab;
var insertHintOnTab,
maxCodeHints = PreferencesManager.get("maxCodeHints");
if (sessionProvider.insertHintOnTab !== undefined) {
insertHintOnTab = sessionProvider.insertHintOnTab;
} else {
Expand All @@ -493,7 +495,7 @@ define(function (require, exports, module) {

sessionEditor = editor;

hintList = new CodeHintList(sessionEditor, insertHintOnTab);
hintList = new CodeHintList(sessionEditor, insertHintOnTab, maxCodeHints);
hintList.onSelect(function (hint) {
var restart = sessionProvider.insertHint(hint),
previousEditor = sessionEditor;
Expand Down
9 changes: 6 additions & 3 deletions src/utils/ValidationUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ define(function (require, exports, module) {
* Used to validate whether type of unknown value is an integer.
*
* @param {*} value Value for which to validate its type
* @return {boolean} true if value is an integer
* @return {boolean} true if value is a finite integer
*/
function isInteger(value) {
// Validate value is a number
Expand All @@ -42,9 +42,12 @@ define(function (require, exports, module) {
}

// Validate number is an integer
if (value > 0 && Math.floor(value) !== value) {
if (Math.floor(value) !== value) {
return false;
} else if (value < 0 && Math.ceil(value) !== value) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The following only works for positive integers, right?

        if (Math.floor(value) !== value) {
            return false;
        }

maxCodeHints will always be positive, but this validation code needs to also handle negative integers as the code was already doing:

        if (value > 0 && Math.floor(value) !== value) {
            return false;
        } else if (value < 0 && Math.ceil(value) !== value) {
            return false;
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Math.floor() of an integer returns the input, no matter it's signed or not (so, Math.floor(-5) is -5).
See also MDN's Polyfill.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind -- I can see that code works just as well. There's no need to correctly truncate any fraction -- just determine whether or not there is a fraction.


// Validate number is finite
if (!isFinite(value)) {
return false;
}

Expand Down