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

New Insert Code Hints with Tab Preference #6984

Merged
RaymondLim merged 2 commits into from
Mar 4, 2014
Merged

New Insert Code Hints with Tab Preference #6984

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
908868a
New Insert Code Hints with Tab Preference
TomMalbran Feb 23, 2014
c864a25
Removes the setInsertHintOnTab method
TomMalbran Mar 4, 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: 12 additions & 10 deletions src/editor/CodeHintManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,12 +236,13 @@ define(function (require, exports, module) {
"use strict";

// Load dependent modules
var Commands = require("command/Commands"),
CommandManager = require("command/CommandManager"),
EditorManager = require("editor/EditorManager"),
Strings = require("strings"),
KeyEvent = require("utils/KeyEvent"),
CodeHintList = require("editor/CodeHintList").CodeHintList;
var Commands = require("command/Commands"),
CommandManager = require("command/CommandManager"),
EditorManager = require("editor/EditorManager"),
Strings = require("strings"),
KeyEvent = require("utils/KeyEvent"),
CodeHintList = require("editor/CodeHintList").CodeHintList,
PreferencesManager = require("preferences/PreferencesManager");

var hintProviders = { "all" : [] },
lastChar = null,
Expand All @@ -250,9 +251,10 @@ define(function (require, exports, module) {
hintList = null,
deferredHints = null,
keyDownEditor = null;


var _insertHintOnTabDefault = false;

PreferencesManager.definePreference("insertHintOnTab", "boolean", false);


/**
* Determines the default behavior of the CodeHintManager on tab key events.
Expand All @@ -265,7 +267,7 @@ define(function (require, exports, module) {
* selected hint on tab key events.
*/
function setInsertHintOnTab(insertHintOnTab) {
_insertHintOnTabDefault = insertHintOnTab;
PreferencesManager.set("insertHintOnTab", insertHintOnTab);
Copy link
Copy Markdown
Contributor

@marcelgerber marcelgerber Feb 23, 2014

Choose a reason for hiding this comment

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

This function exists to get called by extensions, so this preference can be overriden (even in the pref file) by an extension.
I don't think this is what we want.

We could let extensions use 3 states: forced tab, forced no tab and default (use the pref).

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Feb 23, 2014

Choose a reason for hiding this comment

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

Right, this would also change the Preference in the file which might not be desirable. I think we should just leave the variable as before, but with no initial value. This function would change that value to true or false. We can then add another if on line 478 to use this variable if it is not undefined, and leave the else after to fallback to the preference.

Copy link
Copy Markdown
Member

@peterflynn peterflynn Feb 24, 2014

Choose a reason for hiding this comment

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

Or it might be reasonable to just deprecate or remove this API. Afaik there's only one extension that uses it, and it would be basically obsoleted by the new preference anyway.

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Feb 24, 2014

Choose a reason for hiding this comment

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

Yes. I think that would work better. We could leave the code like this and just add a deprecation warning, to eventually remove this function. Or maybe @iwehrman could upgrade his extension to just set the pref to true and we just remove this function?

Copy link
Copy Markdown
Contributor

@marcelgerber marcelgerber Feb 24, 2014

Choose a reason for hiding this comment

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

I guess we shouldn't rely on the extension this much. I mean, we currently deprecate or break so many APIs that this shouldn't be an issue.

Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Mar 4, 2014

Choose a reason for hiding this comment

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

What is the final solution? Just remove the API?

Copy link
Copy Markdown
Member

@peterflynn peterflynn Mar 4, 2014

Choose a reason for hiding this comment

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

It sounds like we're all ok with that, yeah.

}

/**
Expand Down Expand Up @@ -474,7 +476,7 @@ define(function (require, exports, module) {
if (sessionProvider.insertHintOnTab !== undefined) {
insertHintOnTab = sessionProvider.insertHintOnTab;
} else {
insertHintOnTab = _insertHintOnTabDefault;
insertHintOnTab = PreferencesManager.get("insertHintOnTab");
}

sessionEditor = editor;
Expand Down
1 change: 0 additions & 1 deletion src/language/CodeInspection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,6 @@ define(function (require, exports, module) {
updateListeners();
if (!doNotSave) {
prefs.set(PREF_ENABLED, _enabled);
prefs.save();
}
Copy link
Copy Markdown
Contributor Author

@TomMalbran TomMalbran Feb 23, 2014

Choose a reason for hiding this comment

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

I noticed that this isn't needed anymore with the latest API change.

Copy link
Copy Markdown
Contributor

@RaymondLim RaymondLim Mar 4, 2014

Choose a reason for hiding this comment

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

Nice catch on this.


// run immediately
Expand Down
⚡