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

integration of code-folding extension into brackets #10792

Merged
merged 13 commits into from Apr 7, 2015
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
127 changes: 127 additions & 0 deletions src/extensions/default/CodeFolding/Prefs.js
@@ -0,0 +1,127 @@
/**
* Wrapper around brackets pref system to ensure preferences are stored in in one single object instead of using multiple keys.
* This is to make it easy for the user who edits their preferences file to easily manage the potentially numerous lines of preferences generated by the persisting code-folding state.
* @author Patrick Oladimeji
* @date 3/22/14 20:39:53 PM
*/
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, brackets*/
define(function (require, exports, module) {
"use strict";
var PreferencesManager = brackets.getModule("preferences/PreferencesManager"),
prefs = PreferencesManager.getExtensionPrefs("code-folding"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Any objection to renaming code-folding to maybe brackets-code-folding? Simply to helps everyone keep settings separate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in principle no objection. But I would suggest that I change the prefs in the version in the registry to "brackets-code-folding" for consistency - to match the name in the package.json.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be great. It would also help existing users keep saved settings from the extension.

Copy link
Contributor

Choose a reason for hiding this comment

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

@thehogfather Are you proposing that the integrated and the one in the registry should have the same preferences name? Or are you saying that instead of changing this integrated version we change the one in the registry?

What I expect is that users will remove the code folding extension when the new version of brackets comes out. At which point it doesn't really matter what we call the preferences. But if the plan is to continue to move the extension in the registry forward, then giving this integrated version the new name will let the users of the extension continue to use it without needing to migrate their settings.

Maybe I am lacking some understanding. @thehogfather is your plan to continue to develop the extension in the registry and have competing functionality with what we are integrating? Because I am not sure I see the purpose of having this integrated if the one in the registry will continue to provide the same functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MiguelCastillo no. Just proposing that the preference name should match the name in the package.json. So currently the version in the registry stores preferences using the "code-folding" key. I'll update it to "brackets-code-folding" and leave the version integrated into brackets as "code-folding". This also means any preferences saved in the old extension will be kept in the integrated version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. That makes sense. I was just curious if we wanted users to just transparently use the settings they have. In which case I expect them to remove the extension.

strings = brackets.getModule("strings"),
store = {},
foldsKey = "folds";

//default preference values
prefs.definePreference("enabled", "boolean", true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think any other Brackets code uses the fourth parameter options just yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No I didn't think so either. But I thought if the API exists why not use it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the extra meta data in there :)

{name: strings.ENABLE_CODE_FOLDING, description: strings.ENABLE_CODE_FOLDING});
prefs.definePreference("minFoldSize", "number", 2,
{name: strings.MIN_FOLD_SIZE, description: strings.MIN_FOLD_SIZE_HELP});
prefs.definePreference("saveFoldStates", "boolean", true,
{name: strings.SAVE_FOLD_STATES, description: strings.SAVE_FOLD_STATES_HELP});
prefs.definePreference("alwaysUseIndentFold", "boolean", true,
{name: strings.ALWAYS_USE_INDENT_FOLD, description: strings.ALWAYS_USE_INDENT_FOLD_HELP});
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean to have this enabled? What is the behavior e.g. in a JS file if this conflicts with the standard brace-fold/comment-fold helpers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means that line indentation is used as a suggestion for fold regions. Although this shouldn't take priority over range finders based on the current mode. So where there is conflict, the brace-fold-finder should take precedence. Having said that this might be another pref that could be better off disabled by default.

prefs.definePreference("enableRegionFolding", "boolean", true,
{name: strings.ENABLE_REGION_FOLDING, description: strings.ENABLE_REGION_FOLDING});
prefs.definePreference("fadeFoldButtons", "boolean", false,
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this preference should be on by default.

I can't actually get it to work though -- when I set it to true, it seems to have no effect (at least on JS or LESS files). Will file a bug.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, false alarm with the preferences issue.

I wonder if the best UX is actually in between this setting (hidden until mouseover) and the default (always visible) -- having the triangles be fainter until you mouse over the gutter, then getting darker. I may mock that up and see how it feels...

{name: strings.FADE_FOLD_BUTTONS, description: strings.FADE_FOLD_BUTTONS_HELP});
prefs.definePreference("maxFoldLevel", "number", 2,
{name: strings.MAX_FOLD_LEVEL, description: strings.MAX_FOLD_LEVEL_HELP});
prefs.definePreference("folds", "object", {});

/**
Simplifies the fold ranges into an array of pairs of numbers.
Copy link
Contributor

Choose a reason for hiding this comment

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

@thehogfather - do you think we can get all the jsdocs to follow the conventions followed in Brackets?

/**
  *  Simplifies the fold ranges into an array of pairs of numbers.
  *
  * @param {!{number: {from: {ch, line}, to: {ch, line}} folds the raw fold ranges indexed by line numbers
  * @returns {number: Array<Array<number>>} an object whose keys are line numbers and the values are array
  *        of two 2-element arrays. First array contains [from.line, from.ch] and the second contains [to.line, to.ch]
  */

This is a good example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MiguelCastillo not a problem. Thanks for the pointer.

@param {!{number: {from: {ch, line}, to: {ch, line}} folds the raw fold ranges indexed by line numbers
@returns {number: Array<Array<number>>} an object whose keys are line numbers and the values are array
of two 2-element arrays. First array contains [from.line, from.ch] and the second contains [to.line, to.ch]
*/
function simplify(folds) {
if (!folds) {
return;
}
var res = {}, range;
Object.keys(folds).forEach(function (line) {
range = folds[line];
res[line] = Array.isArray(range) ? range : [[range.from.line, range.from.ch], [range.to.line, range.to.ch]];
});
return res;
}
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

add a courtesy new line right here :D

Inflates the fold ranges stored as simplified numeric arrays. The inflation converts the data into
objects whose keys are line numbers and whose values are objects in the format {from: {line, ch}, to: {line, ch}}.
@param {number: Array<Array<number>>} folds the simplified fold ranges
@returns {number: {from: {line, ch}, to: {line, ch}}}
*/
function inflate(folds) {
if (!folds) {
return;
}
//transform the folds into objects with from and to properties
var ranges = {}, obj;
Object.keys(folds).forEach(function (line) {
obj = folds[line];
ranges[line] = {from: {line: obj[0][0], ch: obj[0][1]}, to: {line: obj[1][0], ch: obj[1][1]}};
});

return ranges;
}

/**
Gets the line folds saved for the specified path.
@param {string} path the document path
@returns {number: {from: {line, ch}, to: {line, ch}}} the line folds for the document at the specified path
*/
function get(path) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we rename this get and set to getFolds and setFolds? It improves intent and readability. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure - I agree.

store = (prefs.get(foldsKey) || {});
return inflate(store[path]);
}

/**
Saves the line folds specified
*/
function set(path, folds) {
store[path] = simplify(folds);
Copy link
Member

Choose a reason for hiding this comment

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

This really should be in view state and not the main preferences file -- it's exactly the kind of thing view state was intended for. This seems like something we should fix for 1.3 lest we start polluting everyone's preferences file (which should be easily human-readable) with folding state.

Copy link
Member

Choose a reason for hiding this comment

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

Filed as #10846 to make sure we track this

Copy link
Contributor

Choose a reason for hiding this comment

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

@thehogfather #10846 Instead of using the prefs , could we migrate the folding state to state.json?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abose yes - for the folds. I suspect the other settings would need to be in the regular prefs (brackets.json).

Copy link
Contributor

Choose a reason for hiding this comment

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

@thehogfather could use PreferencesManager.getViewState("codefolding.states"); and PreferencesManager.setViewState("codefolding.states", state) to put to the state.json file.

prefs.set(foldsKey, store);
}

/**
Get the code folding setting with the specified key from the store
@param {!string} key The key for the setting to retrieve
@returns {string} the setting with the specified key
*/
function getSetting(key) {
return prefs.get(key, PreferencesManager.CURRENT_PROJECT);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abose @ryanstewart I need some clarification on how to retrieve preferences from the correct scope. For the sake of argument, the above line is attempting to retrieve code-folding settings that have been entered at the project level (i.e., using .brackets.json in a project dir). But it appears that the values in that file are actually not being returned in this instance. In fact after a little debugging, the project scope does not appear in the scope order used in the _getContext function defined in PreferencesManager (perhaps because there is no path property in the context sent from PreferencesManager.CURRENT_PROJECT). What is the correct way to use this api?

Copy link
Contributor

Choose a reason for hiding this comment

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

@thehogfather prefs.get(key) is supposed to return the value by first searching the project .brackets.json, and if the key isn't found there, it will search the use (global) brackets.json. You don't need the second parameter. But it seems like you are after something a bit different. Can explain on what you are trying to achieve?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MiguelCastillo thanks, that was how I thought it worked. All I am trying to do is to override any code-folding preferences set at the user or default level with one set at the project level. For instance placing "code-folding.enabled": false in .brackets.json file to disable code-folding. I couldn't get this to work consistently yesterday so I thought I'd 'force' it to look at the project scope using the second parameter as suggested by the wiki. At the moment, when I put a breakpoint in developer console to debug, it works as expected. Without the breakpoint it doesn't seem to be reading the correct preference value.
Any insights or questions would be useful. I'll also have a closer look later today.
Cheers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually all is working consistently and as expected now.

}

/**
Gets all the values for code folding settings.
@returns {object} all settings saved in the preferences.
*/
function getAllSettings() {
var res = {};
prefs.getKeys().forEach(function (key) {
res[key] = getSetting(key);
});
return res;
}

/**
Clears all the saved line folds for all documents.
*/
function clearAllFolds() {
prefs.set(foldsKey, {});
Copy link
Member

Choose a reason for hiding this comment

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

This function appears to be unused, but I think it also has a bug: this doesn't reset the store var used elsewhere, so any call to setFolds() later seems like it would immediately restore all the folds that had been cleared previously.

It's probably safest just to re-get the pref before setting it each time. There may be other bugs lurking with the current approach too (e.g. if you edit the prefs file... does it blow those changes away?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterflynn yes editing the prefs file for documents that are currently open in brackets (has no effect - the changes are restored one the document becomes the active editor). This is a bug - that your safe approach should fix.

}

module.exports.get = get;

module.exports.set = set;

module.exports.getSetting = getSetting;

module.exports.getAllSettings = getAllSettings;

module.exports.clearAllFolds = clearAllFolds;

});
226 changes: 226 additions & 0 deletions src/extensions/default/CodeFolding/foldhelpers/foldcode.js
@@ -0,0 +1,226 @@
/**
* Based on http://codemirror.net/addon/fold/foldcode.js
* @author Patrick Oladimeji
Copy link
Member

Choose a reason for hiding this comment

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

I think we still need to keep the original copyright from CodeMirror here

* @date 10/28/13 8:41:46 AM
* @last modified 20 April 2014
*/
/*jslint vars: true, plusplus: true, devel: true, nomen: true, indent: 4, maxerr: 50 */
/*global define, brackets, document*/
define(function (require, exports, module) {
"use strict";
var CodeMirror = brackets.getModule("thirdparty/CodeMirror2/lib/codemirror"),
prefs = require("Prefs");

function doFold(cm, pos, options, force) {
options = options || {};
force = force || "fold";
if (typeof pos === "number") {
pos = CodeMirror.Pos(pos, 0);
}
var finder = options.rangeFinder || CodeMirror.fold.auto;
var minSize = options.minFoldSize || prefs.getSetting("minFoldSize");

function getRange(allowFolded) {
var range = options.range || finder(cm, pos);
if (!range || range.to.line - range.from.line < minSize) {
return null;
}
var marks = cm.findMarksAt(range.from),
i,
lastMark,
foldMarks;
for (i = 0; i < marks.length; ++i) {
if (marks[i].__isFold && force !== "fold") {
if (!allowFolded) {
return null;
}
range.cleared = true;
marks[i].clear();
}
}
//check for overlapping folds
if (marks && marks.length) {
foldMarks = marks.filter(function (d) {
return d.__isFold;
});
if (foldMarks && foldMarks.length) {
lastMark = foldMarks[foldMarks.length - 1].find();
if (lastMark && range.from.line <= lastMark.to.line && lastMark.to.line < range.to.line) {
return null;
}
}
}
return range;
}

function makeWidget() {
var widget = document.createElement("span");
widget.className = "CodeMirror-foldmarker";
return widget;
}

var range = getRange(true);
if (options.scanUp) {
while (!range && pos.line > cm.firstLine()) {
pos = CodeMirror.Pos(pos.line - 1, 0);
range = getRange(false);
}
}
if (!range || range.cleared || force === "unfold" || range.to.line - range.from.line < minSize) {
if (range) { range.cleared = false; }
return;
}

var myWidget = makeWidget();
Copy link
Contributor

Choose a reason for hiding this comment

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

Convention in Brackets is that variables are declared/defined at the beginning of the method scoping them.

Copy link
Member

Choose a reason for hiding this comment

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

@MiguelCastillo Fwiw, not necessarily true. Because of JSLint, it's safe (often actually safer) to declare vars further down in a function at the point when they're initialized. We do have plenty of code in core where vars are not all grouped at the top of the scope.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm, the only reason I say this is because JSLint would complain if your vars werent declared at the top of your functions. I am guessing this changed. Good to know though. :)

Copy link
Member

Choose a reason for hiding this comment

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

We use the vars JSLint option, which lets you declare vars anywhere in the scope -- as long as they're not duplicated or referenced before the declaration. It's sort of a best of both worlds, because it effectively makes var declarations work more like Java or C++ (assuming you don't ignore lint errors).

var myRange = cm.markText(range.from, range.to, {
replacedWith: myWidget,
clearOnEnter: true,
__isFold: true
});
CodeMirror.on(myWidget, "mousedown", function () {
myRange.clear();
});
myRange.on("clear", function (from, to) {
delete cm._lineFolds[from.line];
CodeMirror.signal(cm, "unfold", cm, from, to);
});

if (force === "fold") {
delete range.cleared;
cm._lineFolds[pos.line] = range;
} else {
delete cm._lineFolds[pos.line];
}
CodeMirror.signal(cm, force, cm, range.from, range.to);
return range;
}

/**
Initialises extensions and helpers on the CodeMirror object
*/
function init() {
CodeMirror.defineExtension("foldCode", function (pos, options, force) {
return doFold(this, pos, options, force);
});

CodeMirror.defineExtension("unfoldCode", function (pos, options) {
return doFold(this, pos, options, "unfold");
});

CodeMirror.registerHelper("fold", "combine", function () {
var funcs = Array.prototype.slice.call(arguments, 0);
return function (cm, start) {
var i;
for (i = 0; i < funcs.length; ++i) {
var found = funcs[i] && funcs[i](cm, start);
if (found) {
return found;
}
}
};
});

CodeMirror.defineExtension("isFolded", function (line) {
return this._lineFolds[line];
});
/**
Checks the validity of the ranges passed in the parameter and returns the foldranges
that are still valid in the current document
@param {object} folds the dictionary of lines in the current document that should be folded
@returns {object} valid folds found in those passed in parameter
*/
CodeMirror.defineExtension("getValidFolds", function (folds) {
var keys, rf = CodeMirror.fold.auto, cm = this, result = {};
if (folds && (keys = Object.keys(folds)).length) {
var range, cachedRange;
keys.forEach(function (lineNumber) {
lineNumber = +lineNumber;
if (lineNumber >= cm.firstLine() && lineNumber <= cm.lastLine()) {
range = rf(cm, CodeMirror.Pos(lineNumber));
cachedRange = folds[lineNumber];
if (range && cachedRange && range.from.line === cachedRange.from.line &&
range.to.line === cachedRange.to.line) {
cm.foldCode(lineNumber, {range: folds[lineNumber]}, "fold");
result[lineNumber] = folds[lineNumber];
}
}
});
}
return result;
});

CodeMirror.commands.toggleFold = function (cm) {
cm.foldCode(cm.getCursor());
};
CodeMirror.commands.fold = function (cm, options, force) {
cm.foldCode(cm.getCursor(), options, "fold");
};
CodeMirror.commands.unfold = function (cm, options, force) {
cm.foldCode(cm.getCursor(), options, "unfold");
};

CodeMirror.commands.foldAll = function (cm) {
cm.operation(function () {
var i, e;
for (i = cm.firstLine(), e = cm.lastLine(); i <= e; i++) {
cm.foldCode(CodeMirror.Pos(i, 0), null, "fold");
}
});
};

CodeMirror.commands.unfoldAll = function (cm, from, to) {
from = from || cm.firstLine();
to = to || cm.lastLine();
cm.operation(function () {
var i, e;
for (i = from, e = to; i <= e; i++) {
if (cm.isFolded(i)) { cm.unfoldCode(i, {range: cm._lineFolds[i]}); }
}
});
};
/**
Folds the specified range. The descendants of any fold regions within the range are also folded up to
a level set globally in the codeFolding preferences
*/
CodeMirror.commands.foldToLevel = function (cm, start, end) {
var rf = CodeMirror.fold.auto, level = prefs.getSetting("maxFoldLevel");
function foldLevel(n, from, to) {
if (n > 0) {
var i = from, range;
while (i < to) {
range = rf(cm, CodeMirror.Pos(i, 0));
if (range) {
//call fold level for the range just folded
foldLevel(n - 1, range.from.line + 1, range.to.line - 1);
cm.foldCode(CodeMirror.Pos(i, 0), null, "fold");
i = range.to.line + 1;
} else {
i++;
}
}
}
}
cm.operation(function () {
start = start === undefined ? cm.firstLine() : start;
end = end || cm.lastLine();
foldLevel(level, start, end);
});
};

CodeMirror.registerHelper("fold", "auto", function (cm, start) {
var helpers = cm.getHelpers(start, "fold"), i, cur;
//ensure mode helper is loaded if there is one
var mode = cm.getMode().name;
var modeHelper = CodeMirror.fold[mode];
if (modeHelper && helpers.indexOf(modeHelper) < 0) {
helpers.push(modeHelper);
}
for (i = 0; i < helpers.length; i++) {
cur = helpers[i](cm, start);
if (cur) { return cur; }
}
});
}

exports.init = init;
});